Bug 37892 - Patron category 'can be a guarantee' means that same category cannot be a guarantor
Summary: Patron category 'can be a guarantee' means that same category cannot be a gua...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 37528 12133
Blocks: 31470
  Show dependency treegraph
 
Reported: 2024-09-11 11:16 UTC by Nick Clemens (kidclamp)
Modified: 2024-10-23 23:02 UTC (History)
13 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.05
Circulation function:


Attachments
Bug 37892: Unit tests (4.13 KB, patch)
2024-09-11 14:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37892: Fix patron updates (828 bytes, patch)
2024-09-11 14:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37892: Fix patron creation (829 bytes, patch)
2024-09-11 14:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37892: Fix memberentry.pl (843 bytes, patch)
2024-09-11 14:41 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.53 KB, patch)
2024-09-13 11:58 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Unit tests (4.19 KB, patch)
2024-09-13 14:15 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37892: Fix patron updates (880 bytes, patch)
2024-09-13 14:15 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37892: Fix patron creation (881 bytes, patch)
2024-09-13 14:15 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37892: Fix memberentry.pl (895 bytes, patch)
2024-09-13 14:15 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.60 KB, patch)
2024-09-13 14:15 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.70 KB, patch)
2024-09-13 15:03 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Unit tests (4.24 KB, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: Fix patron updates (933 bytes, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: Fix patron creation (934 bytes, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: Fix memberentry.pl (948 bytes, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.65 KB, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.76 KB, patch)
2024-09-13 15:18 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37892: Unit tests (4.30 KB, patch)
2024-09-16 13:49 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Fix patron updates (997 bytes, patch)
2024-09-16 13:49 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Fix patron creation (998 bytes, patch)
2024-09-16 13:49 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Fix memberentry.pl (1012 bytes, patch)
2024-09-16 13:49 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: (follow-up) Tidyness (1006 bytes, patch)
2024-09-16 13:56 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.64 KB, patch)
2024-09-18 13:50 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: (follow-up) Fix patron creation (1.75 KB, patch)
2024-09-18 13:50 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: (follow-up) Tidyness (1006 bytes, patch)
2024-09-18 13:50 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37892: Fix guarantor restriction, add tests (8.20 KB, patch)
2024-09-20 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37892: (QA follow-up) Add tests for ->is_guarantor/ee (1.63 KB, patch)
2024-09-20 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37892: (QA follow-up) Fix condition in memberentry (1.08 KB, patch)
2024-09-20 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37892: Fix guarantor restriction, add tests (8.23 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37892: (QA follow-up) Add tests for ->is_guarantor/ee (1.66 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37892: (QA follow-up) Fix condition in memberentry (1.11 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2024-09-11 11:16:23 UTC
Bug 12133 changed the way that these categories work. Before hand, a single category could be set up, and two patrons in that category could be set to have a guarantor/guarantee relationship.

In upgrades, we are seeing that if this relationship is preexisting, patrons are unable to return items as we throw an exception: "Child cannot be a guarantor"

I understand preventing the same patron from being a guarantor and a guarantee, however, is it correct that the same category cannot?

We either need to:
1 - Add an update to fix or warn of this existing condition
2 - Remove this restriction
Comment 1 Nick Clemens (kidclamp) 2024-09-11 14:41:18 UTC
Created attachment 171334 [details] [review]
Bug 37892: Unit tests

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"
Comment 2 Nick Clemens (kidclamp) 2024-09-11 14:41:20 UTC
Created attachment 171335 [details] [review]
Bug 37892: Fix patron updates
Comment 3 Nick Clemens (kidclamp) 2024-09-11 14:41:22 UTC
Created attachment 171336 [details] [review]
Bug 37892: Fix patron creation
Comment 4 Nick Clemens (kidclamp) 2024-09-11 14:41:24 UTC
Created attachment 171337 [details] [review]
Bug 37892: Fix memberentry.pl
Comment 5 Baptiste Wojtkowski (bwoj) 2024-09-12 08:42:46 UTC
Hi !
I could not reproduce the errror in 18- : I do not have any button in the interface to add a guarantor and the command you gave in 2- works perfectly. The rest works perfectly atm
Comment 6 Nick Clemens (kidclamp) 2024-09-12 11:36:03 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #5)
> Hi !
> I could not reproduce the errror in 18- : I do not have any button in the
> interface to add a guarantor and the command you gave in 2- works perfectly.
> The rest works perfectly atm

For 18 it is not a single button, apologies:
1 - Find a patron in the category that can be a guarantee
2 - Edit the patron
3 - The second section should be  'Patron guarantor' with an 'Add guarantor' button
4 - Find/Select a patron in the same category - you can
5 - Save the patron
6 - Error
Comment 7 Baptiste Wojtkowski (bwoj) 2024-09-12 11:54:15 UTC
Oh my bad, I picked a user that could be guarantor to be guarantor and was waiting for an error..

However I cannot reproduce the last point. With all patches, I could obtain following only using the interface (data are the same as KTD ones I guess, Dawn and Johnny) :

+-----+--------------+--------------+--------------+
| id  | guarantor_id | guarantee_id | relationship |
+-----+--------------+--------------+--------------+
| 102 |           23 |           31 | father       |
| 103 |           31 |           23 | father       |
+-----+--------------+--------------+--------------+
Comment 8 Baptiste Wojtkowski (bwoj) 2024-09-13 11:58:55 UTC
Created attachment 171451 [details] [review]
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"
        )
Comment 9 Baptiste Wojtkowski (bwoj) 2024-09-13 12:00:02 UTC
Note : Can I signoff the patch and request to signoff my follow-up or does it need a third person to sign off the whole ?
Comment 10 Olivier Vezina 2024-09-13 14:15:32 UTC
Created attachment 171463 [details] [review]
Bug 37892: Unit tests

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>
Comment 11 Olivier Vezina 2024-09-13 14:15:34 UTC
Created attachment 171464 [details] [review]
Bug 37892: Fix patron updates

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 12 Olivier Vezina 2024-09-13 14:15:36 UTC
Created attachment 171465 [details] [review]
Bug 37892: Fix patron creation

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 13 Olivier Vezina 2024-09-13 14:15:38 UTC
Created attachment 171466 [details] [review]
Bug 37892: Fix memberentry.pl

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Comment 14 Olivier Vezina 2024-09-13 14:15:40 UTC
Created attachment 171467 [details] [review]
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>
Comment 15 Brendan Lawlor 2024-09-13 14:30:47 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #7)
> Oh my bad, I picked a user that could be guarantor to be guarantor and was
> waiting for an error..
> 
> However I cannot reproduce the last point. With all patches, I could obtain
> following only using the interface (data are the same as KTD ones I guess,
> Dawn and Johnny) :
> 
> +-----+--------------+--------------+--------------+
> | id  | guarantor_id | guarantee_id | relationship |
> +-----+--------------+--------------+--------------+
> | 102 |           23 |           31 | father       |
> | 103 |           31 |           23 | father       |
> +-----+--------------+--------------+--------------+

When I tested I found the 5th patch resolved this issue, and if you try you get the error message: A guarantor cannot be a guarantee.

But if you use a third patron when you add the guarantor it still lets you.
+----+--------------+--------------+--------------+
| id | guarantor_id | guarantee_id | relationship |
+----+--------------+--------------+--------------+
|  1 |           23 |            5 | father       |
|  2 |           38 |           23 | father       |
+----+--------------+--------------+--------------+

Not sure if this is an issue or if this is supposed to be allowed.
Comment 16 Baptiste Wojtkowski (bwoj) 2024-09-13 14:45:10 UTC Comment hidden (obsolete)
Comment 17 Baptiste Wojtkowski (bwoj) 2024-09-13 14:47:22 UTC
I'm stupid it's the order of the rules...

Looks coherent with the patch applied, it checks the guarantor is not a guarantee, but it should also check that the guarantee is not already a guarantor
Comment 18 Brendan Lawlor 2024-09-13 14:54:40 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #17)
> I'm stupid it's the order of the rules...
> 
> Looks coherent with the patch applied, it checks the guarantor is not a
> guarantee, but it should also check that the guarantee is not already a
> guarantor

I will test again and sign off when there's a follow up
Comment 19 Baptiste Wojtkowski (bwoj) 2024-09-13 15:03:28 UTC
Created attachment 171472 [details] [review]
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
Comment 20 Olivier Vezina 2024-09-13 15:14:00 UTC
My bad i missed this detail. The patch is working fine on my part, i'll let Brendan check it and sign it off to be sure
Comment 21 Brendan Lawlor 2024-09-13 15:18:17 UTC
Created attachment 171475 [details] [review]
Bug 37892: Unit tests

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>
Comment 22 Brendan Lawlor 2024-09-13 15:18:19 UTC
Created attachment 171476 [details] [review]
Bug 37892: Fix patron updates

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 23 Brendan Lawlor 2024-09-13 15:18:21 UTC
Created attachment 171477 [details] [review]
Bug 37892: Fix patron creation

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 24 Brendan Lawlor 2024-09-13 15:18:23 UTC
Created attachment 171478 [details] [review]
Bug 37892: Fix memberentry.pl

Signed-off-by: Olivier V <olivier.vezina@inLibro.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 25 Brendan Lawlor 2024-09-13 15:18:25 UTC
Created attachment 171479 [details] [review]
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>
Comment 26 Brendan Lawlor 2024-09-13 15:18:28 UTC
Created attachment 171480 [details] [review]
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>
Comment 27 Baptiste Wojtkowski (bwoj) 2024-09-16 13:49:19 UTC
Created attachment 171546 [details] [review]
Bug 37892: Unit tests

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>
Comment 28 Baptiste Wojtkowski (bwoj) 2024-09-16 13:49:22 UTC
Created attachment 171547 [details] [review]
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>
Comment 29 Baptiste Wojtkowski (bwoj) 2024-09-16 13:49:25 UTC
Created attachment 171548 [details] [review]
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>
Comment 30 Baptiste Wojtkowski (bwoj) 2024-09-16 13:49:28 UTC
Created attachment 171549 [details] [review]
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>
Comment 31 Baptiste Wojtkowski (bwoj) 2024-09-16 13:56:09 UTC
Created attachment 171550 [details] [review]
Bug 37892: (follow-up) Tidyness
Comment 32 Baptiste Wojtkowski (bwoj) 2024-09-18 13:50:07 UTC
Created attachment 171663 [details] [review]
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>
Comment 33 Baptiste Wojtkowski (bwoj) 2024-09-18 13:50:09 UTC
Created attachment 171664 [details] [review]
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>
Comment 34 Baptiste Wojtkowski (bwoj) 2024-09-18 13:50:12 UTC
Created attachment 171665 [details] [review]
Bug 37892: (follow-up) Tidyness
Comment 35 Marcel de Rooy 2024-09-20 07:52:46 UTC
Looking here. Squashing those patches.
Comment 36 Marcel de Rooy 2024-09-20 09:09:40 UTC
Created attachment 171824 [details] [review]
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.
Comment 37 Marcel de Rooy 2024-09-20 09:09:43 UTC
Created attachment 171825 [details] [review]
Bug 37892: (QA follow-up) Add tests for ->is_guarantor/ee

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2024-09-20 09:09:46 UTC
Created attachment 171826 [details] [review]
Bug 37892: (QA follow-up) Fix condition in memberentry

Patron may be undefined. So the test may crash.
Also there is an issue with operator precedence.
Comment 39 Marcel de Rooy 2024-09-20 09:11:18 UTC
We are not there yet unfortunately.

There is still an issue with at least patron creation. The patron is created now but I am missing the borrower_relationships.
Also note that if I do not fill the relationship, the form crashes on an invalid relation.
Comment 40 Marcel de Rooy 2024-09-20 09:11:51 UTC
NOTE: I left the original patches, but the last three patches should do the tric.
Comment 41 Baptiste Wojtkowski (bwoj) 2024-09-20 15:30:42 UTC
> We are not there yet unfortunately.
> 
> There is still an issue with at least patron creation. The patron is created
> now but I am missing the borrower_relationships.

I'll work on it on Monday

> Also note that if I do not fill the relationship, the form crashes on an
> invalid relation.

This is not the scope of this bug I guess, it should be solved by BZ37528
Comment 42 Baptiste Wojtkowski (bwoj) 2024-09-23 10:02:30 UTC
(In reply to Marcel de Rooy from comment #39)
> We are not there yet unfortunately.
> 
> There is still an issue with at least patron creation. The patron is created
> now but I am missing the borrower_relationships.

Hum.. I could not reproduce, I upgraded to the most recent main but I could not manage to have an unfilled borrowers_relationship when I 

My test plan:
1 - Editing one Patron (category patron) to add Edna Acosta as father -> does work
2 - Creating a new patron (category patron), select Edna Acosta as father in the form -> does work

Could you please provide your test plan so that I can try and reproduce ? :)
Comment 43 Lucas Gass (lukeg) 2024-10-07 22:30:46 UTC
Since Marcel squashed many patches here I obsoleted the ones we don't need to make it less confusing.
Comment 44 Lucas Gass (lukeg) 2024-10-07 22:31:26 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #42)
> (In reply to Marcel de Rooy from comment #39)
> > We are not there yet unfortunately.
> > 
> > There is still an issue with at least patron creation. The patron is created
> > now but I am missing the borrower_relationships.
> 
> Hum.. I could not reproduce, I upgraded to the most recent main but I could
> not manage to have an unfilled borrowers_relationship when I 
> 
> My test plan:
> 1 - Editing one Patron (category patron) to add Edna Acosta as father ->
> does work
> 2 - Creating a new patron (category patron), select Edna Acosta as father in
> the form -> does work
> 
> Could you please provide your test plan so that I can try and reproduce ? :)

I was also unable to reproduce this.
Comment 45 Lucas Gass (lukeg) 2024-10-07 22:33:20 UTC
> > Also note that if I do not fill the relationship, the form crashes on an
> > invalid relation.
> 
> This is not the scope of this bug I guess, it should be solved by BZ37528

Ah, yes. I think we should make Bug 37528 a dependency of this one. I do have a question on that bug. It would be nice to move these along together.
Comment 46 Baptiste Wojtkowski (bwoj) 2024-10-08 09:47:24 UTC
Since there is no functional dependency and they only cover separate regressions (?) introduced by recent changes in Borrower object, I'm not a great fan of setting a dependency relationship between both bugs.

In particular, imo this patch makes sense by itself and should be pushed to main as soon as it passes QA, even if it does not solve the whole problem of guarantors.
Comment 47 Lucas Gass (lukeg) 2024-10-14 21:28:18 UTC
From what I can tell this is FQA for 2 reasons:

1. There is still an issue with at least patron creation. The patron is created now but I am missing the borrower_relationships.
- I can't recreate this. Seems like Baptiste cannot either.


2. Also note that if I do not fill the relationship, the form crashes on an invalid relation.
- Work is being done a seperate bug to deal with this.


Let's set this back to NQA as it is important to get this fix through ASAP.
Comment 48 Lucas Gass (lukeg) 2024-10-14 21:28:51 UTC
Marcel, will you come back to this one?
Comment 49 Martin Renvoize (ashimema) 2024-10-17 15:08:52 UTC
Created attachment 172896 [details] [review]
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>
Comment 50 Martin Renvoize (ashimema) 2024-10-17 15:08:55 UTC
Created attachment 172897 [details] [review]
Bug 37892: (QA follow-up) Add tests for ->is_guarantor/ee

Test plan:
Run t/db_dependent/Koha/Patron.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize (ashimema) 2024-10-17 15:08:58 UTC
Created attachment 172898 [details] [review]
Bug 37892: (QA follow-up) Fix condition in memberentry

Patron may be undefined. So the test may crash.
Also there is an issue with operator precedence.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Martin Renvoize (ashimema) 2024-10-17 15:10:36 UTC
Clear fix of a regression, Works as described and makes sense atop it's dependencies.

Passing QA
Comment 53 Marcel de Rooy 2024-10-18 06:07:07 UTC
(In reply to Lucas Gass (lukeg) from comment #48)
> Marcel, will you come back to this one?

Guess, i am too late :)
Comment 54 Katrin Fischer 2024-10-18 12:27:31 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 55 Todd Goatley 2024-10-21 16:56:18 UTC
Any chance this can be back ported to 24.05? Your consideration is greatly appreciated.
Comment 56 Lucas Gass (lukeg) 2024-10-23 23:02:37 UTC
Backported to 24.05.x for upcoming 24.05.05