MBS-13920 (II): Detect feat. artists in titles when seeding

[derat]

MBS-13920

Make the release editor display an error (and checkbox to bypass) when featured artists appear in seeded track titles. Previously, the error and checkbox were only displayed for manually-entered titles.

Problem

The release editor incorrectly treats featured artists as already having been present in seeded track titles. As a result, editors can skip the warning without needing to confirm that the titles are deliberate when seeding releases.

Solution

Update Track's constructor to first check that the track already has an ID before treating the supplied title as original for the purposes of initializing the hasFeatOnOrigTitle field. Also update Medium's constructor to use hasOrigFeatOnTitle rather than hasFeatOnTitle() when choosing the initial state for confirmedFeatOnTrackTitles.

Testing

Manually checked that I now see an error and checkbox when seeding a release with a featured artist in the title (I used https://volaband.bandcamp.com/album/friend-of-a-phantom). If I edit a release that already has featured artists in titles, I see only a warning (as before).

[derat]

Is this safe for checking that the track already existed? (I used != rather than !== to match the code above that initializes the id field.)

[mwiencek]

I believe that is safe in the release editor. IIRC, there are other parts of the codebase (e.g., the React relationship editor) where we use negative IDs to indicate a new/pending entity. In those cases we use root/static/scripts/common/utility/isDatabaseRowId.js to check whether it's a positive postgres INTEGER.

[derat]

Thanks, I've updated this statement to use isDatabaseRowId since it feels a bit safer.

[derat]

@reosarevok, I think that this fixes the rest of the issue.

[derat]

@brainzbot, retest this please

[reosarevok]

Tested and it seems to work nicely - I'd still want @mwiencek to take a second look and answer your open question though :)

[mwiencek]

Not sure I follow this bit: can tracksUnknownToUser ever be true here in the constructor? AFAICT it can only become true in tracksLoaded, or by the user ticking it.

Edit: I realized maybe this was intended to be a (writable) computed observable, though I'm still not sure if the tracksUnknownToUser check is actually needed.

[derat]

I wasn't sure about this either, so I was trying to play it safe (and lazy) by just copying over the expression used to initialize hasFeatOnTrackTitles above while switching it to use hasFeatOnOrigTitle rather than hasFeatOnTitle.

Does tracksLoaded always get called after the constructor when loading a medium from an existing release? If so, should confirmedVariousArtists and confirmedFeatOnTrackTitles both just be initialized to false here before being updated in tracksLoaded? (I think the intent was for the checkboxes to start out unchecked unless there are already tracks that possibly aren't following the guidelines, right?)

[mwiencek]

Does tracksLoaded always get called after the constructor when loading a medium from an existing release?

tracksLoaded is always called asynchronously for existing releases (i.e., mediums are always loaded async), so it does appear that tracksUnknownToUser will always be false in the constructor.

If so, should confirmedVariousArtists and confirmedFeatOnTrackTitles both just be initialized to false here before being updated in tracksLoaded? (I think the intent was for the checkboxes to start out unchecked unless there are already tracks that possibly aren't following the guidelines, right?)

Yeah, IIRC the intent was to only pre-check them if all initial incorrect tracks were pre-existing issues, so as not to force the user to repair them.

I think it would make sense to just initialize confirmedVariousArtists and confirmedFeatOnTrackTitles to false. Actually, I'm not sure the existing confirmedVariousArtists code works as intended if there are seeded tracks containing Various Artists either:

    this.hasVariousArtistTracks = ko.computed(function () {
      return !self.tracksUnknownToUser() &&
             self.tracks().some(t => t.hasVariousArtists());
    });
    this.confirmedVariousArtists = ko.observable(
      this.hasVariousArtistTracks(),
    );

[derat]

I think you're right about this; I see the same issue with the VA checkbox being pre-checked when I seed a new release with a VA track credit. The checkboxes seem to work as intended both when seeding new releases and editing existing releases with bad data when I initialize both of these to false.

There's still a difference between the two, in that the feat. checkbox is only shown if the user introduced the feat. title while the VA checkbox is shown whenever there's a VA credit, even if it was already there . I think that that probably can't be changed without tracking more state about the original credits, though. I'd prefer to not introduce that in this change if that's okay. :-)

[mwiencek]

Thanks for the changes! LGTM and worked for me locally.