[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]
metabrainz · Oct 31, 2024 · 04bd606 · 04bd606
1 parent 464e9c5
commit 04bd606
Show file tree

Hide file tree

Showing 7 changed files with 214 additions and 16 deletions.
diff --git 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
@@ -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
@@ -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
@@ -60,20 +60,22 @@
           </span>
           [% 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') %]
-          <label id="label-id-edit-artist.end_area.name" for="id-edit-artist.end_area.name">[% add_colon(l('End area')) %]</label>
-          <span id="end_area" class="area autocomplete">
-            [% 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') %]
-          </span>
-          [% field_errors(r.form, 'end_area.name') %]
-        [% END %]
+        <div id="artist.end_date_section">
+          [% 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') %]
+            <label id="label-id-edit-artist.end_area.name" for="id-edit-artist.end_area.name">[% add_colon(l('End area')) %]</label>
+            <span id="end_area" class="area autocomplete">
+              [% 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') %]
+            </span>
+            [% field_errors(r.form, 'end_area.name') %]
+          [% END %]
+        </div>
       </fieldset>
 
       [% 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
@@ -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
@@ -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
@@ -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;