Bug 39014 - Storing a guarantee fails due to TrackLastPatronActivityTriggers "creating a patron"
Summary: Storing a guarantee fails due to TrackLastPatronActivityTriggers "creating a ...
Status: Pushed to main
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: 2026-01-12 21:36 UTC (History)
13 users (show)

See Also:
GIT URL:
Initiative type: ---
Sponsorship status: ---
Comma delimited list of Sponsors:
Crowdfunding goal: 0
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes an issue when creating a patron that requires a guarantor, with this combination of settings: - `TrackLastPatronActivityTriggers`: '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.
Version(s) released in:
26.05.00
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
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by (12.49 KB, patch)
2025-10-16 22:04 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.81 KB, patch)
2025-10-27 06:27 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by (12.48 KB, patch)
2025-10-27 06:28 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.80 KB, patch)
2025-10-27 08:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by (12.52 KB, patch)
2025-10-27 08:34 UTC, David Nind
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.90 KB, patch)
2025-12-15 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by (12.62 KB, patch)
2025-12-15 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.81 KB, patch)
2025-12-29 18:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.81 KB, patch)
2025-12-29 18:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by (12.59 KB, patch)
2025-12-29 18:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: (QA follow-up) Fix tidiness (5.40 KB, patch)
2025-12-29 18:42 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: Add new method validate_guarantor (15.81 KB, patch)
2025-12-30 12:14 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: (follow-up) Refactoring and renaming to can_be_guaranteed_by (12.86 KB, patch)
2025-12-30 12:14 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 39014: Remove unnecessary tests (2.79 KB, patch)
2026-01-07 20:38 UTC, Lucas Gass (lukeg)
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.
Comment 16 Emmi Takkinen 2025-09-15 10:24:16 UTC
(In reply to Marcel de Rooy from comment #15)
> 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 ?
I don't quite get what you mean with this, could you explain this some more? 
> =head3 validate_guarantor
>     Koha::Patron->validate_guarantor(\@guarantors, $contactname, $category );
>     Validates guarantor patron.
> =cut
> Could you elaborate "validate guarantor patron"? Singular ?
You're right, method should be named validate_guarantors and description of the method should be more descriptive.
> 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";
>     }
> }
Hmm, I guess these should be two separate methods then? One for validating guarantors and one for checking if patron needs a 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.
Did this happen while adding patron from Koha or while running tests? Could you provide a plan how to reproduce this?
Comment 17 Marcel de Rooy 2025-09-15 10:35:59 UTC
(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 :)
Comment 18 Emmi Takkinen 2025-09-15 11:15:55 UTC
(In reply to Marcel de Rooy from comment #17)
> (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 :)

Ah, this happens with guarantors. There's definitely something wrong with logic in code. Good catch!
Comment 19 Lari Taskula 2025-10-16 22:04:37 UTC
Created attachment 188041 [details] [review]
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by

To test:
1. prove t/db_dependent/Koha/Patron.t
Comment 20 Lari Taskula 2025-10-16 22:30:31 UTC
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
Comment 21 David Nind 2025-10-26 23:36:13 UTC
The patch no longer applies 8-(...


Applying: Bug 39014: Add new method validate_guarantor
Using index info to reconstruct a base tree...
M	Koha/Exceptions/Patron/Relationship.pm
M	Koha/Patron.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
M	members/memberentry.pl
M	t/db_dependent/Koha/Patron.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patron.t
Auto-merging members/memberentry.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
Auto-merging Koha/Exceptions/Patron/Relationship.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 39014: Add new method validate_guarantor
Comment 22 Emmi Takkinen 2025-10-27 06:27:43 UTC
Created attachment 188461 [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 23 Emmi Takkinen 2025-10-27 06:28:10 UTC
Created attachment 188462 [details] [review]
Bug 39014: (follow-up) Refacotring and renaming to can_be_guaranteed_by

To test:
1. prove t/db_dependent/Koha/Patron.t
Comment 24 Emmi Takkinen 2025-10-27 06:28:22 UTC
Rebased patches.
Comment 25 David Nind 2025-10-27 08:34:38 UTC
Created attachment 188463 [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 26 David Nind 2025-10-27 08:34:43 UTC
Created attachment 188464 [details] [review]
Bug 39014: (follow-up) Refacotring 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>
Comment 27 David Nind 2025-10-27 08:35:27 UTC
(In reply to Emmi Takkinen from comment #24)
> Rebased patches.

Thanks Emmi!
Comment 28 Marcel de Rooy 2025-12-12 10:20:29 UTC
Looking here
Comment 29 Marcel de Rooy 2025-12-12 10:58:31 UTC Comment hidden (obsolete)
Comment 30 Marcel de Rooy 2025-12-15 10:35:27 UTC
Created attachment 190493 [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>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2025-12-15 10:35:30 UTC
Created attachment 190494 [details] [review]
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>
Comment 32 Marcel de Rooy 2025-12-15 10:39:12 UTC
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 ?
Comment 33 Lucas Gass (lukeg) 2025-12-29 16:22:51 UTC
[FAIL] Koha/Exceptions/Patron/Relationship.pm
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl Koha/Exceptions/Patron/Relationship.pm`
[PASS] Koha/Patron.pm
[FAIL] koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt`
[FAIL] members/memberentry.pl
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl members/memberentry.pl`
[FAIL] t/db_dependent/Koha/Patron.t
   FAIL	  tidiness
		File is not tidy, please run `perl misc/devel/tidy.pl t/db_dependent/Koha/Patron.t`
Comment 34 Lari Taskula 2025-12-29 18:39:16 UTC Comment hidden (obsolete)
Comment 35 Lari Taskula 2025-12-29 18:42:08 UTC Comment hidden (obsolete)
Comment 36 Lari Taskula 2025-12-29 18:42:11 UTC Comment hidden (obsolete)
Comment 37 Lari Taskula 2025-12-29 18:42:14 UTC
Created attachment 190770 [details] [review]
Bug 39014: (QA follow-up) Fix tidiness
Comment 38 Tomás Cohen Arazi (tcohen) 2025-12-29 19:10:59 UTC
(In reply to Lari Taskula from comment #37)
> Created attachment 190770 [details] [review] [review]
> Bug 39014: (QA follow-up) Fix tidiness

You folks should be using a KTD instance in the background at least so the commits are tidy automatically!
Comment 39 Lucas Gass (lukeg) 2025-12-29 22:59:39 UTC
Thanks for following up so fast, Lari. I am going to set this to FQA, we don't tidy commits.
Comment 40 Lucas Gass (lukeg) 2025-12-29 23:01:17 UTC
(In reply to Lucas Gass (lukeg) from comment #39)
> Thanks for following up so fast, Lari. I am going to set this to FQA, we
> don't tidy commits.

We don't want to have tidy commits. Can you squash that patch?
Comment 41 Lari Taskula 2025-12-30 12:14:25 UTC
Created attachment 190772 [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 42 Lari Taskula 2025-12-30 12:14:28 UTC
Created attachment 190773 [details] [review]
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>
Comment 43 Lari Taskula 2025-12-30 12:16:54 UTC
(In reply to Lucas Gass (lukeg) from comment #39)
> Thanks for following up so fast, Lari. I am going to set this to FQA, we
> don't tidy commits.
Squasehd. I've set this back to passed QA :)
Comment 44 Lucas Gass (lukeg) 2025-12-30 14:51:28 UTC
Nice work everyone!

Pushed to main for 26.05
Comment 45 Lucas Gass (lukeg) 2025-12-31 14:14:36 UTC
Tests are failing now:

    #   Failed test 'AddReturn generates a warning when can_be_guarantee type patron is missing a guarantor and ChildNeedsGuarantor'
    #   at /kohadevbox/koha/t/db_dependent/Circulation.t line 7787.
    # didn't find a warning
    # expected to find warning: (?^u:Problem updating lastseen)
    ok 7 - AddIssue generates no warning when Child type patron is missing a guarantor and not ChildsNeedsGuarantor
    ok 8 - AddRenewal generates no warning when Child type patron is missing a guarantor and not ChildNeedsGuarantor
    ok 9 - AddReturn generates no warning when Child type patron is missing a guarantor and not ChildNeedsGuarantor
    ok 10 - AddIssue generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildsNeedsGuarantor
    ok 11 - AddRenewal generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildNeedsGuarantor
    ok 12 - AddReturn generates no warning when can_be_guarantee type patron is missing a guarantor and not ChildNeedsGuarantor
    ok 13 - AddIssue generates no warning when Child type patron has a guarantor and ChildsNeedsGuarantor
    ok 14 - AddRenewal generates no warning when Child type patron has a guarantor and not ChildNeedsGuarantor
    ok 15 - AddReturn generates no warning when Child type patron has a guarantor and not ChildNeedsGuarantor
    ok 16 - AddIssue generates no warning when can_be_guarantee type patron has a guarantor and not ChildsNeedsGuarantor
    ok 17 - AddRenewal generates no warning when can_be_guarantee type patron has a guarantor and not ChildNeedsGuarantor
    ok 18 - AddReturn generates no warning when can_be_guarantee type patron has a guarantor and not ChildNeedsGuarantor
    # Looks like you failed 6 tests of 18.
not ok 79 - ChildNeedsGuarantor



Please follow-up
Comment 46 Lari Taskula 2026-01-05 20:13:23 UTC
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.
Comment 47 Lucas Gass (lukeg) 2026-01-07 20:38:47 UTC
Created attachment 191014 [details] [review]
Bug 39014: Remove unnecessary tests
Comment 48 Lucas Gass (lukeg) 2026-01-07 20:40:19 UTC
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?
Comment 49 Lari Taskula 2026-01-09 12:14:49 UTC
(In reply to Lucas Gass (lukeg) from comment #48)
> 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?

I don't see much value in logging that, as misconfigured patron relationships no longer stop Koha from performing circulation actions. Having librarians alerted on the staff client is in my opinion sufficient. Librarians are notified of invalid patron relationships on the patron's "Check out" and "Details" tab, thanks to Bug 39180. As for the check-in screen, I suppose we have lost that ability.

(out of scope) It would also be useful to have a report that lists all the patrons with an invalid patron relationship/alerts them on the staff main page.
Comment 50 Lucas Gass (lukeg) 2026-01-09 13:43:51 UTC
follow-up pushed to main
Comment 51 Nick Clemens (kidclamp) 2026-01-09 14:35:28 UTC
(In reply to Lari Taskula from comment #49)
> (In reply to Lucas Gass (lukeg) from comment #48)
> > 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?
> 
> I don't see much value in logging that, as misconfigured patron
> relationships no longer stop Koha from performing circulation actions.
> Having librarians alerted on the staff client is in my opinion sufficient.
> Librarians are notified of invalid patron relationships on the patron's
> "Check out" and "Details" tab, thanks to Bug 39180. As for the check-in
> screen, I suppose we have lost that ability.
> 
> (out of scope) It would also be useful to have a report that lists all the
> patrons with an invalid patron relationship/alerts them on the staff main
> page.

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.

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
Comment 52 Lari Taskula 2026-01-12 21:36:47 UTC
(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'.