From cd355066774f4a375adf3df94ba88db1886c610e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Thu, 31 Oct 2024 14:38:04 +0100 Subject: [PATCH 1/5] MBS-13802: Always use "Created (in)" for characters We changed this for sidebar display for Created a while ago, but I missed the artist editor, and area should use Created in for consistency. --- root/artist/utils.js | 2 ++ root/static/scripts/edit/MB/Control/ArtistEdit.js | 14 ++++++++++++++ 2 files changed, 16 insertions(+) diff --git a/root/artist/utils.js b/root/artist/utils.js index a6ece52bace..abb987c1113 100644 --- a/root/artist/utils.js +++ b/root/artist/utils.js @@ -15,6 +15,8 @@ export function artistBeginAreaLabel(typeId: ?number): string { case 5: case 6: return addColonText(lp('Founded in', 'group artist')); + case 4: + return addColonText(lp('Created in', 'character artist')); default: return addColonText(l('Begin area')); } diff --git a/root/static/scripts/edit/MB/Control/ArtistEdit.js b/root/static/scripts/edit/MB/Control/ArtistEdit.js index 288097094bf..df98b8b4111 100644 --- a/root/static/scripts/edit/MB/Control/ArtistEdit.js +++ b/root/static/scripts/edit/MB/Control/ArtistEdit.js @@ -40,6 +40,7 @@ MB.Control.ArtistEdit = function () { * Unknown: 0 * Person: 1 * Group: 2 + * Character: 4 * Orchestra: 5 * Choir: 6 */ @@ -70,6 +71,19 @@ MB.Control.ArtistEdit = function () { self.disableGender(); break; + case '4': + self.changeDateText( + addColonText(lp('Created', 'character artist')), + addColonText(lp('Ended', 'artist end date')), + l('This artist has ended.'), + ); + self.changeAreaText( + addColonText(lp('Created in', 'character artist')), + addColonText(l('End area')), + ); + self.enableGender(); + break; + case '0': default: self.changeDateText( From b73af6d320caa9fea0a8a14376f8209676f2ee56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Thu, 31 Oct 2024 16:55:15 +0100 Subject: [PATCH 2/5] MBS-9885: Prevent setting end date for character artist 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 in the same way as we already do for genders on groups. On selecting character, this just blanks and hides the end date/area fields (including ended) rather than disabling them, since disabled fields would not actually remove dates if they were already present. Merge code and a db-level constraint will be added in subsequent commits. --- lib/MusicBrainz/Server/Constants.pm | 3 +- lib/MusicBrainz/Server/Edit/Artist/Edit.pm | 25 ++++ lib/MusicBrainz/Server/Form/Artist.pm | 15 +++ root/artist/edit_form.tt | 30 ++--- .../scripts/edit/MB/Control/ArtistEdit.js | 45 +++++++ .../t/MusicBrainz/Server/Edit/Artist/Edit.pm | 110 ++++++++++++++++++ t/tests.t | 2 +- 7 files changed, 214 insertions(+), 16 deletions(-) diff --git a/lib/MusicBrainz/Server/Constants.pm b/lib/MusicBrainz/Server/Constants.pm index 1c71142e40d..b92b1d22b4d 100644 --- a/lib/MusicBrainz/Server/Constants.pm +++ b/lib/MusicBrainz/Server/Constants.pm @@ -67,7 +67,7 @@ our @EXPORT_OK = ( $DLABEL_ID $DARTIST_ID $VARTIST_ID $VARTIST_GID $NOLABEL_ID $NOLABEL_GID $COVERART_FRONT_TYPE $COVERART_BACK_TYPE $AREA_TYPE_COUNTRY $AREA_TYPE_CITY - $ARTIST_TYPE_PERSON $ARTIST_TYPE_GROUP + $ARTIST_TYPE_PERSON $ARTIST_TYPE_GROUP $ARTIST_TYPE_CHARACTER $INSTRUMENT_ROOT_ID $VOCAL_ROOT_ID $REQUIRED_VOTES $OPEN_EDIT_DURATION $MINIMUM_RESPONSE_PERIOD $MINIMUM_VOTING_PERIOD @@ -395,6 +395,7 @@ Readonly our $AREA_TYPE_CITY => 3; Readonly our $ARTIST_TYPE_PERSON => 1; Readonly our $ARTIST_TYPE_GROUP => 2; +Readonly our $ARTIST_TYPE_CHARACTER => 4; Readonly our $REQUIRED_VOTES => 3; Readonly our $OPEN_EDIT_DURATION => DateTime::Duration->new(days => 7); diff --git a/lib/MusicBrainz/Server/Edit/Artist/Edit.pm b/lib/MusicBrainz/Server/Edit/Artist/Edit.pm index 7d2d6ffb6de..06d3a6afe50 100644 --- a/lib/MusicBrainz/Server/Edit/Artist/Edit.pm +++ b/lib/MusicBrainz/Server/Edit/Artist/Edit.pm @@ -3,6 +3,7 @@ use 5.10.0; use Moose; use MusicBrainz::Server::Constants qw( + $ARTIST_TYPE_CHARACTER $ARTIST_TYPE_GROUP $EDIT_ARTIST_CREATE $EDIT_ARTIST_EDIT @@ -272,6 +273,12 @@ around merge_changes => sub { my $artist = $self->current_instance; my $gender_id = exists $merged->{gender_id} ? $merged->{gender_id} : $artist->gender_id; + my $end_date = exists $merged->{end_date} ? + PartialDate->new($merged->{end_date}) : $artist->end_date; + my $ended = (exists $merged->{ended} ? + $merged->{ended} : $artist->ended) // 0; + my $end_area_id = exists $merged->{end_area_id} ? + $merged->{end_area_id} : $artist->end_area_id; my $type_id = exists $merged->{type_id} ? $merged->{type_id} : $artist->type_id; @@ -284,6 +291,24 @@ around merge_changes => sub { ); } + if (defined $end_date && !$end_date->is_empty && defined $type_id) { + MusicBrainz::Server::Edit::Exceptions::GeneralError->throw( + 'Characters cannot have an end date.', + ) if ($type_id == $ARTIST_TYPE_CHARACTER); + } + + if (defined $end_area_id && defined $type_id) { + MusicBrainz::Server::Edit::Exceptions::GeneralError->throw( + 'Characters cannot have an end area.', + ) if ($type_id == $ARTIST_TYPE_CHARACTER); + } + + if ($ended && defined $type_id) { + MusicBrainz::Server::Edit::Exceptions::GeneralError->throw( + 'Characters cannot be marked as ended.', + ) if ($type_id == $ARTIST_TYPE_CHARACTER); + } + return $merged; }; diff --git a/lib/MusicBrainz/Server/Form/Artist.pm b/lib/MusicBrainz/Server/Form/Artist.pm index e552ee9e6f1..f0b05ba6c35 100644 --- a/lib/MusicBrainz/Server/Form/Artist.pm +++ b/lib/MusicBrainz/Server/Form/Artist.pm @@ -89,6 +89,21 @@ sub validate { $self->field('gender_id')->add_error(l('Choirs cannot have a gender.')); } } + + if ($self->field('type_id')->value && + $self->field('type_id')->value == 4) { + if ($self->field('period.end_date.year')->value || + $self->field('period.end_date.month')->value || + $self->field('period.end_date.day')->value) { + $self->field('period.end_date')->add_error(l('Characters cannot have an end date.')); + } + if ($self->field('period.ended')->value) { + $self->field('period.ended')->add_error(l('Characters cannot be marked as ended.')); + } + if ($self->field('end_area_id')->value) { + $self->field('end_area.name')->add_error(l('Characters cannot have an end area.')); + } + } } 1; diff --git a/root/artist/edit_form.tt b/root/artist/edit_form.tt index dabe29f0c35..9f3d0f126b7 100644 --- a/root/artist/edit_form.tt +++ b/root/artist/edit_form.tt @@ -60,20 +60,22 @@ [% field_errors(r.form, 'begin_area.name') %] [% END %] - [% form_row_date(r, 'period.end_date', add_colon(l('End date'))) %] - [% too_short_year_error('too_short_end_year') %] - [% form_row_checkbox(r, 'period.ended', l('This artist has ended.')) %] - [% WRAPPER form_row %] - [% end_area_field = form.field('end_area.name') %] - - - [% React.embed(c, 'static/scripts/common/components/SearchIcon') %] - [% r.hidden(form.field('end_area').field('gid'), { class => 'gid' }) %] - [% r.hidden('end_area_id', class => 'id') %] - [% r.text(end_area_field, class => 'name') %] - - [% field_errors(r.form, 'end_area.name') %] - [% END %] +
+ [% form_row_date(r, 'period.end_date', add_colon(l('End date'))) %] + [% too_short_year_error('too_short_end_year') %] + [% form_row_checkbox(r, 'period.ended', l('This artist has ended.')) %] + [% WRAPPER form_row %] + [% end_area_field = form.field('end_area.name') %] + + + [% React.embed(c, 'static/scripts/common/components/SearchIcon') %] + [% r.hidden(form.field('end_area').field('gid'), { class => 'gid' }) %] + [% r.hidden('end_area_id', class => 'id') %] + [% r.text(end_area_field, class => 'name') %] + + [% field_errors(r.form, 'end_area.name') %] + [% END %] +
[% PROCESS 'forms/relationship-editor.tt' %] diff --git a/root/static/scripts/edit/MB/Control/ArtistEdit.js b/root/static/scripts/edit/MB/Control/ArtistEdit.js index df98b8b4111..bd3fe95eafa 100644 --- a/root/static/scripts/edit/MB/Control/ArtistEdit.js +++ b/root/static/scripts/edit/MB/Control/ArtistEdit.js @@ -16,12 +16,23 @@ MB.Control.ArtistEdit = function () { self.$name = $('#id-edit-artist\\.name'); self.$begin = $('#label-id-edit-artist\\.period\\.begin_date'); self.$ended = $('#label-id-edit-artist\\.period\\.ended'); + self.$ended_checkbox = $('#id-edit-artist\\.period\\.ended'); self.$end = $('#label-id-edit-artist\\.period\\.end_date'); + self.$end_section = $('#artist\\.end_date_section'); + self.$end_year = $('#id-edit-artist\\.period\\.end_date\\.year'); + self.$end_month = $('#id-edit-artist\\.period\\.end_date\\.month'); + self.$end_day = $('#id-edit-artist\\.period\\.end_date\\.day'); self.$beginarea = $('#label-id-edit-artist\\.begin_area\\.name'); self.$endarea = $('#label-id-edit-artist\\.end_area\\.name'); self.$type = $('#id-edit-artist\\.type_id'); self.$gender = $('#id-edit-artist\\.gender_id'); + self.$end_area_id = $('#id-edit-artist\\.end_area_id'); self.old_gender = self.$gender.val(); + self.old_ended_checkbox = self.$ended_checkbox.prop('checked'); + self.old_end_year = self.$end_year.val(); + self.old_end_month = self.$end_month.val(); + self.old_end_day = self.$end_day.val(); + self.old_end_area = self.$end_area_id.val(); self.changeDateText = function (begin, end, ended) { self.$begin.text(begin); @@ -54,6 +65,7 @@ MB.Control.ArtistEdit = function () { ); self.changeAreaText(l('Born in:'), l('Died in:')); self.enableGender(); + self.enableEndSection(); break; case '2': @@ -69,6 +81,7 @@ MB.Control.ArtistEdit = function () { addColonText(lp('Dissolved in', 'group artist')), ); self.disableGender(); + self.enableEndSection(); break; case '4': @@ -82,6 +95,7 @@ MB.Control.ArtistEdit = function () { addColonText(l('End area')), ); self.enableGender(); + self.disableEndSection(); break; case '0': @@ -96,6 +110,7 @@ MB.Control.ArtistEdit = function () { addColonText(l('End area')), ); self.enableGender(); + self.enableEndSection(); break; } }; @@ -114,6 +129,36 @@ MB.Control.ArtistEdit = function () { self.$gender.val(''); }; + self.enableEndSection = function () { + if (self.$end_section.is(':hidden')) { + self.$end_section.show(); + self.$ended_checkbox.prop('checked', self.old_ended_checkbox); + self.$end_area_id.val(self.old_end_area); + self.$end_year.val(self.old_end_year); + self.$end_month.val(self.old_end_month); + self.$end_day.val(self.old_end_day); + } + }; + + self.disableEndSection = function () { + self.old_ended_checkbox = self.$ended_checkbox.prop('checked'); + self.$ended_checkbox.prop('checked', false); + + self.old_end_area = self.$end_area_id.val(); + self.$end_area_id.val(''); + + self.old_end_year = self.$end_year.val(); + self.$end_year.val(''); + + self.old_end_month = self.$end_month.val(); + self.$end_month.val(''); + + self.old_end_day = self.$end_day.val(); + self.$end_day.val(''); + + self.$end_section.hide(); + }; + self.typeChanged(); self.$type.bind('change.mb', self.typeChanged); diff --git a/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm b/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm index cefe7ce6974..95b526ece89 100644 --- a/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm +++ b/t/lib/t/MusicBrainz/Server/Edit/Artist/Edit.pm @@ -424,6 +424,116 @@ test 'Type can be set to group when gender is removed (MBS-8801)' => sub { is($c->model('Artist')->get_by_id(2)->type_id, 2); }; +test 'Fails edits trying to set an end date to a character artist' => sub { + my $test = shift; + my $c = $test->c; + + $c->sql->do(<<~'SQL'); + INSERT INTO artist (id, gid, name, sort_name) + VALUES (2, 'cdf5588d-cca8-4e0c-bae1-d53bc73b012a', 'person', 'person'); + SQL + + my $edit = $c->model('Edit')->create( + edit_type => $EDIT_ARTIST_EDIT, + editor_id => 1, + to_edit => $c->model('Artist')->get_by_id(2), + end_date => { year => 2000, month => 3, day => 20 }, + ipi_codes => [], + isni_codes => [], + privileges => $UNTRUSTED_FLAG, + ); + + ok($edit->is_open); + $c->sql->do('UPDATE artist SET type = 4 WHERE id = 2'); + + my $exception = exception { $edit->accept }; + isa_ok $exception, 'MusicBrainz::Server::Edit::Exceptions::GeneralError'; + is $exception->message, 'Characters cannot have an end date.'; +}; + +test 'Fails edits trying to set an artist with an end date as a character' => sub { + my $test = shift; + my $c = $test->c; + + $c->sql->do(<<~'SQL'); + INSERT INTO artist (id, gid, name, sort_name) + VALUES (2, 'cdf5588d-cca8-4e0c-bae1-d53bc73b012a', 'person', 'person'); + SQL + + my $edit = $c->model('Edit')->create( + edit_type => $EDIT_ARTIST_EDIT, + editor_id => 1, + to_edit => $c->model('Artist')->get_by_id(2), + type_id => 4, + ipi_codes => [], + isni_codes => [], + privileges => $UNTRUSTED_FLAG, + ); + + ok($edit->is_open); + $c->sql->do('UPDATE artist SET end_date_year = 1991 WHERE id = 2'); + + my $exception = exception { $edit->accept }; + isa_ok $exception, 'MusicBrainz::Server::Edit::Exceptions::GeneralError'; + is $exception->message, 'Characters cannot have an end date.'; +}; + +test 'Fails edits trying to set a character artist as ended' => sub { + my $test = shift; + my $c = $test->c; + + $c->sql->do(<<~'SQL'); + INSERT INTO artist (id, gid, name, sort_name) + VALUES (2, 'cdf5588d-cca8-4e0c-bae1-d53bc73b012a', 'person', 'person'); + SQL + + my $edit = $c->model('Edit')->create( + edit_type => $EDIT_ARTIST_EDIT, + editor_id => 1, + to_edit => $c->model('Artist')->get_by_id(2), + ended => 1, + ipi_codes => [], + isni_codes => [], + privileges => $UNTRUSTED_FLAG, + ); + + ok($edit->is_open); + $c->sql->do('UPDATE artist SET type = 4 WHERE id = 2'); + + my $exception = exception { $edit->accept }; + isa_ok $exception, 'MusicBrainz::Server::Edit::Exceptions::GeneralError'; + is $exception->message, 'Characters cannot be marked as ended.'; +}; + +test 'Fails edits trying to set an end area to a character artist' => sub { + my $test = shift; + my $c = $test->c; + + $c->sql->do(<<~'SQL'); + INSERT INTO artist (id, gid, name, sort_name) + VALUES (2, 'cdf5588d-cca8-4e0c-bae1-d53bc73b012a', 'person', 'person'); + INSERT INTO area (id, gid, name, type) + VALUES (221, '8a754a16-0027-3a29-b6d7-2b40ea0481ed', 'United Kingdom', 1); + SQL + + my $edit = $c->model('Edit')->create( + edit_type => $EDIT_ARTIST_EDIT, + editor_id => 1, + to_edit => $c->model('Artist')->get_by_id(2), + end_area_id => 221, + ipi_codes => [], + isni_codes => [], + privileges => $UNTRUSTED_FLAG, + ); + + ok($edit->is_open); + $c->sql->do('UPDATE artist SET type = 4 WHERE id = 2'); + + my $exception = exception { $edit->accept }; + isa_ok $exception, 'MusicBrainz::Server::Edit::Exceptions::GeneralError'; + is $exception->message, 'Characters cannot have an end area.'; +}; + sub _create_full_edit { my ($c, $artist) = @_; return $c->model('Edit')->create( diff --git a/t/tests.t b/t/tests.t index 94b28f8039d..431a958ccb6 100644 --- a/t/tests.t +++ b/t/tests.t @@ -26,7 +26,7 @@ MusicBrainz::Server::Test->prepare_test_server; @classes = commandline_override('t::MusicBrainz::Server::', @classes); # Image editing temporarily disabled -@classes = grep { $_ !~ /Art/ } @classes; +# @classes = grep { $_ !~ /Art/ } @classes; plan tests => scalar(@classes); run_tests($_ => $_) for @classes; From 7c53816c0a73df476b9465d13f052b68a7e27812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Mon, 4 Nov 2024 16:12:53 +0100 Subject: [PATCH 3/5] MBS-9885: Add character_type_implies_no_end DB constraint Characters should never have an end date or area; this just enforces it at the database level like we do with group genders. --- admin/sql/CreateConstraints.sql | 11 +++++++ admin/sql/updates/20241104-mbs-9885.sql | 27 +++++++++++++++ .../30.master_and_standalone.sql | 33 +++++++++++++++++++ upgrade.json | 5 +++ 4 files changed, 76 insertions(+) create mode 100644 admin/sql/updates/20241104-mbs-9885.sql create mode 100644 admin/sql/updates/schema-change/30.master_and_standalone.sql diff --git a/admin/sql/CreateConstraints.sql b/admin/sql/CreateConstraints.sql index 5df42837219..5720281fd90 100644 --- a/admin/sql/CreateConstraints.sql +++ b/admin/sql/CreateConstraints.sql @@ -170,6 +170,17 @@ ADD CONSTRAINT group_type_implies_null_gender CHECK ( OR type IS NULL ); +ALTER TABLE artist +ADD CONSTRAINT character_type_implies_no_end CHECK ( + type != 4 OR ( + end_date_day IS NULL AND + end_date_month IS NULL AND + end_date_year IS NULL AND + end_area IS NULL AND + ended = FALSE + ) +); + ALTER TABLE release_label ADD CHECK (catalog_number IS NOT NULL OR label IS NOT NULL); diff --git a/admin/sql/updates/20241104-mbs-9885.sql b/admin/sql/updates/20241104-mbs-9885.sql new file mode 100644 index 00000000000..4edded422ea --- /dev/null +++ b/admin/sql/updates/20241104-mbs-9885.sql @@ -0,0 +1,27 @@ +\set ON_ERROR_STOP 1 + +BEGIN; + +UPDATE artist +SET end_date_day = NULL, + end_date_month = NULL, + end_date_year = NULL, + end_area = NULL, + ended = FALSE +WHERE type = 4; + +ALTER TABLE artist +DROP CONSTRAINT IF EXISTS character_type_implies_no_end; + +ALTER TABLE artist +ADD CONSTRAINT character_type_implies_no_end CHECK ( + type != 4 OR ( + end_date_day IS NULL AND + end_date_month IS NULL AND + end_date_year IS NULL AND + end_area IS NULL AND + ended = FALSE + ) +); + +COMMIT; diff --git a/admin/sql/updates/schema-change/30.master_and_standalone.sql b/admin/sql/updates/schema-change/30.master_and_standalone.sql new file mode 100644 index 00000000000..4214fd3d6c5 --- /dev/null +++ b/admin/sql/updates/schema-change/30.master_and_standalone.sql @@ -0,0 +1,33 @@ +-- Generated by CompileSchemaScripts.pl from: +-- 20241104-mbs-9885.sql +\set ON_ERROR_STOP 1 +BEGIN; +SET search_path = musicbrainz, public; +SET LOCAL statement_timeout = 0; +-------------------------------------------------------------------------------- +SELECT '20241104-mbs-9885.sql'; + + +UPDATE artist +SET end_date_day = NULL, + end_date_month = NULL, + end_date_year = NULL, + end_area = NULL, + ended = FALSE +WHERE type = 4; + +ALTER TABLE artist +DROP CONSTRAINT IF EXISTS character_type_implies_no_end; + +ALTER TABLE artist +ADD CONSTRAINT character_type_implies_no_end CHECK ( + type != 4 OR ( + end_date_day IS NULL AND + end_date_month IS NULL AND + end_date_year IS NULL AND + end_area IS NULL AND + ended = FALSE + ) +); + +COMMIT; diff --git a/upgrade.json b/upgrade.json index 80853eb68d6..085414f3b7a 100644 --- a/upgrade.json +++ b/upgrade.json @@ -230,5 +230,10 @@ "20240223-mbs-13421-fks.sql", "20240319-mbs-13514.sql" ] + }, + "30": { + "master_and_standalone": [ + "20241104-mbs-9885.sql" + ] } } From 5d9e1a14c457ad82f69c5cceceddcc013d9103e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Tue, 5 Nov 2024 21:56:43 +0100 Subject: [PATCH 4/5] Fix issues with localized_note The way this currently works causes issues with functions like comma_list (because it inserts an unwanted message even if none is passed) and with attempts to call it recursively on its own arguments (such as a localized_note containing additional levels of localized_note variables). --- lib/MusicBrainz/Server/Entity/EditNote.pm | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/MusicBrainz/Server/Entity/EditNote.pm b/lib/MusicBrainz/Server/Entity/EditNote.pm index 7adde8cd0db..46d18b476a7 100644 --- a/lib/MusicBrainz/Server/Entity/EditNote.pm +++ b/lib/MusicBrainz/Server/Entity/EditNote.pm @@ -7,7 +7,10 @@ use Moose; use namespace::autoclean; use MusicBrainz::Server::Constants qw( $EDITOR_MODBOT ); -use MusicBrainz::Server::Data::Utils qw( datetime_to_iso8601 ); +use MusicBrainz::Server::Data::Utils qw( + contains_string + datetime_to_iso8601 +); use MusicBrainz::Server::Entity::Types; use MusicBrainz::Server::Entity::Util::JSON qw( to_json_object ); use MusicBrainz::Server::Filters qw( format_editnote ); @@ -75,17 +78,22 @@ sub _localize_text { my $version = $source->{version} // 0; my $fn_package = 'MusicBrainz::Server::Translation'; my $fn_name = 'l'; - my @args = ($source->{message} // ''); + my @args; my $source_vars; if ($version == 0) { + @args = ($source->{message} // ''); # old versions of `localized_note` passed substitution # variables as `args`. $source_vars = $source->{args}; } elsif ($version == 1) { - push @args, @{ $source->{args} // [] }; + my $no_message_functions = [ qw ( comma_list comma_only_list ) ]; $fn_name = $source->{function} // 'l'; + @args = ($source->{message} // '') + unless contains_string($no_message_functions, $fn_name); + push @args, map { _localize_text($_, $depth + 1) } @{ $source->{args} // [] }; + $source_vars = $source->{vars}; my $domain = $source->{domain}; From bc900c681d26471571efd577f9c46a91ebf343f4 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 5/5] 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 | 140 ++++++++++--- lib/MusicBrainz/Server/Edit/Artist/Merge.pm | 120 +++++++++-- .../t/MusicBrainz/Server/Edit/Artist/Merge.pm | 192 ++++++++++++++++++ t/sql/edit_artist_merge.sql | 3 + 4 files changed, 414 insertions(+), 41 deletions(-) diff --git a/lib/MusicBrainz/Server/Data/Artist.pm b/lib/MusicBrainz/Server/Data/Artist.pm index 0b632236b3f..4ddd7d953cd 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; + 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 { 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 { - $dropped_columns{gender} = $merged_gender; - $dropped_columns{type} = $merged_type; + 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..c4ff18df794 100644 --- a/lib/MusicBrainz/Server/Edit/Artist/Merge.pm +++ b/lib/MusicBrainz/Server/Edit/Artist/Merge.pm @@ -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);