https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |emmi.takkinen@koha-suomi.fi |ity.org | Severity|enhancement |normal -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #3 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 177563 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177563&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177563|0 |1 is obsolete| | --- Comment #4 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 177564 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=177564&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Jake Deery <jake.deery@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off CC| |jake.deery@ptfs-europe.com --- Comment #6 from Jake Deery <jake.deery@ptfs-europe.com> --- All works as intended, now! Signed off Jake. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #177619|0 |1 is obsolete| | --- Comment #7 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 178546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178546&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #178546|0 |1 is obsolete| | --- Comment #9 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 178547 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=178547&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |baptiste.wojtkowski@biblibr | |e.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179572|0 |1 is obsolete| | --- Comment #11 from Magnus Enger <magnus@libriotech.no> --- Created attachment 180298 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=180298&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #180298|0 |1 is obsolete| | --- Comment #12 from David Nind <david@davidnind.com> --- Created attachment 182290 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=182290&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Bug 39014 depends on bug 23486, which changed state. Bug 23486 Summary: TrackLastPatronActivityTriggers should have an option for patron creation https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23486 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Emmi Takkinen from comment #16)

...

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. Did this happen while adding patron from Koha or while running tests? Could you provide a plan how to reproduce this?

Adding a regular patron. May 30 is a bit long ago already :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lari.taskula@hypernova.fi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #20 from Lari Taskula <lari.taskula@hypernova.fi> --- We've also come accross with this bug. I attempted to address some of the issues Marcel pointed out. I've also added more detailed error messages, as I could not initially wrap my head around with the difference of "A guarantor cannot be a guarantee" vs "A guarantee cannot be a guarantor" :D -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #182290|0 |1 is obsolete| | --- Comment #22 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Created attachment 188461 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188461&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Emmi Takkinen <emmi.takkinen@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #24 from Emmi Takkinen <emmi.takkinen@koha-suomi.fi> --- Rebased patches. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188461|0 |1 is obsolete| | --- Comment #25 from David Nind <david@davidnind.com> --- Created attachment 188463 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=188463&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #27 from David Nind <david@davidnind.com> --- (In reply to Emmi Takkinen from comment #24)

Rebased patches.

Thanks Emmi! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #29 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Did not finish yet. Will resume next week. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #188463|0 |1 is obsolete| | Attachment #188464|0 |1 is obsolete| | --- Comment #30 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 190493 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190493&action=edit 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> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #32 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Just an observation: members/memberentry.pl: push @errors, "ERROR_guarantor_is_guarantee"; if ( $_->invalid_guarantor ) { push @errors, "ERROR_guarantor_is_guarantee"; $template->param( guarantor_is_guarantee => $_->guarantor ); => Does this code not beg for renaming invalid_guarantor to something like guarantor_is_guarantee ? Koha/Exceptions/Patron/Relationship.pm: } elsif ( $self->invalid_guarantor ) { } elsif ( $self->invalid_guarantor ) { $msg = sprintf("Guarantee patron cannot be a guarantor."); => [SAME] Does this code not beg for renaming invalid_guarantor to something like guarantor_is_guarantee ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190493|0 |1 is obsolete| | --- Comment #34 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 190767 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190767&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190767|0 |1 is obsolete| | --- Comment #35 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 190768 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190768&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #37 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 190770 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190770&action=edit Bug 39014: (QA follow-up) Fix tidiness -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #39 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Thanks for following up so fast, Lari. I am going to set this to FQA, we don't tidy commits. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #190769|0 |1 is obsolete| | --- Comment #42 from Lari Taskula <lari.taskula@hypernova.fi> --- Created attachment 190773 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=190773&action=edit Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by To test: 1. prove t/db_dependent/Koha/Patron.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This fixes an issue when release notes| |creating a patron that | |requires a guarantor, with | |this combination of | |settings: | |- | |`TrackLastPatronActivityTri | |ggers`: 'Creating a patron' | |is selected | |- `ChildNeedsGuarantor`: is | |set to 'requires' | |- The patron category is a | |'Child' and 'Can be | |guarantee' is set to 'Yes' | | | |With these settings, an | |error message "The | |following fields are wrong. | |Please fix them." (without | |a list of fields) was | |incorrectly shown when | |creating a child patron, | |even though you had | |correctly added the | |guarantor information. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lari Taskula <lari.taskula@hypernova.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=39180 --- Comment #46 from Lari Taskula <lari.taskula@hypernova.fi> --- Hi, the above tests were also added fairly recently by Bug 39180 and it looks like these two Bugs have had some overlapping that was left unnoticed. After Emmi's patch we no longer throw those particular exceptions, so the tests expecting them (in the form of warnings) will fail. I suppose we could just remove those tests, unless we explicitly want to notify librarians of invalid patron relationships during circulation. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #48 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- I added a patch to remove the tests in question. I'm not convinced this is correct. Do we want to add a warn somewhere that can be logged? For troubleshooting/support? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #50 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- follow-up pushed to main -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #52 from Lari Taskula <lari.taskula@hypernova.fi> --- (In reply to Nick Clemens (kidclamp) from comment #51)

In the new catch, we specifically handle two types of exceptions, but we don't have an else case, so if another exception is thrown, it's just going to be ignored. The other exceptions are handled in the follow-up patch.

I kind of think if we're throwing an exception, we should always log it. The librarian may see the alert, and then dismiss it, without resolving the problem, having it in the logs ensures it can be found later. I would actually lean towards making all exceptions log, every time - if we think it warrants an exception, I think it warrants logging It would be first useful to test the amount of data that would be logged in a production environment if we had this enabled.

Personally I don't think we should do it by default. Every unhandled exception must be logged, but as for caught exceptions, I'd leave it up for the developer to decide. In my opinion not every exception we have is always logworthy, for example failing to meet minimum password requirements when setting a password (in many cases we have frontend validation to avoid a HTTP request/an exception, but there are also REST API clients that just pass Koha whatever input was given by the user). But then again someone else could find that useful. Maybe a patron calls the library and the library has so awesome customer support that the system administrator checks exception logs and finds that their password contained a whitespace (Koha::Exceptions::Password::WhitespaceCharacters logged). If we did something like that, the log level should probably be at most to 'trace'. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39014 --- Comment #53 from Jacob O'Mara <jacob.omara@openfifth.co.uk> --- Thanks all, pushed to 25.11.x -- You are receiving this mail because: You are watching all bug changes.