Bug 39331 - Guarantor relationships not removed when changing patron category from memberentry.pl
Summary: Guarantor relationships not removed when changing patron category from member...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Lucas Gass (lukeg)
QA Contact: Baptiste Wojtkowski (bwoj)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-03-14 13:31 UTC by Nick Clemens (kidclamp)
Modified: 2025-03-21 16:51 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes changing a patron with guarantors from a patron category that allows guarantees to one that doesn't (for example, from Kid to a Patron). Currently, the guarantor relationships are kept (when they shouldn't be).
Version(s) released in:
25.05.00
Circulation function:


Attachments
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors (1.81 KB, patch)
2025-03-14 22:33 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors (1.81 KB, patch)
2025-03-14 22:37 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors (1.86 KB, patch)
2025-03-16 19:04 UTC, David Nind
Details | Diff | Splinter Review
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors (1.92 KB, patch)
2025-03-21 08:26 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2025-03-14 13:31:23 UTC
When editing a patron in the staff interface the guarantor relationships are being recreated after they are removed

The code calls Patron->store - this is where guarantors are removed when category changes - after this we call add_guarantors using the $input from the form

To recreate:
1 - Remove age limits from a Child and Adult category in Administration->Patron categories. Ensure Child category can be guarantor and Adult cannot
2 - The above step is to avoid any age checks and form resubmission which removes the guarantor fields
3 - Find/create a child patron
4 - Add a guarantee from the Adult category to the child
5 - Check the DB:
   SELECT * FROM borrower_relationships WHERE guarantee_id={borrowernumber of child};
6 - Note the id colum
7 - Edit the child patron to Adult category
8 - Repeat 5
9 - Note the relationship still exists and the id is incremented 
10 - Oof!
Comment 1 Lucas Gass (lukeg) 2025-03-14 21:13:53 UTC
Yup, everything in Koha::Patron->store looks correct but add_guarantors adds them back
Comment 2 Lucas Gass (lukeg) 2025-03-14 22:33:20 UTC
Created attachment 179360 [details] [review]
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors

To test:
1 - Remove age limits from a Child and Adult category in Administration->Patron categories. Ensure Child category can be guarantor and Adult cannot
2 - The above step is to avoid any age checks and form resubmission which removes the guarantor fields
3 - Find/create a child patron
4 - Add a guarantee from the Adult category to the child
5 - Check the DB:
   SELECT * FROM borrower_relationships WHERE guarantee_id={borrowernumber of child};
6 - Note the id colum
7 - Edit the child patron to Adult category
8 - Repeat 5
9 - Note the relationship still exists and the id is incremented
10 - Oof!
11 - APPLY PATCH, restart_all
12 - Try again steps 3 - 8
13 - The relationship should not be re-added
14 - Try with more than 1 guarantor on the record, confirming all are deleted
15 - Try adding some guarantor to patron types that should be able to, they relationship should work.
16 - Try changing some patrons to categories that do allow guarantors, making sure the relationship does stay.
Comment 3 Lucas Gass (lukeg) 2025-03-14 22:37:13 UTC
Created attachment 179361 [details] [review]
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors

To test:
1 - Remove age limits from a Child and Adult category in Administration->Patron categories. Ensure Child category can be guarantor and Adult cannot
2 - The above step is to avoid any age checks and form resubmission which removes the guarantor fields
3 - Find/create a child patron
4 - Add a guarantee from the Adult category to the child
5 - Check the DB:
   SELECT * FROM borrower_relationships WHERE guarantee_id={borrowernumber of child};
6 - Note the id colum
7 - Edit the child patron to Adult category
8 - Repeat 5
9 - Note the relationship still exists and the id is incremented
10 - Oof!
11 - APPLY PATCH, restart_all
12 - Try again steps 3 - 8
13 - The relationship should not be re-added
14 - Try with more than 1 guarantor on the record, confirming all are deleted
15 - Try adding some guarantor to patron types that should be able to, they relationship should work.
16 - Try changing some patrons to categories that do allow guarantors, making sure the relationship does stay.
Comment 4 David Nind 2025-03-16 19:04:59 UTC
Created attachment 179369 [details] [review]
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors

