From d67571de8ece6f586decff1db85ec1ed88fc2853 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Tue, 5 Nov 2024 21:58:12 +0100 Subject: [PATCH] MBS-9885: Deal with character vs ended while merging The guidelines say that characters should never have an end date, since a character can always be reused forever once created. As such, this restricts end dates and areas for characters when merging in the same way as we already do for genders on groups. --- lib/MusicBrainz/Server/Data/Artist.pm | 142 ++++++++++--- lib/MusicBrainz/Server/Edit/Artist/Merge.pm | 122 +++++++++-- .../t/MusicBrainz/Server/Edit/Artist/Merge.pm | 192 ++++++++++++++++++ t/sql/edit_artist_merge.sql | 3 + 4 files changed, 416 insertions(+), 43 deletions(-) diff --git a/lib/MusicBrainz/Server/Data/Artist.pm b/lib/MusicBrainz/Server/Data/Artist.pm index 0b632236b3f..4528801ef2b 100644 --- a/lib/MusicBrainz/Server/Data/Artist.pm +++ b/lib/MusicBrainz/Server/Data/Artist.pm @@ -4,7 +4,10 @@ use namespace::autoclean; use Carp; use List::AllUtils qw( any ); -use MusicBrainz::Server::Constants qw( $ARTIST_TYPE_GROUP ); +use MusicBrainz::Server::Constants qw( + $ARTIST_TYPE_CHARACTER + $ARTIST_TYPE_GROUP +); use MusicBrainz::Server::Entity::Artist; use MusicBrainz::Server::Entity::PartialDate; use MusicBrainz::Server::Data::ArtistCredit; @@ -19,6 +22,7 @@ use MusicBrainz::Server::Data::Utils qw( load_subobjects merge_table_attributes merge_date_period + merge_partial_date order_by ); use MusicBrainz::Server::Data::Utils::Cleanup qw( used_in_relationship ); @@ -389,12 +393,21 @@ sub merge # that's arguably more annoying for the editor. See MBS-10187. my %dropped_columns; unless (is_special_artist($new_id)) { - my $merge_columns = [ qw( area begin_area end_area ) ]; - my $target_row = $self->sql->select_single_row_hash('SELECT gender, type FROM artist WHERE id = ?', $new_id); + my $merge_columns = [ qw( area begin_area ) ]; + my $target_row = $self->sql->select_single_row_hash( + 'SELECT gender, type, ended, end_area, end_date_year, end_date_month, end_date_day FROM artist WHERE id = ?', + $new_id, + ); my $target_type = $target_row->{type}; my $target_gender = $target_row->{gender}; + my $target_ended = $target_row->{ended}; + my $target_end_date = MusicBrainz::Server::Entity::PartialDate->new_from_row($target_row, 'end_date_'); + my $target_end_area = $target_row->{end_area}; my $merged_type = $target_type; my $merged_gender = $target_gender; + my $merged_ended = $target_ended; + my $merged_end_date = $target_end_date; + my $merged_end_area = $target_end_area; if (!$merged_type) { my ($query, $args) = conditional_merge_column_query( @@ -408,6 +421,38 @@ sub merge $merged_gender = $self->c->sql->select_single_value($query, @$args); } + if (!$merged_ended) { + my ($query, $args) = conditional_merge_column_query( + 'artist', 'ended', $new_id, [$new_id, @$old_ids], '= TRUE'); + $merged_ended = $self->c->sql->select_single_value($query, @$args) // 0; + } + + if ($merged_end_date->is_empty) { + my ($query, $args) = conditional_merge_column_query( + 'artist', 'end_date_year', $new_id, [$new_id, @$old_ids], 'IS NOT NULL'); + my $merged_end_year = $self->c->sql->select_single_value($query, @$args); + + ($query, $args) = conditional_merge_column_query( + 'artist', 'end_date_month', $new_id, [$new_id, @$old_ids], 'IS NOT NULL'); + my $merged_end_month = $self->c->sql->select_single_value($query, @$args); + + ($query, $args) = conditional_merge_column_query( + 'artist', 'end_date_day', $new_id, [$new_id, @$old_ids], 'IS NOT NULL'); + my $merged_end_day = $self->c->sql->select_single_value($query, @$args); + + $merged_end_date = MusicBrainz::Server::Entity::PartialDate->new( + year => $merged_end_year, + month => $merged_end_month, + day => $merged_end_day, + ); + } + + if (!$merged_end_area) { + my ($query, $args) = conditional_merge_column_query( + 'artist', 'end_area', $new_id, [$new_id, @$old_ids], 'IS NOT NULL'); + $merged_end_area = $self->c->sql->select_single_value($query, @$args); + } + my $group_types = $self->sql->select_single_column_array(q{ WITH RECURSIVE atp(id) AS ( VALUES (?::int) @@ -422,23 +467,61 @@ sub merge defined $merged_type && contains_string($group_types, $merged_type); - if ($merged_type_is_group && $merged_gender) { - my $target_type_is_group = - defined $target_type && - contains_string($group_types, $target_type); - - if ($target_type_is_group) { - $dropped_columns{gender} = $merged_gender; - push @$merge_columns, 'type'; - } elsif ($target_gender) { - $dropped_columns{type} = $merged_type; - push @$merge_columns, 'gender'; + my $merged_type_is_character = + defined $merged_type && $merged_type == $ARTIST_TYPE_CHARACTER; + + my $merge_ended = 0; + + if ($merged_type_is_group || $merged_type_is_character) { + if ($merged_type_is_group && $merged_gender) { + my $target_type_is_group = + defined $target_type && + contains_string($group_types, $target_type); + + if ($target_type_is_group) { + $dropped_columns{gender} = $merged_gender; + push @$merge_columns, 'type'; + } elsif ($target_gender) { + $dropped_columns{type} = $merged_type; + push @$merge_columns, 'gender'; + } else { + $dropped_columns{gender} = $merged_gender; + $dropped_columns{type} = $merged_type; + } } else { - $dropped_columns{gender} = $merged_gender; - $dropped_columns{type} = $merged_type; + push @$merge_columns, 'gender'; + } + + if ( + $merged_type_is_character && ( + $merged_ended || + $merged_end_area || + !($merged_end_date->is_empty) + ) + ) { + my $target_type_is_character = + defined $target_type && $target_type == $ARTIST_TYPE_CHARACTER; + + if ($target_type_is_character) { + $dropped_columns{ended} = $merged_ended; + $dropped_columns{end_area} = $merged_end_area; + $dropped_columns{end_date} = $merged_end_date->format; + push @$merge_columns, 'type'; + } elsif ($target_ended || $target_end_area || !($target_end_date->is_empty)) { + $dropped_columns{type} = $merged_type; + push @$merge_columns, 'end_area'; + $merge_ended = 1; + } else { + $dropped_columns{ended} = $merged_ended; + $dropped_columns{type} = $merged_type; + } + } else { + push @$merge_columns, 'end_area'; + $merge_ended = 1; } } else { - push @$merge_columns, qw( gender type ); + push @$merge_columns, qw( type gender end_area ); + $merge_ended = 1; } merge_table_attributes( @@ -450,13 +533,24 @@ sub merge ), ); - merge_date_period( - $self->sql => ( - table => 'artist', - old_ids => $old_ids, - new_id => $new_id, - ), - ); + if ($merge_ended) { + merge_date_period( + $self->sql => ( + table => 'artist', + old_ids => $old_ids, + new_id => $new_id, + ), + ); + } else { + merge_partial_date( + $self->sql => ( + table => 'artist', + old_ids => $old_ids, + new_id => $new_id, + ), + field => 'begin_date', + ); + } } $self->_delete_and_redirect_gids('artist', $new_id, @$old_ids); diff --git a/lib/MusicBrainz/Server/Edit/Artist/Merge.pm b/lib/MusicBrainz/Server/Edit/Artist/Merge.pm index 4ba2615a6f6..ddd6c2b71b9 100644 --- a/lib/MusicBrainz/Server/Edit/Artist/Merge.pm +++ b/lib/MusicBrainz/Server/Edit/Artist/Merge.pm @@ -6,7 +6,7 @@ use MooseX::Types::Moose qw( ArrayRef Bool Int Str ); use MooseX::Types::Structured qw( Dict ); use MusicBrainz::Server::Constants qw( $EDIT_ARTIST_MERGE $EDITOR_MODBOT ); use MusicBrainz::Server::Data::Utils qw( boolean_to_json localized_note ); -use MusicBrainz::Server::Translation qw( N_l N_lp ); +use MusicBrainz::Server::Translation qw( comma_list N_l N_lp ); use Hash::Merge qw( merge ); extends 'MusicBrainz::Server::Edit::Generic::Merge'; @@ -65,20 +65,75 @@ sub do_merge my $dropped_type = $self->c->model('ArtistType')->get_by_id($dropped_columns->{type}); + if ($dropped_type->id == 4) { + $self->c->model('EditNote')->add_note( + $self->id => { + editor_id => $EDITOR_MODBOT, + text => localized_note( + N_l('The following data has not been added to the destination ' . + 'artist because it conflicted with other data: ' . + '{dropped_data}. {reason}'), + vars => { + dropped_data => localized_note( + N_l('Type: {artist_type}'), + vars => { + artist_type => localized_note( + $dropped_type->name, + function => 'lp', + domain => 'attributes', + args => ['artist_type'], + ), + }, + ), + reason => localized_note( + N_l('Characters cannot have an end date nor area ' . + 'nor be marked as ended.') + ), + }, + ), + }, + ); + } else { + $self->c->model('EditNote')->add_note( + $self->id => { + editor_id => $EDITOR_MODBOT, + text => localized_note( + N_l('The “{artist_type}” type has not been added to the ' . + 'destination artist because it conflicted with the ' . + 'gender setting of one of the artists here. Group ' . + 'artists cannot have a gender.'), + vars => { + artist_type => localized_note( + $dropped_type->name, + function => 'lp', + domain => 'attributes', + args => ['artist_type'], + ), + }, + ), + }, + ); + } + } + + if ($dropped_columns->{gender}) { + my $dropped_gender = + $self->c->model('Gender')->get_by_id($dropped_columns->{gender}); + $self->c->model('EditNote')->add_note( $self->id => { editor_id => $EDITOR_MODBOT, text => localized_note( - N_l('The “{artist_type}” type has not been added to the ' . + N_l('The “{gender}” gender has not been added to the ' . 'destination artist because it conflicted with the ' . - 'gender setting of one of the artists here. Group ' . - 'artists cannot have a gender.'), + 'group type of one of the artists here. Group artists ' . + 'cannot have a gender.'), vars => { - artist_type => localized_note( - $dropped_type->name, + gender => localized_note( + $dropped_gender->name, function => 'lp', domain => 'attributes', - args => ['artist_type'], + args => ['gender'], ), }, ), @@ -86,24 +141,53 @@ sub do_merge ); } - if ($dropped_columns->{gender}) { - my $dropped_gender = - $self->c->model('Gender')->get_by_id($dropped_columns->{gender}); + my $dropped_ended = defined $dropped_columns->{ended}; + + if ($dropped_ended || + $dropped_columns->{end_date} || + $dropped_columns->{end_area} + ) { + my $dropped_area = $dropped_columns->{end_area} + ? $self->c->model('Area')->get_by_id($dropped_columns->{end_area}) + : undef; + + my $dropped_data = []; + + if ($dropped_ended) { + push @$dropped_data, localized_note( N_l('Ended: Yes') ); + } + + if ($dropped_columns->{end_date}) { + push @$dropped_data, localized_note( + N_l('End date: {end_date}'), + vars => { end_date => $dropped_columns->{end_date} }, + ); + } + + if ($dropped_area) { + push @$dropped_data, localized_note( + N_l('End area: {end_area}'), + vars => { end_area => $dropped_area->name }, + ); + } $self->c->model('EditNote')->add_note( $self->id => { editor_id => $EDITOR_MODBOT, text => localized_note( - N_l('The “{gender}” gender has not been added to the ' . - 'destination artist because it conflicted with the ' . - 'group type of one of the artists here. Group artists ' . - 'cannot have a gender.'), + N_l('The following data has not been added to the destination ' . + 'artist because it conflicted with other data: ' . + '{dropped_data}. {reason}'), vars => { - gender => localized_note( - $dropped_gender->name, - function => 'lp', - domain => 'attributes', - args => ['gender'], + dropped_data => localized_note( + undef, + function => 'comma_list', + domain => 'mb_server', + args => $dropped_data, + ), + reason => localized_note( + N_l('Characters cannot have an end date nor area ' . + 'nor be marked as ended.') ), }, ), diff --git a/t/lib/t/MusicBrainz/Server/Edit/Artist/Merge.pm b/t/lib/t/MusicBrainz/Server/Edit/Artist/Merge.pm index fddaa727c14..62045a246f7 100644 --- a/t/lib/t/MusicBrainz/Server/Edit/Artist/Merge.pm +++ b/t/lib/t/MusicBrainz/Server/Edit/Artist/Merge.pm @@ -207,6 +207,198 @@ test 'Merging a group, and an artist with no type and a gender, into an artist w ); }; +test 'Merging an artist with no type and an end date into a character' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = 4, end_date_year = NULL WHERE id = 4; + UPDATE artist SET type = NULL, end_date_year = 1999 WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging an artist with no type and an end date into a character does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, 4, 'The resulting type is a character'); + is($row->{end_date_year}, undef, 'The resulting end date year is null'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist ' . + 'because it conflicted with other data: Ended: Yes and End date: 1999. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + +test 'Merging an artist with no type and an end area into a character' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = 4, end_area = NULL WHERE id = 4; + UPDATE artist SET type = NULL, end_area = 221 WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging an artist with no type and an end date into a character does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, 4, 'The resulting type is a character'); + is($row->{end_area}, undef, 'The resulting end area is null'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist because ' . + 'it conflicted with other data: Ended: Yes and End area: United Kingdom. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + +test 'Merging an artist with no type and marked as ended into a character' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = 4, ended = FALSE WHERE id = 4; + UPDATE artist SET type = NULL, ended = TRUE WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging an artist with no type and an end date into a character does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, 4, 'The resulting type is a character'); + is($row->{ended}, 0, 'The resulting ended value is false'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist ' . + 'because it conflicted with other data: Ended: Yes. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + +test 'Merging a character into an artist with no type and an end date' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = NULL, end_date_year = 1999 WHERE id = 4; + UPDATE artist SET type = 4, end_date_year = NULL WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging a character into an artist with no type and an end date does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, undef, 'The resulting type is null'); + is($row->{end_date_year}, 1999, 'The resulting end date year is 1999'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist ' . + 'because it conflicted with other data: Type: Character. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + +test 'Merging a character into an artist with no type and an end area' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = NULL, end_area = 221 WHERE id = 4; + UPDATE artist SET type = 4, end_area = NULL WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging a character into an artist with no type and an end date does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, undef, 'The resulting type is null'); + is($row->{end_area}, 221, 'The resulting end area is the United Kingdom'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist ' . + 'because it conflicted with other data: Type: Character. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + +test 'Merging a character into an artist with no type marked as ended' => sub { + my $test = shift; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($c, '+edit_artist_merge'); + $c->sql->do(<<~'SQL'); + UPDATE artist SET type = NULL, ended = TRUE WHERE id = 4; + UPDATE artist SET type = 4, ended = FALSE WHERE id = 3; + SQL + + # merge 3 -> 4 + my $edit = create_edit($c); + + ok(!exception { accept_edit($c, $edit) }, + 'Edit merging a character into an artist with no type and an end date does not cause an exception'); + + my $row = $c->sql->select_single_row_hash('SELECT * FROM artist WHERE id = 4'); + is($row->{type}, undef, 'The resulting type is null'); + is($row->{ended}, 1, 'The resulting ended value is true'); + + $c->model('EditNote')->load_for_edits($edit); + is(scalar $edit->all_edit_notes, 1); + + my $note = scalar($edit->all_edit_notes) ? $edit->edit_notes->[0] : undef; + is( + defined $note && $note->localize, + 'The following data has not been added to the destination artist ' . + 'because it conflicted with other data: Type: Character. ' . + 'Characters cannot have an end date nor area nor be marked as ended.', + ); +}; + # The name of this test may be confusing, since the code should do the opposite # of what is understood to happen in the UI. By "renaming" the credits, the code # should do nothing and leave them empty, so that they take on the new artist diff --git a/t/sql/edit_artist_merge.sql b/t/sql/edit_artist_merge.sql index eb323243d27..8ed6e6b1b61 100644 --- a/t/sql/edit_artist_merge.sql +++ b/t/sql/edit_artist_merge.sql @@ -25,3 +25,6 @@ INSERT INTO recording (id, gid, name, artist_credit, length) INSERT INTO link (id, link_type, attribute_count) VALUES (1, 156, 0); INSERT INTO l_artist_recording (id, link, entity0, entity1) VALUES (1, 1, 3, 1); + +INSERT INTO area (id, gid, name, type) VALUES + (221, '8a754a16-0027-3a29-b6d7-2b40ea0481ed', 'United Kingdom', 1);