|
Description
Jonathan Druart
2024-09-10 10:24:26 UTC
Did some testing, first issue seems to come from bug 14570 and has probably existed for a while. For second issue, indeed I'm unable to edit and save guarantee with existing guarantor relationship on main. However this works in version 24.05.01, so something must have changed recently? (In reply to Jonathan Druart from comment #0) > 2. If I select one, save. Then edit and save (without modifying anything): > re 500 "There already exists a relationship for the same guarantor (5) and > guarantee (35) combination" Caused by bug 32530. Hi, Working on it but I have a few questions. Concerning point 1, I do not think it is related to the bug since it does not concern the same part of the code. Concerning point 2 Since we now always submit the old relationship to the user, it appears it is detected as a duplicate and an error is thrown while storing the object. I'm wondering why we throw an exception here since storing an existing relationship cannot have impact on data ? Could the fix be removing the exception thrown ? Maybe I can rework this part of the code to use no try/catch. (In reply to Baptiste Wojtkowski (bwoj) from comment #3) > Hi, > Working on it but I have a few questions. > Concerning point 1, I do not think it is related to the bug since it does > not concern the same part of the code. Indeed, it could be moved to a separate bug report. > Concerning point 2 > Since we now always submit the old relationship to the user, it appears it > is detected as a duplicate and an error is thrown while storing the object. > > I'm wondering why we throw an exception here since storing an existing > relationship cannot have impact on data ? Could the fix be removing the > exception thrown ? Maybe I can rework this part of the code to use no > try/catch. You cannot remove the exception, it is at the DB level: UNIQUE KEY `guarantor_guarantee_idx` (`guarantor_id`,`guarantee_id`), It ensures that we won't have a guarantor with different relationships on a given guarantee. All this code is weird to be honest, Koha::Patron->store takes guarantors in parameters but does not update them. We are passing the existing guarantors, (even if deleted!) and the new ones. We are removing and adding guarantors before and after the store. So if the store fails we have already removed the guarantors (you will be prompted with a "cannot save" error, but cancelling at this point will be confusing). Anyway, it seems that the logic is flaw and the code is buggy. What I would do is to simply test (in the controller, sub add_guarantors) if the guarantor already exists before trying to insert it. I created the bug for the 1. in BZ37890 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37890) Note : since we fixed the add, and the add is performed after the remove without check, the checkbox "remove" is now broken Created attachment 171302 [details] [review] Bug 37881: Editing patron with guarantor won't crash This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted Created attachment 171303 [details] [review] Bug 37881: Fix the delete button for guarantor This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted This patch works for editing a patron with a guarantor and deleting a guarantor. But when adding a new guarantor, if you don't select a relationship it still crashes with the error: Invalid relationship passed, '' is not defined. at /usr/share/perl5/Exception/Class/Base.pm line 88 (In reply to Brendan Lawlor from comment #9) > This patch works for editing a patron with a guarantor and deleting a > guarantor. > > But when adding a new guarantor, if you don't select a relationship it still > crashes with the error: > > Invalid relationship passed, '' is not defined. at > /usr/share/perl5/Exception/Class/Base.pm line 88 Yes, it has been moved to bug 37890. It should not be considered blocker here. Created attachment 171399 [details] [review] Bug 37881: Editing patron with guarantor won't crash This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 171400 [details] [review] Bug 37881: Fix the delete button for guarantor This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 171442 [details] [review] Bug 37881: Editing patron with guarantor won't crash This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 171443 [details] [review] Bug 37881: Fix the delete button for guarantor This patch fixes issue introduced by BZ32530 Test Plan (on main): 1 - Edit a patron 2 - Do not change anything and submit -> you get an error 3 - Apply both patches 4 - Repeat 1&2 -> everything works fine 5 - Try and delete the guarantor -> it will be deleted Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 171444 [details] [review] Bug 37881: (QA follow-up) Correction to TT filters used We were missing a couple of TT filters and we were using a `$raw` where it was more appropriate to use `html`. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Apologies for not catching this earlier with bug 32530. This is all working and other issues have been put into other bugs. Passing QA Pushed for 24.11! Well done everyone, thank you! Missing dependencies for 24.05.x |