From 3cb0b5dc447f890d678ecf4315a2f4442f89c66a 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] [WIP] 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 genres 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. [Still missing: merge code, and a db-level constraint] --- 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;