Attachment 120248 Details for Bug 28217 – Bug 28217: Prevent several non-repeatable attributes to be merged

[patch] Bug 28217: Prevent several non-repeatable attributes to be merged

Bug-28217-Prevent-several-non-repeatable-attribute.patch (text/plain), 6.97 KB, created by Victor Grousset/tuxayo on 2021-04-28 00:08:50 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Victor Grousset/tuxayo

Created: 2021-04-28 00:08:50 UTC

Size: 6.97 KB

patch

obsolete

>From 926e58bb864ae270d8ad6b50bf8ed2d0c4aea57d Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>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.
>
>Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
>---
> 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 d47b3f77d2..dc87674f4d 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 5d54d56fbe..120e981324 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.31.1

Actions: View | Diff | Splinter Review

Attachments on bug 28217: 120165 | 120248 | 120325