Skip to content

Commit

Permalink
MBS-12550: Don't enter useless annotation edit in merge
Browse files Browse the repository at this point in the history
The entity merge system was trying to merge the annotations
in entities, but if all entities had the same annotation already,
it still tried to enter an edit, triggering the NoChanges exception
added by MBS-12556.

There seems to be no reason why we would even try to enter an edit
in this case, since the annotation won't change. I'd also argue
it's misleading to claim it's the "Result of $type merge" since it's
just the same as before. This just checks the current annotation
in the target entity, and skips entering an edit if the annotation
resulting from the merge process would have the same text.
  • Loading branch information
reosarevok committed Sep 21, 2022
1 parent 61e8eb3 commit 80268c1
Show file tree
Hide file tree
Showing 2 changed files with 81 additions and 2 deletions.
11 changes: 9 additions & 2 deletions lib/MusicBrainz/Server/Data/EntityAnnotation.pm
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ sub merge
my ($self, $new_id, @old_ids) = @_;
my $table = $self->table;
my $type = $self->type;
my $model = type_to_model($type);

my @ids = ($new_id, @old_ids);
my %entity_to_annotation = map { @$_ } @{
Expand All @@ -118,18 +119,24 @@ sub merge
)
};

my $current_target_annotation =
$self->c->model($model)->annotation->get_latest($new_id);
my $current_target_annotation_text = $current_target_annotation
? $current_target_annotation->text
: '';

my $modbot = $self->c->model('Editor')->get_by_id($EDITOR_MODBOT);
if (keys %entity_to_annotation > 1) {
my $new_text = join("\n\n-------\n\n",
uniq
grep { $_ ne '' }
map { $entity_to_annotation{$_} // '' }
@ids);
if ($new_text ne '') {
if ($new_text ne '' && $new_text ne $current_target_annotation_text) {
$self->c->model('Edit')->create(
edit_type => $ENTITIES{$type}{annotations}{edit_type},
editor => $modbot,
entity => $self->c->model(type_to_model($type))->get_by_id($new_id),
entity => $self->c->model($model)->get_by_id($new_id),
text => $new_text,
changelog => "Result of $type merge"
);
Expand Down
72 changes: 72 additions & 0 deletions t/lib/t/MusicBrainz/Server/Edit/Release/Merge.pm
Original file line number Diff line number Diff line change
Expand Up @@ -605,4 +605,76 @@ test 'Merging release with empty medium (MBS-11614)' => sub {
ok($mediums[1]->track_count == 1, 'Second medium has one track');
};

test 'Merge goes through despite duplicate annotations (MBS-12550)' => sub {

my $test = shift;
my $c = $test->c;

MusicBrainz::Server::Test->prepare_test_database($c, '+release');
MusicBrainz::Server::Test->prepare_test_database($c, <<~'SQL');
INSERT INTO annotation (id, editor, text)
VALUES (100, 1, 'Annotation'),
(200, 1, 'Annotation');
INSERT INTO release_annotation (release, annotation)
VALUES (6, 100), (7, 200);
SQL

my $edit = $c->model('Edit')->create(
edit_type => $EDIT_RELEASE_MERGE,
editor_id => 1,
new_entity => {
id => 6,
name => 'Release 1',
},
old_entities => [
{
id => 7,
name => 'Release 2'
}
],
merge_strategy => $MusicBrainz::Server::Data::Release::MERGE_APPEND,
medium_changes => [
{
release => {
id => 6,
name => 'Release 1',
},
mediums => [
{
id => 2,
old_position => 1,
new_position => 1,
old_name => '',
new_name => '',
},
]
},
{
release => {
id => 7,
name => 'Release 2',
},
mediums => [
{
id => 3,
old_position => 1,
new_position => 2,
old_name => '',
new_name => '',
},
]
}
]
);

accept_edit($c, $edit);
is($edit->status, $STATUS_APPLIED, 'The edit was applied');
my $merged_annotation = $c->model('Release')->annotation->get_latest(6);
is(
$merged_annotation->text,
'Annotation',
'The annotation was kept and not duplicated since it was the same'
);
};

1;

0 comments on commit 80268c1

Please sign in to comment.