Attachment 172896 Details for Bug 37892 – Bug 37892: Fix guarantor restriction, add tests

[patch] Bug 37892: Fix guarantor restriction, add tests

Bug-37892-Fix-guarantor-restriction-add-tests.patch (text/plain), 8.23 KB, created by Martin Renvoize (ashimema) on 2024-10-17 15:08:52 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2024-10-17 15:08:52 UTC

Size: 8.23 KB

patch

obsolete

>From ec620c766dc0f92ffe03832ca8218fc788ca37cc Mon Sep 17 00:00:00 2001
>From: Nick Clemens <nick@bywatersolutions.com>
>Date: Wed, 11 Sep 2024 14:04:17 +0000
>Subject: [PATCH] Bug 37892: Fix guarantor restriction, add tests
>
>[SQUASHED IN QA]
>
>These patches will alter the checks for a patron that prevent a category with
>'can_be_guarantee' from being a guarantor.  Two patrons in the same category should be
>allowed to have a guarantee/guarantor relationship
>
>The tests below assume you are using the KTD sample data. Update borrowernumbers if not.
>
>To test:
> 0 - Apply tests patch
> 1 - Set the 'Patron' category as 'Can be a guarantee'
> 2 - Add a relationship between two patrons of the same category
>     This is restricted from the staff interface
>     perl -e 'use Koha::Patrons; my $p = Koha::Patrons->find(5)->add_guarantor({ guarantor_id => 23, relationship => 'father'});'
> 3 - Note there is no warning or exception. This should be allowed.
> 4 - Checkout an item to Edna (borrowernumber 5)
> 5 - Set 'TrackLastPatronActivityTriggers' to 'Checking in an item'
> 6 - Try to check the item in, KABOOM
> 7 - Set 'TrackLastPatronActivityTriggers' to 'Checking out an item'
> 8 - Try to issue an item to Enda, KABOOM
> 9 - prove -v t/db_dependent/Koha/Patron.t, fail
>10 - Apply second patch
>11 - prove -v t/db_dependent/Koha/Patron.t, one more test passes, but then fail
>12 - Apply third patch
>13 - prove -v t/db_dependent/Koha/Patron.t, pass!
>14 - restart_all
>15 - Checkout to Enda, OK!
>16 - Checkin from Edna, OK!
>17 - Find two more patrons in the category and attempt to link them
>18 - 'Guarantor cannot be a guarantee'
>19 - Apply fourth patch
>20 - You can add a guarantor from the same category in interface
>21 - Try to add a guarantor to the guarantor assigned in 20
>22 - Confirm you cannot add a guarantor - "Guarantor cannot be a guarantee"
>
>Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
>
>Bug 37892: Fix patron updates
>
>Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
>
>Bug 37892: Fix patron creation
>
>Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
>
>Bug 37892: Fix memberentry.pl
>
>Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
>
>Bug 37892: (follow-up) Fix patron creation
>
>This patch fixes the 22. of the test plan (22 - Confirm you cannot add a guarantor - "Guarantor cannot be a guarantee"
>        )
>
>Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>
>Bug 37892: (follow-up) Fix patron creation
>
>TEST PLAN:
>1 - Do the 22 parts of the test plan
>2 - Add a guarantor to one patron not selected before (let's say A is
>  the guarantee, B the guarantor)
>3 - Try and add a guarantor to B -> you will success
>4 - Remove B's guarantor
>5 - Apply this patch
>6 - Repeat 3 -> you will not be able to
>
>Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
>
>Bug 37892: (follow-up) Tidyness
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>[EDIT]
>Renamed a subtest to patron creation tests in Patron.t.
>Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>---
> Koha/Patron.pm               | 28 +++++++++++++++++++--
> members/memberentry.pl       |  2 +-
> t/db_dependent/Koha/Patron.t | 47 +++++++++++++++++++++++++++++++++++-
> 3 files changed, 73 insertions(+), 4 deletions(-)
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index cf359d4b3be..d23792a6b37 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -315,7 +315,7 @@ sub store {
>                 }
> 
>                 foreach my $guarantor (@$guarantors) {
>-                    if ( $guarantor->is_child or $guarantor->category->can_be_guarantee ) {
>+                    if ( $guarantor->is_child ) {
>                         Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 );
>                     }
>                 }
>@@ -372,7 +372,7 @@ sub store {
>                 }
> 
>                 foreach my $guarantor (@$guarantors) {
>-                    if ( $guarantor->is_child or $guarantor->category->can_be_guarantee ) {
>+                    if ( $guarantor->is_child ) {
>                         Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( invalid_guarantor => 1 );
>                     }
>                 }
>@@ -533,6 +533,18 @@ sub guarantor_relationships {
>     return Koha::Patron::Relationships->search( { guarantee_id => $self->id } );
> }
> 
>+=head3 is_guarantee
>+
>+Returns true if the patron has a guarantor.
>+
>+=cut
>+
>+sub is_guarantee {
>+    my ($self) = @_;
>+    return $self->guarantor_relationships()->count();
>+}
>+
>+
> =head3 guarantee_relationships
> 
> Returns Koha::Patron::Relationships object for this patron's guarantors
>@@ -557,6 +569,18 @@ sub guarantee_relationships {
>     );
> }
> 
>+=head3 is_guarantor
>+
>+Returns true if the patron is a guarantor.
>+
>+=cut
>+
>+sub is_guarantor {
>+    my ($self) = @_;
>+    return $self->guarantee_relationships()->count();
>+}
>+
>+
> =head3 relationships_debt
> 
> Returns the amount owed by the patron's guarantors *and* the other guarantees of those guarantors
>diff --git a/members/memberentry.pl b/members/memberentry.pl
>index 44e6b982b2f..eb92536fcc6 100755
>--- a/members/memberentry.pl
>+++ b/members/memberentry.pl
>@@ -285,7 +285,7 @@ if (   ( $op eq 'cud-save' || $op eq 'cud-insert' )
> }
> 
> foreach my $guarantor (@guarantors) {
>-    if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) && $guarantor->is_child || $guarantor->category->can_be_guarantee ) {
>+    if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) && ($guarantor->is_child ) || $guarantor->is_guarantee || $patron->is_guarantor)  {
>         push @errors, 'ERROR_guarantor_is_guarantee';
>     }
> }
>diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t
>index 3e19279f76b..a885a4f4a5a 100755
>--- a/t/db_dependent/Koha/Patron.t
>+++ b/t/db_dependent/Koha/Patron.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 35;
>+use Test::More tests => 36;
> use Test::Exception;
> use Test::Warn;
> use Time::Fake;
>@@ -410,6 +410,51 @@ subtest 'add_guarantor() tests' => sub {
>     $schema->storage->txn_rollback;
> };
> 
>+subtest 'guarantor checks on patron creation / update tests' => sub {
>+
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    t::lib::Mocks::mock_preference( 'borrowerRelationship', 'guarantor' );
>+    my $category = $builder->build_object(
>+        { class => 'Koha::Patron::Categories', value => { can_be_guarantee => 1, category_type => 'A' } } );
>+
>+    my $guarantor =
>+        $builder->build_object( { class => 'Koha::Patrons', value => { categorycode => $category->categorycode } } );
>+    my $guarantee =
>+        $builder->build_object( { class => 'Koha::Patrons', value => { categorycode => $category->categorycode } } );
>+
>+    subtest 'patron update tests' => sub {
>+        plan tests => 4;
>+        ok(
>+            $guarantee->add_guarantor( { guarantor_id => $guarantor->borrowernumber, relationship => 'guarantor' } ),
>+            "Relationship is added, no problem"
>+        );
>+        is( $guarantor->guarantee_relationships->count, 1, 'Relationship added' );
>+        ok( $guarantor->surname("Duck")->store(), "Updating guarantor is okay" );
>+        ok( $guarantee->surname("Duck")->store(), "Updating guarantee is okay" );
>+    };
>+
>+    subtest 'patron creation tests' => sub {
>+        plan tests => 1;
>+
>+        my $new_guarantee = $builder->build_object(
>+            { class => 'Koha::Patrons', value => { categorycode => $category->categorycode } } );
>+        my $new_guarantee_hash = $new_guarantee->unblessed;
>+        $new_guarantee->delete();
>+
>+        delete $new_guarantee_hash->{borrowernumber};
>+
>+        ok(
>+            Koha::Patron->new($new_guarantee_hash)->store( { guarantors => [$guarantor] } ),
>+            "We can add the new patron and indicate guarantor"
>+        );
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
> subtest 'relationships_debt() tests' => sub {
> 
>     plan tests => 168;
>-- 
>2.47.0

Actions: View | Diff | Splinter Review

Attachments on bug 37892: 171334 | 171335 | 171336 | 171337 | 171451 | 171463 | 171464 | 171465 | 171466 | 171467 | 171472 | 171475 | 171476 | 171477 | 171478 | 171479 | 171480 | 171546 | 171547 | 171548 | 171549 | 171550 | 171663 | 171664 | 171665 | 171824 | 171825 | 171826 | 172896 | 172897 | 172898