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!
Yup, everything in Koha::Patron->store looks correct but add_guarantors adds them back
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.
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.
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>
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.
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
Thanks Lucas for filing the bug for the enhancement request. I like option 2 as well.
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.
(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
(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
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>
Pushed for 25.05! Well done everyone, thank you!