To test:
1 - Remove age limits from a Child and Adult category in Administration->Patron categories. Ensure Child category can be guarantor and Adult cannot
2 - The above step is to avoid any age checks and form resubmission which removes the guarantor fields
3 - Find/create a child patron
4 - Add a guarantee from the Adult category to the child
5 - Check the DB:
   SELECT * FROM borrower_relationships WHERE guarantee_id={borrowernumber of child};
6 - Note the id colum
7 - Edit the child patron to Adult category
8 - Repeat 5
9 - Note the relationship still exists and the id is incremented
10 - Oof!
11 - APPLY PATCH, restart_all
12 - Try again steps 3 - 8
13 - The relationship should not be re-added
14 - Try with more than 1 guarantor on the record, confirming all are deleted
15 - Try adding some guarantor to patron types that should be able to, they relationship should work.
16 - Try changing some patrons to categories that do allow guarantors, making sure the relationship does stay.

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2025-03-16 19:10:01 UTC
I've signed off, as the patch does what it says it does.

However, what is the expected behavour when changing a patron's category from one with a guarantor to one without?

Options:
1. Should it silently remove the guarantor? (This is what happens now after the patch, which I assume is the behavour expected before the bug was identified.)
2. Should it display a warning message when attempting to save the patron, asking you to confirm that the guarantor relationship(s) will be removed?
3. Should there be an information message after saving the changes to the patron, saying that the guarantor(s) was removed because the patron category was changed?

Or is this outside the scope of this bug? (That is, this bug should restore the current expected behavour (option 1)).

Testing notes (using KTD):
1. Go to Koha administration > Patrons and circulation > Patron categories.
2. For the Kid patron category (Code = K, Type = Child):
   - remove the values in these fields:
     . Age required
     . Upper age limit
   - check that the "Can be guarantee field" is set to "Yes"
3. For the Patron patron category (Code = PT, Type = Adult):
   - remove the values in these fields:
     . Age required
     . Upper age limit
   - check that the "Can be guarantee field" is set to "No"
4. Add a guarantee to a child patron:
   - Child patron: Lisa Charles (borrownumber = 37)
   - Guarantor: Edna Acosta (borrowernumber = 5)
5. Check the database:
   SELECT * FROM borrower_relationships WHERE guarantee_id=37;
   Result:
   +----+--------------+--------------+--------------+
   | id | guarantor_id | guarantee_id | relationship |
   +----+--------------+--------------+--------------+
   |  1 |            5 |           37 | mother       |
   +----+--------------+--------------+--------------+
6. Change Lisa Charles from a Kid patron category to a Patron patron category.
   Result: Still has a guarantor
7. Repeat step 5.
   Result:
   +----+--------------+--------------+--------------+
   | id | guarantor_id | guarantee_id | relationship |
   +----+--------------+--------------+--------------+
   |  2 |            5 |           37 | mother       |
   +----+--------------+--------------+--------------+
8. Note that the relationship still exists, and the id is incremented.
   What should have happened: the guarantor should have been removed (?)
9. Apply the patch and RESET everything (reset_all).
10. Repeat steps 2 to 7. For the result in step 7, the guarantor relationship is removed:
    - SQL query (SELECT * FROM borrower_relationships;): Empty set (0.000 sec)
    - Lisa Charles: No guarantor shown on patron details page
    - Edna Acosta: No guarantee shown on patron details page
11. Test with removing multiple guarantors:
    - Change the patron category for Lisa Charles back to Kid
    - Repeat steps 4 to 7 with these changes:
      .add TWO guarantors.
    Result: All guarantors should be removed.
12. Test changing the patron category to a category that can have a guarantee (for example, J - Juvenile):
    - Change the patron category for Lisa Charles back to Kid
    - Repeat steps 4 to 7 with these changes:
      . add TWO guarantors
      . change the patron category for Lisa Charles from Kid to Juvenile (not Patron)
    Result: all the guarantors should remain.
Comment 6 Lucas Gass (lukeg) 2025-03-17 17:41:53 UTC
Hey David, thanks for testing. 

