Bug 12133 - Guarantor requirements when registering a patron
Summary: Guarantor requirements when registering a patron
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 1 vote (vote)
Assignee: Emmi Takkinen
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 23687
  Show dependency treegraph
 
Reported: 2014-04-24 06:12 UTC by paxed
Modified: 2023-11-30 16:25 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement makes two changes to guarantors and guarantees when registering a new patron: - A child patron must have a guarantor - this is controlled by the new `ChildNeedsGuarantor` system preference. - A guarantor cannot be a guarantee.
Version(s) released in:
23.11.00


Attachments
Add two requirements when registering a new patron (2.17 KB, patch)
2014-04-24 06:12 UTC, paxed
Details | Diff | Splinter Review
Guarantor requirements when registering a patron (5.89 KB, patch)
2014-04-25 09:41 UTC, paxed
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12133: Guarantor requirements when registering a patron (5.90 KB, patch)
2014-11-11 16:08 UTC, Pierre-Paul Paquin
Details | Diff | Splinter Review
Bug 12133: Guarantor requirements when registering a patron (6.79 KB, patch)
2019-07-12 10:54 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: (QA Follow up) Guarantor requirements when registering a patron (6.55 KB, patch)
2019-10-17 12:36 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (6.53 KB, patch)
2019-10-22 12:22 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (6.58 KB, patch)
2019-10-22 12:43 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 12133: Fix issue with modify (1.74 KB, patch)
2020-02-26 08:15 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Fix issues with modifying child patron and adding guarantors (5.30 KB, patch)
2020-11-05 13:05 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Enforce guarantor restrictions on Koha::Patron->store() (7.06 KB, patch)
2020-11-05 13:07 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (7.52 KB, patch)
2022-04-27 09:40 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Fix issues with modifying child patron and adding guarantors (5.26 KB, patch)
2022-04-27 09:40 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Enforce guarantor restrictions on Koha::Patron->store() (7.05 KB, patch)
2022-04-27 09:41 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (13.50 KB, patch)
2023-02-17 11:18 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (15.32 KB, patch)
2023-02-27 12:32 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (14.99 KB, patch)
2023-03-10 11:10 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (15.15 KB, patch)
2023-03-10 12:51 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (16.02 KB, patch)
2023-06-27 07:15 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (16.47 KB, patch)
2023-07-06 05:56 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (16.51 KB, patch)
2023-07-07 11:21 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (16.08 KB, patch)
2023-08-07 07:37 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (16.13 KB, patch)
2023-08-08 12:53 UTC, David Nind
Details | Diff | Splinter Review
Bug 12133: Fix QA issue (834 bytes, patch)
2023-09-29 12:21 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: Add requirements for guarantor and guarantee (15.99 KB, patch)
2023-10-01 17:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 12133: Fix QA issue (880 bytes, patch)
2023-10-01 17:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 12133: Fix QA issue (938 bytes, patch)
2023-11-03 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12133: (QA follow-up) Fix number of tests (708 bytes, patch)
2023-11-03 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12133: (QA follow-up) Tidy for qa script (19.09 KB, patch)
2023-11-03 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12133: Fix failing tests (3.02 KB, patch)
2023-11-14 12:41 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12133: (follow-up) Fix failing tests (1.06 KB, patch)
2023-11-15 11:12 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2014-04-24 06:12:28 UTC Comment hidden (obsolete)
Comment 1 Owen Leonard 2014-04-24 12:37:21 UTC
(In reply to paxed from comment #0)

> so perhaps a syspref to control one, or both,
> might be in order?

My library would definitely want a system preference to control the first option (child must have a guarantor). The second option seems like simple logic to me, but I've learned not to trust my sense of logic in these matters.
Comment 2 paxed 2014-04-25 09:41:50 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2014-04-25 16:37:12 UTC
Please provide a test plan.
Comment 4 paxed 2014-04-29 06:18:03 UTC
To test:

1) Add a new child patron. It will be successful without a guarantor.
2) Add a new child patron, with guarantor set to some other patron who already has a guarantor. Adding the child patron is successful.
3) Add a new non-child patron, with guarantor set to some other patron who already has a guarantor. Adding the patron is successful.
4) Install patch
5) Add a new child patron. It will be successful without a guarantor.
6) Add a new child patron, with guarantor set to some other patron who already has a guarantor. Adding the child patron should fail with error message.
7) Add a new non-child patron, with guarantor set to some other patron who already has a guarantor. Adding the patron should fail with error message.
8) Set syspref ChildNeedsGuarantor to ON
9) Try to add a new child patron without a guarantor. It should fail with error message.
10) Add a new non-child patron. It will be successful without a guarantor.
11) Set syspref ChildNeedsGuarantor to OFF
12) Repeat the steps 5-10, but instead of adding a patron, edit already existing patron.
Comment 5 Pierre-Paul Paquin 2014-11-11 16:08:17 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2014-12-19 15:03:47 UTC
On master, you can set a firstname+surname as guarantor. You are not forced to choose an existing patron.
With you patch, pref enabled, it's not possible to do that.
Worse, if you try to edit a patron without guarantorid, you are not allowed to save any changes.

