From 5c5028f822624d751e68f94af85c86405ff88eaa Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 23 Apr 2021 15:37:46 +0200 Subject: [PATCH] Bug 28217: Prevent several non-repeatable attributes to be merged When using the patron merge feature it's possible to generate a patron with several non-repeatable attributes. This patch prevents that. Test plan: Create 2 patron attribute types, one repeatable and one non-repeatable Create 2 patrons and add them repeatable attributes Add a non-repeatable attribute to one of them Merge the 2 patrons => It succeeds, the resulting patron has all the repeatable attribute and the non-repeatable one. Do it again but this time add non-repeatable to both patrons Merge them => It fails, you should see an error on the UI Merge failed! The following error was reported: Tried to add more than one non-repeatable attributes. type=TYPE value=VALUE. --- Koha/Patron.pm | 11 +++- t/db_dependent/Koha/Patrons.t | 114 +++++++++++++++++++++++++++++++++- 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index d47b3f77d25..dc87674f4d5 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -62,7 +62,6 @@ our $RESULTSET_PATRON_ID_MAPPING = { Aqbudget => 'budget_owner_id', Aqbudgetborrower => 'borrowernumber', ArticleRequest => 'borrowernumber', - BorrowerAttribute => 'borrowernumber', BorrowerDebarment => 'borrowernumber', BorrowerFile => 'borrowernumber', BorrowerModification => 'borrowernumber', @@ -629,6 +628,16 @@ sub merge_with { # Unbless for safety, the patron will end up being deleted $results->{merged}->{$patron_id}->{patron} = $patron->unblessed; + my $attributes = $patron->extended_attributes; + my $new_attributes = [ + map { { code => $_->code, attribute => $_->attribute } } + $attributes->as_list + ]; + $attributes->delete; # We need to delete before trying to merge them to prevent exception on unique and repeatable + for my $attribute ( @$new_attributes ) { + $self->add_extended_attribute($attribute); + } + while (my ($r, $field) = each(%$RESULTSET_PATRON_ID_MAPPING)) { my $rs = $schema->resultset($r)->search({ $field => $patron_id }); $results->{merged}->{ $patron_id }->{updated}->{$r} = $rs->count(); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 5d54d56fbe7..120e9813243 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1606,7 +1606,7 @@ subtest 'BorrowersLog tests' => sub { $schema->storage->txn_rollback; subtest 'Test Koha::Patrons::merge' => sub { - plan tests => 113; + plan tests => 110; my $schema = Koha::Database->new()->schema(); @@ -1657,6 +1657,118 @@ subtest 'Test Koha::Patrons::merge' => sub { is( $results, undef, "Anonymous patron cannot have other patrons merged into it" ); is( Koha::Patrons->search( { borrowernumber => $keeper->id } )->count, 1, "Patron from attempted merge with AnonymousPatron still exists" ); + subtest 'extended attributes' => sub { + plan tests => 5; + + my $keep_patron = + $builder->build_object( { class => 'Koha::Patrons' } ); + my $merge_patron = + $builder->build_object( { class => 'Koha::Patrons' } ); + + my $attribute_type_normal_1 = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 0, unique_id => 0 } + } + ); + my $attribute_type_normal_2 = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 0, unique_id => 0 } + } + ); + + my $attribute_type_repeatable = $builder->build_object( + { + class => 'Koha::Patron::Attribute::Types', + value => { repeatable => 1, unique_id => 0 } + } + ); + + my $attr_keep = [ + { + code => $attribute_type_normal_1->code, + attribute => 'from attr 1' + }, + { + code => $attribute_type_repeatable->code, + attribute => 'from attr repeatable' + } + ]; + + my $attr_merge = [ + { + code => $attribute_type_normal_2->code, + attribute => 'to attr 2' + }, + { + code => $attribute_type_repeatable->code, + attribute => 'to attr repeatable' + }, + ]; + + $keep_patron->extended_attributes($attr_keep); + $merge_patron->extended_attributes($attr_merge); + + $keep_patron->merge_with( [ $merge_patron->borrowernumber ] ); + my $merged_attributes = $keep_patron->extended_attributes; + is( $merged_attributes->count, 4 ); + + sub compare_attributes { + my ( $got, $expected, $code ) = @_; + + is_deeply( + [ + sort $got->search( { code => $code } ) + ->get_column('attribute') + ], + $expected + ); + } + compare_attributes( + $merged_attributes, + ['from attr 1'], + $attribute_type_normal_1->code + ); + compare_attributes( + $merged_attributes, + ['to attr 2'], + $attribute_type_normal_2->code + ); + compare_attributes( + $merged_attributes, + [ 'from attr repeatable', 'to attr repeatable' ], + $attribute_type_repeatable->code + ); + + # Cleanup + $keep_patron->delete; + $merge_patron->delete; + + # Recreate but expect an exception because 2 "normal" attributes will be in the resulting patron + $keep_patron = + $builder->build_object( { class => 'Koha::Patrons' } ); + $merge_patron = + $builder->build_object( { class => 'Koha::Patrons' } ); + + $keep_patron->extended_attributes($attr_keep); + $merge_patron->extended_attributes( + [ + @$attr_merge, + { + code => $attribute_type_normal_1->code, + attribute => 'yet another attribute for non-repeatable' + } + ] + ); + + throws_ok { + $keep_patron->merge_with( [ $merge_patron->borrowernumber ] ); + } + 'Koha::Exceptions::Patron::Attribute::NonRepeatable', + 'Exception thrown trying to merge several non-repeatable attributes'; + }; + t::lib::Mocks::mock_preference( 'AnonymousPatron', '' ); $schema->storage->txn_rollback; }; -- 2.20.1