Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MBS-9885: Prevent setting end date for character artist #3402

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

MBS-9885: Prevent setting end date for character artist #3402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cd35506
MBS-13802: Always use "Created (in)" for characters
reosarevok Oct 31, 2024
b73af6d
MBS-9885: Prevent setting end date for character artist
reosarevok Oct 31, 2024
7c53816
MBS-9885: Add character_type_implies_no_end DB constraint
reosarevok Nov 4, 2024
5d9e1a1
Fix issues with localized_note
reosarevok Nov 5, 2024
bc900c6
MBS-9885: Deal with character vs ended while merging
reosarevok Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions admin/sql/CreateConstraints.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
27 changes: 27 additions & 0 deletions admin/sql/updates/20241104-mbs-9885.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
33 changes: 33 additions & 0 deletions admin/sql/updates/schema-change/30.master_and_standalone.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
3 changes: 2 additions & 1 deletion lib/MusicBrainz/Server/Constants.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
140 changes: 117 additions & 23 deletions lib/MusicBrainz/Server/Data/Artist.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 );
Expand Down Expand Up @@ -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(
Expand All @@ -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)
Expand All @@ -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(
Expand All @@ -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);
Expand Down
25 changes: 25 additions & 0 deletions lib/MusicBrainz/Server/Edit/Artist/Edit.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand All @@ -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;
};

Expand Down
Loading