Marked as Failed QA.
Comment 7 Emmi Takkinen 2019-07-12 10:54:02 UTC Comment hidden (obsolete)
Comment 8 Mwoo 2019-09-21 16:46:28 UTC Comment hidden (obsolete)
Comment 9 Emmi Takkinen 2019-09-27 08:24:08 UTC
That's due changes made in bug 14570 which also makes changes proposed in this bugs. Marking this as resolved.
Comment 10 Emmi Takkinen 2019-09-27 09:40:08 UTC
Spoke too soon, syspref to require a guarantor for a child is still needed :D
Comment 11 Emmi Takkinen 2019-10-17 12:36:54 UTC Comment hidden (obsolete)
Comment 12 Emmi Takkinen 2019-10-22 12:22:20 UTC Comment hidden (obsolete)
Comment 13 Johanna Räisä 2019-10-22 12:43:42 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2020-01-22 10:29:38 UTC
I think this new restriction needs to be enforced at module level, when the patron is added/updated (Koha::Patron->store), with changes covered by tests.
Comment 15 Emmi Takkinen 2020-02-26 08:15:31 UTC Comment hidden (obsolete)
Comment 16 Emmi Takkinen 2020-02-26 08:27:23 UTC
(In reply to Jonathan Druart from comment #14)
> I think this new restriction needs to be enforced at module level, when the
> patron is added/updated (Koha::Patron->store), with changes covered by test

You mean "guarantor can't be guarantee"?
Comment 17 Jonathan Druart 2020-02-26 09:05:56 UTC
(In reply to Emmi Takkinen from comment #16)
> (In reply to Jonathan Druart from comment #14)
> > I think this new restriction needs to be enforced at module level, when the
> > patron is added/updated (Koha::Patron->store), with changes covered by test
> 
> You mean "guarantor can't be guarantee"?

And "child patron must have a guarantor".
Comment 18 Emmi Takkinen 2020-11-05 13:05:53 UTC Comment hidden (obsolete)
Comment 19 Emmi Takkinen 2020-11-05 13:06:33 UTC
Finally had time to look at this again and noticed that the latest patch didn't take into account possible multiple guarantor ids. It fixed issue with modifying but one could add child patron as guarantor. Since these stem up from same a problem, missing guarantor ids, I obsoleted the latest patch and provided a new one which fixes both of these issues.
Comment 20 Emmi Takkinen 2020-11-05 13:07:53 UTC Comment hidden (obsolete)
Comment 21 Emmi Takkinen 2022-04-27 09:40:15 UTC Comment hidden (obsolete)
Comment 22 Emmi Takkinen 2022-04-27 09:40:45 UTC Comment hidden (obsolete)
Comment 23 Emmi Takkinen 2022-04-27 09:41:17 UTC Comment hidden (obsolete)
Comment 24 solene.ngamga 2023-01-13 20:53:49 UTC
Bug 12133 - Guarantor requirements when registering a patron

134013 - Bug 12133: Add requirements for guarantor and guarantee
134014 - Bug 12133: Fix issues with modifying child patron and adding guarantors
134015 - Bug 12133: Enforce guarantor restrictions on Koha::Patron->store()

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 12133: Add requirements for guarantor and guarantee
Using index info to reconstruct a base tree...
M	koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
M	members/memberentry.pl
Falling back to patching base and 3-way merge...
Auto-merging members/memberentry.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/patrons.pref
error: Failed to merge in the changes.
Patch failed at 0001 Bug 12133: Add requirements for guarantor and guarantee
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-12133-Add-requirements-for-guarantor-and-guara-ofLCAA.patch
Comment 25 Emmi Takkinen 2023-02-17 11:18:00 UTC Comment hidden (obsolete)
Comment 26 Emmi Takkinen 2023-02-17 11:19:12 UTC
Combined and rebased patches into one patch. They became a nightmare to handle :D
Comment 27 Emmi Takkinen 2023-02-27 12:32:39 UTC Comment hidden (obsolete)
Comment 28 Emmi Takkinen 2023-03-10 11:10:28 UTC Comment hidden (obsolete)
Comment 29 Emmi Takkinen 2023-03-10 12:51:41 UTC Comment hidden (obsolete)
Comment 30 Emmi Takkinen 2023-03-16 07:07:12 UTC
There's still some work to do with this. If one deletes patron information and tries to save patron, error "Child patron needs a guarantor" is lifted as it should be. But if patron modification is canceled after deleting the guarantor, all guarantor info is lost because there is no check if patron would end up without guarantor due delete.
Comment 31 Emmi Takkinen 2023-06-27 07:15:33 UTC Comment hidden (obsolete)
Comment 32 Sam Lau 2023-07-05 19:20:31 UTC
Everything in test plan worked fine for me, just failing t/db_dependent/Koha/Patron.t. 

# Subtest: guarantor requirements tests
    1..6
    ok 1 - Child patron can be stored without guarantor when ChildNeedsGuarantor is off.
    ok 2 - Exception thrown when guarantor is required but not provided.
    ok 3 - Exception thrown when child patron is added as guarantor.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0xaaab1c4b5b58)' for column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 170
    # Looks like you planned 6 tests but ran 3.
not ok 25 - guarantor requirements tests

#   Failed test 'guarantor requirements tests'
#   at t/db_dependent/Koha/Patron.t line 1588.
Invalid value passed, borrowers.updated_on=SCALAR(0xaaab1c4b5b58) expected type is datetime# Looks like your test exited with 255 just after 25.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/25 subtests
Comment 33 Emmi Takkinen 2023-07-06 05:54:56 UTC
(In reply to Sam Lau from comment #32)
> Everything in test plan worked fine for me, just failing
> t/db_dependent/Koha/Patron.t. 
> 
> # Subtest: guarantor requirements tests
>     1..6
>     ok 1 - Child patron can be stored without guarantor when
> ChildNeedsGuarantor is off.
>     ok 2 - Exception thrown when guarantor is required but not provided.
>     ok 3 - Exception thrown when child patron is added as guarantor.
> DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st
> execute failed: Incorrect datetime value: 'SCALAR(0xaaab1c4b5b58)' for
> column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at
> /kohadevbox/koha/Koha/Object.pm line 170
>     # Looks like you planned 6 tests but ran 3.
> not ok 25 - guarantor requirements tests
> 
> #   Failed test 'guarantor requirements tests'
> #   at t/db_dependent/Koha/Patron.t line 1588.
> Invalid value passed, borrowers.updated_on=SCALAR(0xaaab1c4b5b58) expected
> type is datetime# Looks like your test exited with 255 just after 25.
> Dubious, test returned 255 (wstat 65280, 0xff00)
> Failed 1/25 subtests

Hmm, I'm unable to repeat this. Did you test this with current master? However for some reason 'guarantor requirements tests' fail for me randomly with error "Child patron cannot be a guarantor." This probably happens because $child2 is first attempted to be stored with child as guarantor and then without it..? Adding $child2->discard_changes after first store seems to help.
Comment 34 Emmi Takkinen 2023-07-06 05:56:26 UTC
Created attachment 153082 [details] [review]
Bug 12133: Add requirements for guarantor and guarantee

Add two requirements when registering a new patron:

- A child patron must have a guarantor. This is controlled by
  a new syspref ChildNeedsGuarantor.
- A guarantor cannot be a guarantee.

Test plan:
1. Add a child patron without guarantor or child patron with guarantee
as guarantor succesfully.
2. Apply this patch.
3. Add a child patron as a guarantor.
=> Error is raised.
4. Turn syspref "ChildNeedsGuarantor" ON.
5. Add a child patron without a guarantor and error "Child needs a
guarantor" is raised.
6. Add guarantor. Guarantor can either be existing patron or added with
"Contact" section.
=> Save without errors.

Also prove t/db_dependent/Koha/Patron.t

Sponsored-by: Koha-Suomi Oy
Comment 35 Emmi Takkinen 2023-07-06 06:01:59 UTC
I'm setting status back to "Needs Signoff" since Im' unable to repeat Sams problem with tests. However if it recurs then I'll look again.
Comment 36 Lucas Gass 2023-07-06 15:54:56 UTC
Emmi,

When I apply this patch, run the database update, and restart services the tests fail the same as what Sam notes:

# Subtest: guarantor requirements tests
    1..6
    ok 1 - Child patron can be stored without guarantor when ChildNeedsGuarantor is off.
    ok 2 - Exception thrown when guarantor is required but not provided.
    ok 3 - Exception thrown when child patron is added as guarantor.
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Incorrect datetime value: 'SCALAR(0x558a5143c3d8)' for column `koha_kohadev`.`borrowers`.`updated_on` at row 1 at /kohadevbox/koha/Koha/Object.pm line 170
    # Looks like you planned 6 tests but ran 3.
not ok 25 - guarantor requirements tests

#   Failed test 'guarantor requirements tests'
#   at t/db_dependent/Koha/Patron.t line 1589.
Invalid value passed, borrowers.updated_on=SCALAR(0x558a5143c3d8) expected type is datetime# Looks like your test exited with 255 just after 25.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/25 subtests
Comment 37 Emmi Takkinen 2023-07-07 06:05:40 UTC
Okay, now I got the same error. I'll take a look ->
Comment 38 Emmi Takkinen 2023-07-07 11:19:31 UTC
Okay, "Child cannot be guarantor" error happens because $patron_category needs "can_be_guarantee" value set as 0. 

"Invalid value passed, borrowers.updated_on=SCALAR(0x558a5143c3d8) expected type is datetime#" happens because calling store for $child2 a.k.a modding patron sends "updated_on" as a scalar. This can be solved with calling discard_changes() after first time calling $child2->store succesfully.
Comment 39 Emmi Takkinen 2023-07-07 11:21:28 UTC
Created attachment 153154 [details] [review]
Bug 12133: Add requirements for guarantor and guarantee

Add two requirements when registering a new patron:

- A child patron must have a guarantor. This is controlled by
  a new syspref ChildNeedsGuarantor.
- A guarantor cannot be a guarantee.

Test plan:
1. Add a child patron without guarantor or child patron with guarantee
as guarantor succesfully.
2. Apply this patch.
3. Add a child patron as a guarantor.
=> Error is raised.
4. Turn syspref "ChildNeedsGuarantor" ON.
5. Add a child patron without a guarantor and error "Child needs a
guarantor" is raised.
6. Add guarantor. Guarantor can either be existing patron or added with
"Contact" section.
=> Save without errors.

Also prove t/db_dependent/Koha/Patron.t

Sponsored-by: Koha-Suomi Oy
Comment 40 Sam Lau 2023-07-07 14:44:25 UTC
I am still failing subtest 'safe_to_delete() tests'. The only thing that changed in that subtest was that you commented out this line: 

$patron->account->pay({ amount => 10, debits => [ $debit ] });

Uncommenting it made all tests successful for me
Comment 41 Emmi Takkinen 2023-08-07 07:01:40 UTC
(In reply to Sam Lau from comment #40)
> I am still failing subtest 'safe_to_delete() tests'. The only thing that
> changed in that subtest was that you commented out this line: 
> 
> $patron->account->pay({ amount => 10, debits => [ $debit ] });
> 
> Uncommenting it made all tests successful for me

Ouch, that comment was there to make tests pass for me and I forgot to remove it. They failed because of that line, but that wasn't in scope of this bug, maybe it's now fixed.
Comment 42 Emmi Takkinen 2023-08-07 07:37:19 UTC
Created attachment 154300 [details] [review]
Bug 12133: Add requirements for guarantor and guarantee

Add two requirements when registering a new patron:

- A child patron must have a guarantor. This is controlled by
  a new syspref ChildNeedsGuarantor.
- A guarantor cannot be a guarantee.

Test plan:
1. Add a child patron without guarantor or child patron with guarantee
as guarantor succesfully.
2. Apply this patch.
3. Add a child patron as a guarantor.
=> Error is raised.
4. Turn syspref "ChildNeedsGuarantor" ON.
5. Add a child patron without a guarantor and error "Child needs a
guarantor" is raised.
6. Add guarantor. Guarantor can either be existing patron or added with
"Contact" section.
=> Save without errors.

Also prove t/db_dependent/Koha/Patron.t

Sponsored-by: Koha-Suomi Oy
Comment 43 Emmi Takkinen 2023-08-07 07:37:51 UTC
I deleted the unnecessary comment, tests should now pass.
Comment 44 David Nind 2023-08-08 12:53:48 UTC
Created attachment 154316 [details] [review]
Bug 12133: Add requirements for guarantor and guarantee

Add two requirements when registering a new patron:

- A child patron must have a guarantor. This is controlled by
  a new syspref ChildNeedsGuarantor.
- A guarantor cannot be a guarantee.

Test plan:
1. Add a child patron without guarantor or child patron with guarantee
as guarantor succesfully.
2. Apply this patch.
3. Add a child patron as a guarantor.
=> Error is raised.
4. Turn syspref "ChildNeedsGuarantor" ON.
5. Add a child patron without a guarantor and error "Child needs a
guarantor" is raised.
6. Add guarantor. Guarantor can either be existing patron or added with
"Contact" section.
=> Save without errors.

Also prove t/db_dependent/Koha/Patron.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2023-08-08 12:54:40 UTC
Testing notes (using KTD):

1. After apply the patch, run updatedatabase
Comment 46 Marcel de Rooy 2023-09-29 09:44:16 UTC
 FAIL   members/memberentry.pl
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
Comment 47 Emmi Takkinen 2023-09-29 11:56:56 UTC
(In reply to Marcel de Rooy from comment #46)
>  FAIL   members/memberentry.pl
>    FAIL   critic
>                 # Variables::ProhibitConditionalDeclarations: Got 1
> violation(s).

Any ideas where that comes from?
Comment 48 Jonathan Druart 2023-09-29 12:07:16 UTC
(In reply to Emmi Takkinen from comment #47)
> (In reply to Marcel de Rooy from comment #46)
> >  FAIL   members/memberentry.pl
> >    FAIL   critic
> >                 # Variables::ProhibitConditionalDeclarations: Got 1
> > violation(s).
> 
> Any ideas where that comes from?

perlcritic. You should be able to recreate easily with `perlcritic members/memberentry.pl` it will give you more info.
Comment 49 Emmi Takkinen 2023-09-29 12:21:34 UTC
Created attachment 156387 [details] [review]
Bug 12133: Fix QA issue
Comment 50 David Nind 2023-10-01 17:26:19 UTC
Created attachment 156446 [details] [review]
Bug 12133: Add requirements for guarantor and guarantee

Add two requirements when registering a new patron:

- A child patron must have a guarantor. This is controlled by
  a new syspref ChildNeedsGuarantor.
- A guarantor cannot be a guarantee.

Test plan:
1. Add a child patron without guarantor or child patron with guarantee
as guarantor succesfully.
2. Apply this patch.
3. Add a child patron as a guarantor.
=> Error is raised.
4. Turn syspref "ChildNeedsGuarantor" ON.
5. Add a child patron without a guarantor and error "Child needs a
guarantor" is raised.
6. Add guarantor. Guarantor can either be existing patron or added with
"Contact" section.
=> Save without errors.

Also prove t/db_dependent/Koha/Patron.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Nind <david@davidnind.com>
Comment 51 David Nind 2023-10-01 17:26:23 UTC
Created attachment 156447 [details] [review]
Bug 12133: Fix QA issue

Signed-off-by: David Nind <david@davidnind.com>
Comment 52 Kyle M Hall 2023-11-03 15:45:13 UTC
Created attachment 158344 [details] [review]
Bug 12133: Fix QA issue

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Kyle M Hall 2023-11-03 15:45:20 UTC
Created attachment 158345 [details] [review]
Bug 12133: (QA follow-up) Fix number of tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 54 Kyle M Hall 2023-11-03 15:45:23 UTC
Created attachment 158346 [details] [review]
Bug 12133: (QA follow-up) Tidy for qa script

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 55 Emmi Takkinen 2023-11-06 06:39:36 UTC
Thank you Kyle for Passed QA! But it seems you signed off only part of patches and obsoleted the main patch.
Comment 56 Katrin Fischer 2023-11-06 07:13:43 UTC
Restored the patch, but might be better if Kyle uploaded all again.
Comment 57 Tomás Cohen Arazi 2023-11-10 14:02:07 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 58 Fridolin Somers 2023-11-11 08:06:04 UTC
Enhancement not pushed to 23.05.x
Comment 59 Jonathan Druart 2023-11-14 08:59:22 UTC
Tests are failing

koha_1       | #   Failed test 'No tests run for subtest "list() tests"'
koha_1       | #   at t/db_dependent/api/v1/erm_users.t line 92.
koha_1       | Child patron cannot be a guarantor.# Looks like your test exited with 2 just after 1.
koha_1       | [08:24:56] t/db_dependent/api/v1/erm_users.t

koha_1       | #   Failed test 'Test guarantor handling in SendQueuedMessages'
koha_1       | #   at t/db_dependent/Letters.t line 1163.
koha_1       | Child patron cannot be a guarantor.# Looks like your test exited with 11 just after 95.
koha_1       | [08:23:02] t/db_dependent/Letters.t 

Please fix ASAP!
Comment 60 Emmi Takkinen 2023-11-14 11:47:28 UTC

(In reply to Jonathan Druart from comment #59)
> Tests are failing
> 
> koha_1       | #   Failed test 'No tests run for subtest "list() tests"'
> koha_1       | #   at t/db_dependent/api/v1/erm_users.t line 92.
> koha_1       | Child patron cannot be a guarantor.# Looks like your test
> exited with 2 just after 1.
> koha_1       | [08:24:56] t/db_dependent/api/v1/erm_users.t
> 
> koha_1       | #   Failed test 'Test guarantor handling in
> SendQueuedMessages'
> koha_1       | #   at t/db_dependent/Letters.t line 1163.
> koha_1       | Child patron cannot be a guarantor.# Looks like your test
> exited with 11 just after 95.
> koha_1       | [08:23:02] t/db_dependent/Letters.t 
> 
> Please fix ASAP!

Fixed tests, but should I create new bug or add patch here?
Comment 61 Jonathan Druart 2023-11-14 12:39:47 UTC
Please attach here.
Comment 62 Emmi Takkinen 2023-11-14 12:41:38 UTC
Created attachment 158919 [details] [review]
Bug 12133: Fix failing tests

Tests in erm_users.t and Letters.t failed because when
we mock patrons they receive category where setting
can_be_guarantee is not 0. This triggers error "Child
patron cannot be a guarantor". We also need to mock
syspref "ChildNeedsGuarantor" accordingly.

To test prove t/db_dependent/api/v1/erm_users.t and
t/db_dependent/Letters.t.

Sponsored-by: Koha-Suomi Oy
Comment 63 Tomás Cohen Arazi 2023-11-14 13:28:14 UTC
(In reply to Emmi Takkinen from comment #62)
> Created attachment 158919 [details] [review] [review]
> Bug 12133: Fix failing tests
> 
> Tests in erm_users.t and Letters.t failed because when
> we mock patrons they receive category where setting
> can_be_guarantee is not 0. This triggers error "Child
> patron cannot be a guarantor". We also need to mock
> syspref "ChildNeedsGuarantor" accordingly.
> 
> To test prove t/db_dependent/api/v1/erm_users.t and
> t/db_dependent/Letters.t.
> 
> Sponsored-by: Koha-Suomi Oy

Ha, just had written almost the same patch.
Comment 64 Tomás Cohen Arazi 2023-11-14 13:28:34 UTC
Follow-up pushed, thanks.
Comment 65 Jonathan Druart 2023-11-15 11:03:21 UTC
I don't understand why this has been designed in a so limited way.

Why only children?

This should have been a patron's category flag, or the syspref should accept a list of patron's categories.

At some point only children were allowed to have guarantor, but since bug 12446 this limitation should not persist.
Comment 66 Jonathan Druart 2023-11-15 11:10:48 UTC
The test is still failing

koha_1       | #   Failed test 'No tests run for subtest "Test where parameter for SendQueuedMessages"'
koha_1       | #   at t/db_dependent/Letters.t line 1274.
koha_1       | [Child patron needs a guarantor]# Looks like your test exited with 11 just after 97.
koha_1       | [19:28:21] t/db_dependent/Letters.t .................................................... 
koha_1       | Dubious, test returned 11 (wstat 2816, 0xb00)
Comment 67 Jonathan Druart 2023-11-15 11:12:32 UTC
Created attachment 158979 [details] [review]
Bug 12133: (follow-up) Fix failing tests

Tests were still failing

 #   Failed test 'No tests run for subtest "Test where parameter for SendQueuedMessages"'
 #   at t/db_dependent/Letters.t line 1274.
 [Child patron needs a guarantor]# Looks like your test exited with 11 just after 97.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 68 Jonathan Druart 2023-11-15 11:13:28 UTC
(In reply to Jonathan Druart from comment #67)
> Created attachment 158979 [details] [review] [review]
> Bug 12133: (follow-up) Fix failing tests
> 
> Tests were still failing
> 
>  #   Failed test 'No tests run for subtest "Test where parameter for
> SendQueuedMessages"'
>  #   at t/db_dependent/Letters.t line 1274.
>  [Child patron needs a guarantor]# Looks like your test exited with 11 just
> after 97.
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master.
Comment 69 Emmi Takkinen 2023-11-15 11:21:52 UTC
(In reply to Jonathan Druart from comment #65)
> I don't understand why this has been designed in a so limited way.
> 
> Why only children?
> 
> This should have been a patron's category flag, or the syspref should accept
> a list of patron's categories.
> 
> At some point only children were allowed to have guarantor, but since bug
> 12446 this limitation should not persist.

It does take into account if patrons category has setting "Can be guarantee" enabled. In Koha::Patron::store:

if (    C4::Context->preference('ChildNeedsGuarantor')
                    and ( $self->is_child or $self->category->can_be_guarantee )
                    and $self->contactname eq ""
                    and !@$guarantors )
                {
                    Koha::Exceptions::Patron::Relationship::NoGuarantor->throw();
}

But yeah, syspref and errors don't implicate that syspref has anything to do with other than child patrons.