> However, what is the expected behavour when changing a patron's category
> from one with a guarantor to one without?
> 
> Options:
> 1. Should it silently remove the guarantor? (This is what happens now after
> the patch, which I assume is the behavour expected before the bug was
> identified.)
> 2. Should it display a warning message when attempting to save the patron,
> asking you to confirm that the guarantor relationship(s) will be removed?
> 3. Should there be an information message after saving the changes to the
> patron, saying that the guarantor(s) was removed because the patron category
> was changed?

I think, for the sake of this bug, we want to do option 1 for fixing the bug. 

As a separate enhancement request I like option 2. I think it would be an enhancment. We already have a JS alert that pops up regarding patron messaging prefs being reset. Maybe we need to combine that ( in a modal? ) with information about the guarantor being removed.

I filed Bug 39363:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39363
Comment 7 David Nind 2025-03-17 17:48:25 UTC
Thanks Lucas for filing the bug for the enhancement request. I like option 2 as well.
Comment 8 Baptiste Wojtkowski (bwoj) 2025-03-20 11:13:36 UTC
Hi !
I'd love to see this functionality in next version, guarantors need a lot of improvements.
However I don't like the idea of a silent fail of a so low level function. It looks like a feature that will bring very weird and hard to understand bugs.

The behavior of guarantors is very weird here : it destroys the old relationship to create a brand new one. It should check if it makes sense, add_guarantors should throw an (caught) exception.
Comment 9 Nick Clemens (kidclamp) 2025-03-20 16:49:37 UTC
(In reply to Baptiste Wojtkowski (bwoj) from comment #8)
> Hi !
> I'd love to see this functionality in next version, guarantors need a lot of
> improvements.
> However I don't like the idea of a silent fail of a so low level function.
> It looks like a feature that will bring very weird and hard to understand
> bugs.
> 
> The behavior of guarantors is very weird here : it destroys the old
> relationship to create a brand new one. It should check if it makes sense,
> add_guarantors should throw an (caught) exception.

What do you mean by silent fail here? Koha in the past has always silently removed guarantors when category was switched, this patch just fixes that broken behaviour
Comment 10 Baptiste Wojtkowski (bwoj) 2025-03-21 08:24:26 UTC
(In reply to Nick Clemens (kidclamp) from comment #9)
> (In reply to Baptiste Wojtkowski (bwoj) from comment #8)
> > Hi !
> > I'd love to see this functionality in next version, guarantors need a lot of
> > improvements.
> > However I don't like the idea of a silent fail of a so low level function.
> > It looks like a feature that will bring very weird and hard to understand
> > bugs.
> > 
> > The behavior of guarantors is very weird here : it destroys the old
> > relationship to create a brand new one. It should check if it makes sense,
> > add_guarantors should throw an (caught) exception.
> 
> What do you mean by silent fail here? Koha in the past has always silently
> removed guarantors when category was switched, this patch just fixes that
> broken behaviour

Oh, didn't know it had always been done so, otherwise asking Koha to do something it cannot do should result in an error, and just returning without doing the action.

Setting PQA
Comment 11 Baptiste Wojtkowski (bwoj) 2025-03-21 08:26:06 UTC
Created attachment 179578 [details] [review]
Bug 39331: If patron category cant be guarantee do not continue with add_guarantors

To test:
1 - Remove age limits from a Child and Adult category in Administration->Patron categories. Ensure Child category can be guarantor and Adult cannot
2 - The above step is to avoid any age checks and form resubmission which removes the guarantor fields
3 - Find/create a child patron
4 - Add a guarantee from the Adult category to the child
5 - Check the DB:
   SELECT * FROM borrower_relationships WHERE guarantee_id={borrowernumber of child};
6 - Note the id colum
7 - Edit the child patron to Adult category
8 - Repeat 5
9 - Note the relationship still exists and the id is incremented
10 - Oof!
11 - APPLY PATCH, restart_all
12 - Try again steps 3 - 8
13 - The relationship should not be re-added
14 - Try with more than 1 guarantor on the record, confirming all are deleted
15 - Try adding some guarantor to patron types that should be able to, they relationship should work.
16 - Try changing some patrons to categories that do allow guarantors, making sure the relationship does stay.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com>
Comment 12 Katrin Fischer 2025-03-21 16:51:24 UTC
Pushed for 25.05!

Well done everyone, thank you!