Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix release merge issues MBS-10188 and MBS-10279 #1148

Merged
merged 16 commits into from Dec 4, 2019
Merged

Fix release merge issues MBS-10188 and MBS-10279 #1148

merged 16 commits into from Dec 4, 2019

Conversation

mwiencek
Copy link
Member

@mwiencek mwiencek commented Jul 15, 2019 •
edited by reosarevok

@reosarevok reosarevok added the Bug Bugs that should be checked/fixed soonish label Sep 27, 2019
@mwiencek mwiencek force-pushed the mbs-10188 branch 2 times, most recently from 378bdd0 to a14818e Compare October 24, 2019 01:30
@yvanzo yvanzo added this to the Grandpa Shark milestone Nov 27, 2019
@mwiencek mwiencek mentioned this pull request Nov 28, 2019
Merged
yvanzo
yvanzo approved these changes Dec 2, 2019
View reviewed changes
Copy link
Contributor

@yvanzo yvanzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Localization of future ModBot edit notes is probably worth an improvement ticket.

@reosarevok
Copy link
Member

For the ModBot bit, I'd suggest also adding a separate ticket to convert previous notes to the new format with a SQL script at some point (a lot should be trivial to improve).

@mwiencek
Fix release-recording merge display (MBS-10279)
434f499 
Prior to this commit, there were three different subroutines for
calculating recording merges for $EDIT_RELEASE_MERGE edits. As you might
expect, these did not share a common implementation.

 1. Data::Release::determine_recording_merges:

    This was used for detecting "bad" recording merges where the artists
    differed (see root/release/merge.tt).

 2. Edit::Utils::calculate_recording_merges:

    This was added for storing `recording_merges` in the edit data, for
    display and historical purposes (see 0e41a26), but still not for
    deciding what would actually get merged. (And after 0e41a26,
    $EDIT_RELEASE_MERGE edits did not display any recording merges that
    would actually happen, only those that were predicted to happen
    when the edit was opened. Thus, the edit might say "All recordings
    for these releases are already merged" while open, yet that may
    have been false.)

 3. Data::Medium::merge:

    This is what implemented the actual merge logic. It received a
    couple fixes over the years in dae5b54 and c1992b2, both of which
    could cause the result of the edit to disagree with the display
    (assuming, at a minimum, the tracklist didn't change in the
    meantime).

These three methods have been unified into
Data::Release::determine_recording_merges, but the implementation is
largely based on the previous Data::Medium::merge method. The reasoning
for its being moved to Data::Release (and Data::Medium::merge being
removed) is that (1) a recording could appear multiple times on the
release, which needs to be factored into the merge calculation, and (2)
it doesn't make sense to merge two arbitrary mediums outside the context
of a release merge.

This commit also fixes the display issue mentioned in point 2 above. We
now display the recording merges that will actually happen when the edit
is applied, not those that were (incorrectly) calculated when the edit
was opened. Additionally, when the edit is accepted, we update the
edit_data table to save the applied recording merge info for historical
display purposes.
@mwiencek
Remove unnecessary can_merge call
416c193 
It's already invoked in Edit::Release::Merge::do_merge. And it confesses
a possibly-incorrect error message here.
@mwiencek
MBS-10500: Allow localizing ModBot edit notes
d2f9ced 
ModBot's notes are currently always stored and displayed in English.
With this commit, a note can be stored in the database in a special
format:

  localize:{"message": "...", "args": {...}}

where "message" is a message in our gettext catalog, and "args" are any
substitution arguments needed.

When loading a note from ModBot, we check if it's in this format before
displaying it, and parse it to form a localized string using those
parameters.

This syntax can only be used in notes left by ModBot (a regular user
could not).
@mwiencek
Put release merge errors in hash
4d79ab9
@mwiencek
Use more specific var name
6c7a7ec
@mwiencek
Change can_merge signature
ea6cf54 
Return (success, reason) instead of adding _cannot_merge_reason to the
hash that was passed in.
@mwiencek
Localize ModBot release merge errors
8d3c6cc
@mwiencek
Add error for ambiguous recording merge target
b9f8702 
While we would always skip recordings that had multiple merge targets,
we did this silently without informing the user (allowing the release
merge to go through). This makes it an explicit error (causing the
release merge to fail) with a user-visible explanation. The user will be
required to fix the tracklists before proceeding with the merge.
@mwiencek
Use _validate_merge instead of _merge_submit
b282f12 
We weren't wrapping _merge_submit here for anything that wasn't
validation, so we can use _validate_merge instead. Returning 0 from
_validate_merge prevents _merge_submit from running altogether.
@mwiencek
Calculate bad_recording_merges in _validate_merge
b7d4e23 
This makes more sense to me, and also means we don't have to call
determine_recording_merges twice. Explanatory comments have been added.
@mwiencek
Reject recording merge cycles (MBS-10188)
b98081c 
We previously addressed this in c1992b2 by silently ignoring such
situations (but allowing the release merge to go through). It turns out
that the fix there only works for some types of merge cycles, but not
all. It also seems unsafe to allow releases with potentially misordered
recordings to be merged at all, so this is now a hard error.

Note: I added ModBot to t/sql/initial.sql, since a lot of tests use
them. ModBot has an editor id of 4, which conflicts with unrelated
tests. I had to fix the editor ids in those tests to not conflict
with ModBot.
reosarevok
reosarevok approved these changes Dec 4, 2019
View reviewed changes
Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good to me now :) I haven't tested it locally though since I guess I would have to come up with a bunch of complicated conditions to reproduce it, but since you added tests, I expect they're good!

@mwiencek mwiencek force-pushed the mbs-10188 branch 2 times, most recently from ba8d5ea to ed369f3 Compare December 4, 2019 08:15
@mwiencek
Copy link
Member Author

mwiencek commented Dec 4, 2019

Tweaked the message, since it might also show if the medium counts are the same, but a medium position is missing from the target.

@mwiencek
Separate merge error for missing target mediums
025e4a7 
The current `medium_track_counts` error is shown in cases where the
target release is missing a medium position present on one of the source
releases, which doesn't make sense. I've created a separate error message
for this case.

Thanks to @reosarevok for the wording.
@mwiencek mwiencek merged commit 83c3900 into metabrainz:master Dec 4, 2019
@mwiencek mwiencek deleted the mbs-10188 branch December 4, 2019 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reosarevok reosarevok reosarevok approved these changes

@yvanzo yvanzo yvanzo approved these changes

Assignees
No one assigned
Labels
Bug Bugs that should be checked/fixed soonish
Projects
None yet
Milestone
Grandpa Shark
3 participants
@mwiencek @reosarevok @yvanzo