Bug 39014 - Storing a guarantee fails due to TrackLastPatronActivityTriggers "creating a patron"
Summary: Storing a guarantee fails due to TrackLastPatronActivityTriggers "creating a ...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Emmi Takkinen
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 23486
Blocks:
  Show dependency treegraph
 
Reported: 2025-02-03 07:32 UTC by Lari Strand
Modified: 2025-05-30 07:26 UTC (History)
10 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 39014: Add new method validate_guarantor (11.20 KB, patch)
2025-02-06 09:53 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (12.49 KB, patch)
2025-02-06 10:20 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (12.54 KB, patch)
2025-02-07 12:50 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.48 KB, patch)
2025-02-24 06:42 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.71 KB, patch)
2025-02-24 06:47 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.60 KB, patch)
2025-03-21 07:30 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.67 KB, patch)
2025-04-02 11:30 UTC, Magnus Enger
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.76 KB, patch)
2025-05-12 14:23 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Strand 2025-02-03 07:32:16 UTC
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.
Comment 1 Emmi Takkinen 2025-02-04 07:17:23 UTC
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]".
Comment 2 Emmi Takkinen 2025-02-05 07:36:39 UTC
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.
Comment 3 Emmi Takkinen 2025-02-06 09:53:28 UTC Comment hidden (obsolete)
Comment 4 Emmi Takkinen 2025-02-06 10:20:50 UTC Comment hidden (obsolete)
Comment 5 Jake Deery 2025-02-07 12:50:17 UTC Comment hidden (obsolete)
Comment 6 Jake Deery 2025-02-07 12:51:16 UTC
All works as intended, now! Signed off

Jake.
Comment 7 Emmi Takkinen 2025-02-24 06:42:17 UTC Comment hidden (obsolete)
Comment 8 Emmi Takkinen 2025-02-24 06:43:57 UTC
There were some things I forgot to handle in the original patch. Patch is now rebased against current main.
Comment 9 Emmi Takkinen 2025-02-24 06:47:52 UTC Comment hidden (obsolete)
Comment 10 Emmi Takkinen 2025-03-21 07:30:40 UTC Comment hidden (obsolete)
Comment 11 Magnus Enger 2025-04-02 11:30:56 UTC
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.
Comment 12 David Nind 2025-05-12 14:23:13 UTC
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>
Comment 13 David Nind 2025-05-12 14:24:12 UTC
Added my sign-off, in case Magnus's can count as QA
Comment 14 Marcel de Rooy 2025-05-30 06:51:48 UTC
Looking here
Comment 15 Marcel de Rooy 2025-05-30 07:26:48 UTC
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.