Skip to content

Commit

Permalink
MBS-9885: Deal with character vs ended while merging
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@reosarevok
reosarevok committed Nov 5, 2024
1 parent 5d9e1a1 commit d67571d
Show file tree
Hide file tree
Showing 4 changed files with 416 additions and 43 deletions.
142 changes: 118 additions & 24 deletions lib/MusicBrainz/Server/Data/Artist.pm
View file
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;
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(
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
122 changes: 103 additions & 19 deletions lib/MusicBrainz/Server/Edit/Artist/Merge.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -65,45 +65,129 @@ 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'],
),
},
),
},
);
}

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.')
),
},
),
Expand Down
Loading

0 comments on commit d67571d

Please sign in to comment.