Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. Feature was added in ticket 23486.
The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron. 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]".
I think we need a new method validate_guarantor instead of checking guarantor validity in Koha::Patron->store. This way we can ensure that saving patron doesn't fail if store is called again in another function and we can create tests for this new method.
Created attachment 177563 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 177564 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 177619 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Jake Deery <jake.deery@ptfs-europe.com>
All works as intended, now! Signed off Jake.
Created attachment 178546 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
There were some things I forgot to handle in the original patch. Patch is now rebased against current main.
Created attachment 178547 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 179572 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy
Created attachment 180298 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised.
Created attachment 182290 [details] [review] Bug 39014: Add new method validate_guarantor Adding a new guarantee and storing it fails with "Creating a patron" selected in TrackLastPatronActivityTriggers when guarantors are set to be mandatory. The problem comes from Koha::Patron->store called in update_lastseen. Since we validate guarantor in Koha::Patron->store but don't store them in there, new call to store fails because it doesn't receive guarantor info as parameter. As a solution, we should validate guarantors in separated method. Validation can't be moved e.g. to Koha::Patron::Relationship->store since this could lead to situations where guarantee is stored without guarantor. This patch adds new method validate_guarantor and adjusts old guarantor requirements tests to match this new method. To test: 1. Select option "Creating a patron" in "TrackLastPatronActivityTriggers" syspref. 2. Enable syspref "ChildNeedsGuarantor" and make sure you have options in "borrowerRelationship" syspref. 3. Create a guarantee patron, fill all required fields and add guarantor for patron (either add Patron guarantor or fill Non-patron guarantor inputs). 4. Attempt to save patron. => Error box is displayed with text "The following fields are wrong. Please fix them.". => Logs show error "Patron creation failed! - [Child patron needs a guarantor]". 5. Apply this patch. 6. Create a guarantee patron again and save. => Patron should be saved correctly. 7. Create a new guarantee patron but this time don't add patron guarantor or fill non-patron guarantor inputs. 8. Attempt to save patron. => Save should fail with error "A child patron needs a guarantor." 9. Add patron guarantor who can also be a guarantee and save. => Save should fail with error "A guarantor cannot be a guarantee." Also prove t/db_dependent/Koha/Patron.t Sponsored-by: Koha-Suomi Oy Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Signed-off-by: David Nind <david@davidnind.com>
Added my sign-off, in case Magnus's can count as QA
Looking here
QA Comment: Thx for your patch ! Koha::Patron->validate_guarantor( \@guarantors, $newdata{'contactname'}, $category ); validate_guarantor is a CLASS method ! Should we at least reflect that in the code by renaming to $class ? Or should we default to checking guarantor_relationships when you would pass a patron in $self ? =head3 validate_guarantor Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category ); Validates guarantor patron. =cut Could you elaborate "validate guarantor patron"? Singular ? Having the impression that we do two separate things here? FIrst we check ChildNeedsGuarantor, which is not really validating guarantors, right? And secondly we walk thru all guarantors (which is the name of the sub). How can this test on patron work in all cases? if ( ( $op eq 'cud-save' || $op eq 'cud-insert' ) ) { unless ( $patron && $patron->is_guarantor ) { [etc] } else { push @errors, "ERROR_guarantee_is_guarantor"; } } Test adding a regular patron: [2025/05/30 09:06:36] [WARN] L291 at /usr/share/koha/members/memberentry.pl line 291. Test modify a guarantor: The following fields are wrong. Please fix them. A guarantor cannot be a guarantee. The patron is no guarantee! The test here is WRONG.