[Bug 39331] New: Guarantor relationships not removed when changing patron category from memberentry.pl
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Bug ID: 39331 Summary: Guarantor relationships not removed when changing patron category from memberentry.pl Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: nick@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #1 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Yup, everything in Koha::Patron->store looks correct but add_guarantors adds them back -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |lucas@bywatersolutions.com |ity.org | Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #2 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 179360 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179360&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179360|0 |1 is obsolete| | --- Comment #3 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Created attachment 179361 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179361&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179361|0 |1 is obsolete| | --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 179369 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179369&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Text to go in the| |This fixes changing a release notes| |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). --- Comment #5 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #6 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #7 from David Nind <david@davidnind.com> --- Thanks Lucas for filing the bug for the enhancement request. I like option 2 as well. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com QA Contact|testopia@bugs.koha-communit |baptiste.wojtkowski@biblibr |y.org |e.com --- Comment #8 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #9 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #10 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #179369|0 |1 is obsolete| | --- Comment #11 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 179578 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=179578&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 --- Comment #12 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 25.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=39233 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.05.00 |25.05.00,24.11.06 released in| | Status|Pushed to main |Pushed to stable CC| |fridolin.somers@biblibre.co | |m --- Comment #13 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 24.11.x for 24.11.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Jesse Maseto <jesse@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable CC| |jesse@bywatersolutions.com --- Comment #14 from Jesse Maseto <jesse@bywatersolutions.com> --- Pushed to 24.05.x for 24.05.12 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to oldstable |Needs documenting --- Comment #15 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11.x unless requested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Needs documenting |RESOLVED --- Comment #16 from David Nind <david@davidnind.com> --- Bug fix, no change to the manual required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39331 Christopher Brannon <cbrannon@cdalibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |cbrannon@cdalibrary.org --- Comment #17 from Christopher Brannon <cbrannon@cdalibrary.org> --- This bug apparently does not fix already affected accounts. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org