[Bug 37528] New: Using borrowerRelationship while guarantor relationship is unchecked from BorrowerMandatoryField results in error
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Bug ID: 37528 Summary: Using borrowerRelationship while guarantor relationship is unchecked from BorrowerMandatoryField results in error Change sponsored?: --- Product: Koha Version: 24.05 Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: Patrons Assignee: koha-bugs@lists.koha-community.org Reporter: katariina.pohto@ouka.fi QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Before 24.05 having values in borrowerRelationship made the guarantor relationship field required in patron page. Now one needs to check the relationship option from BorrowerMandatoryField settings. If borrower relationships are used but the relationship is not set up as required field, it is possible to save a patron with the empty option from gurantor relationship drop down selector. This will result in an internal service error. Despite the error, the new patron will be saved, but the guarantor information is lost in the process, as it wasn't valid. To replicate: Have at least one option in borrowerRelationship Have the relationship field unchecked from BorrowerMandatoryFields 1. Create or select an existing patron in category that can be a guarantee. 2. Select a guarantor but leave the relationship blank 3. Fill in the other required information if not present yet. 4. Try to save the patron. 5. Notice an error page. 6. Check the patron and note that the new patron or changes to existing one were saved without the new guarantor information. Also, I'd like to add that when adding a new guarantee for an existing patron, the hint text for required field doesn't show correctly with the mandatory field setting. It will show 'required field' when it's UNchecked from mandatory fields, and doesn't show it when it's checked. When creating a new patron from scratch or editing an existing guarantee patron, the 'required field' text works correctly. -- 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=37528 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |johanna.raisa@koha-suomi.fi |ity.org | Status|NEW |Needs Signoff CC| |johanna.raisa@koha-suomi.fi --- Comment #1 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- Created attachment 169925 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=169925&action=edit Bug 37528: check if selected relationship is valid This patch checks if the selected relationship is valid before trying to save the patron record. It takes the list of valid relationships from borrowerRelationships syspref and checks if the selected relationship is in the list. Also this patch fixes relationship field required message when BorrowerMandatoryField is not set. The required message is shown when adding the guarantee from guarantor's detail page. Test plan: 1) Add at least one option to borrowerRelationships syspref. 2) Leave the relationship unchecked from BorrowerMandatoryField syspref. 3) Create a new guarantee patron. 4) Add a guarantor to the guarantee patron. 5) Leave the relationship field empty and try to save the patron record. 6) Notice the 500 error page. 7) Apply the patch. 8) Repeat steps 3-5. 9) Notice the error message "Guarantor relationship is invalid". Sponsored-by: Koha-Suomi Oy -- 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=37528 Johanna Räisä <johanna.raisa@koha-suomi.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Olivier V <ovezina@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #169925|0 |1 is obsolete| | --- Comment #2 from Olivier V <ovezina@inlibro.com> --- Created attachment 171277 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171277&action=edit Bug 37528: check if selected relationship is valid This patch checks if the selected relationship is valid before trying to save the patron record. It takes the list of valid relationships from borrowerRelationships syspref and checks if the selected relationship is in the list. Also this patch fixes relationship field required message when BorrowerMandatoryField is not set. The required message is shown when adding the guarantee from guarantor's detail page. Test plan: 1) Add at least one option to borrowerRelationships syspref. 2) Leave the relationship unchecked from BorrowerMandatoryField syspref. 3) Create a new guarantee patron. 4) Add a guarantor to the guarantee patron. 5) Leave the relationship field empty and try to save the patron record. 6) Notice the 500 error page. 7) Apply the patch. 8) Repeat steps 3-5. 9) Notice the error message "Guarantor relationship is invalid". Sponsored-by: Koha-Suomi Oy Signed-off-by: Olivier V <olivier.vezina@inLibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Olivier V <ovezina@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ovezina@inlibro.com 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=37528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171277|0 |1 is obsolete| | --- Comment #3 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 171541 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171541&action=edit Bug 37528: check if selected relationship is valid This patch checks if the selected relationship is valid before trying to save the patron record. It takes the list of valid relationships from borrowerRelationships syspref and checks if the selected relationship is in the list. Also this patch fixes relationship field required message when BorrowerMandatoryField is not set. The required message is shown when adding the guarantee from guarantor's detail page. Test plan: 1) Add at least one option to borrowerRelationships syspref. 2) Leave the relationship unchecked from BorrowerMandatoryField syspref. 3) Create a new guarantee patron. 4) Add a guarantor to the guarantee patron. 5) Leave the relationship field empty and try to save the patron record. 6) Notice the 500 error page. 7) Apply the patch. 8) Repeat steps 3-5. 9) Notice the error message "Guarantor relationship is invalid". Sponsored-by: Koha-Suomi Oy Signed-off-by: Olivier V <olivier.vezina@inLibro.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=37528 --- Comment #4 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 171542 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171542&action=edit Bug 37528: (follow-up) Tidyness -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |baptiste.wojtkowski@biblibr | |e.com --- Comment #5 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Note: if you add a pipe at the end of the syspref while doing 1., this will not result in an arror -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #6 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- *** Bug 37890 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #7 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- *** Bug 37896 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com --- Comment #8 from Lucas Gass <lucas@bywatersolutions.com> --- Can you explain why an empty value, or '', is an invalid relationship? If I do not include relationship in the BorrowerMandatoryField system preference than I should be able to submit the form without requiring a relationship, right? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|24.05 |Main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |37892 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37892 [Bug 37892] Patron category 'can be a guarantee' means that same category cannot be a guarantor -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #9 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Hi ! I'm not the author of the patch but I'm quite interesting in helping it to pass :) I think the reason is in Relationship.pm (code introduced in Bug 14570, v19.11). The matter of current patch is to react by a lock in the interface to the change and preventing from a dirty crash. 47 sub store { 48 my ( $self ) = @_; 49 50 my @valid_relationships = split /\|/, C4::Context->preference('borrowerRelationship'), -1; 51 @valid_relationships = ('') unless @valid_relationships; 52 53 Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( 54 no_relationship => 1 ) 55 unless defined $self->relationship; 56 57 Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( 58 relationship => $self->relationship ) 59 unless any { $_ eq $self->relationship } @valid_relationships; 60 61 return try { 62 $self->SUPER::store; 63 } 64 catch { 65 if ( ref($_) eq 'Koha::Exceptions::Object::DuplicateID' ) { 66 Koha::Exceptions::Patron::Relationship::DuplicateRelationship->throw( 67 guarantee_id => $self->guarantee_id, 68 guarantor_id => $self->guarantor_id 69 ); 70 } 71 }; 72 } In these lines, we can clearly see that "" is invalid unless 1 - it is expressly specified in borrowerRelationship, by adding a "|" at the end OR 2- thes is nothing in borrowerRelationship. It looks intentional to me but I could not find any reference in concerned BZ. Forbidding the use of "" might be an undesired behaviour but the 500 error is way more an issue. So I see multiple options : 1 - We consider this patch is valid and add to the documentation that "" must be explicitly allowed (which is my opinion here). We can add a tooltip to help librarian configuring it properly (something like "Your configuration requires relationships to be filled", linking to syspref, and in syspref a text like "finish by '|' to allow leaving it blank"). 2 - (not sure it can be done tbh) In relationship.pm, We check the content of mandatory subfields to check if guarantor relationship and add "" in relationships.pm. 3 - We consider that forbidding "" by default is a regression and therefore this patch is just a workaround for a problem, hence we add "" by default in the list of @validRelationships in the aforementionned code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #10 from Johanna Räisä <johanna.raisa@koha-suomi.fi> --- (In reply to Baptiste Wojtkowski (bwoj) from comment #9)
Hi ! I'm not the author of the patch but I'm quite interesting in helping it to pass :)
I think the reason is in Relationship.pm (code introduced in Bug 14570, v19.11). The matter of current patch is to react by a lock in the interface to the change and preventing from a dirty crash.
47 sub store { 48 my ( $self ) = @_; 49 50 my @valid_relationships = split /\|/, C4::Context->preference('borrowerRelationship'), -1; 51 @valid_relationships = ('') unless @valid_relationships; 52 53 Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( 54 no_relationship => 1 ) 55 unless defined $self->relationship; 56 57 Koha::Exceptions::Patron::Relationship::InvalidRelationship->throw( 58 relationship => $self->relationship ) 59 unless any { $_ eq $self->relationship } @valid_relationships; 60 61 return try { 62 $self->SUPER::store; 63 } 64 catch { 65 if ( ref($_) eq 'Koha::Exceptions::Object::DuplicateID' ) { 66 Koha::Exceptions::Patron::Relationship::DuplicateRelationship->throw( 67 guarantee_id => $self->guarantee_id, 68 guarantor_id => $self->guarantor_id 69 ); 70 } 71 }; 72 }
In these lines, we can clearly see that "" is invalid unless 1 - it is expressly specified in borrowerRelationship, by adding a "|" at the end OR 2- thes is nothing in borrowerRelationship. It looks intentional to me but I could not find any reference in concerned BZ.
Forbidding the use of "" might be an undesired behaviour but the 500 error is way more an issue. So I see multiple options : 1 - We consider this patch is valid and add to the documentation that "" must be explicitly allowed (which is my opinion here). We can add a tooltip to help librarian configuring it properly (something like "Your configuration requires relationships to be filled", linking to syspref, and in syspref a text like "finish by '|' to allow leaving it blank"). 2 - (not sure it can be done tbh) In relationship.pm, We check the content of mandatory subfields to check if guarantor relationship and add "" in relationships.pm. 3 - We consider that forbidding "" by default is a regression and therefore this patch is just a workaround for a problem, hence we add "" by default in the list of @validRelationships in the aforementionned code.
I think it is a bit unclear how those should be configured to work together. The systempreference should have more information about the empty value. I had to go to see the code to figure out that there can be pipe added at the end of the preference to allow the empty value. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #11 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Created attachment 172560 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172560&action=edit Bug 37528: (follow-up) Add help strings for blank borrowerrelationship -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #12 from Baptiste Wojtkowski (bwoj) <baptiste.wojtkowski@biblibre.com> --- Note: I started to create a special error for empty string, but when it came to creating a test plan, I realised that there is no way to enter anything wrong except "", so I used invalid_relationship to display the error on empty string. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Michael Adamyk <madamyk@ckls.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |madamyk@ckls.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171541|0 |1 is obsolete| | --- Comment #13 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 172893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172893&action=edit Bug 37528: check if selected relationship is valid This patch checks if the selected relationship is valid before trying to save the patron record. It takes the list of valid relationships from borrowerRelationships syspref and checks if the selected relationship is in the list. Also this patch fixes relationship field required message when BorrowerMandatoryField is not set. The required message is shown when adding the guarantee from guarantor's detail page. Test plan: 1) Add at least one option to borrowerRelationships syspref. 2) Leave the relationship unchecked from BorrowerMandatoryField syspref. 3) Create a new guarantee patron. 4) Add a guarantor to the guarantee patron. 5) Leave the relationship field empty and try to save the patron record. 6) Notice the 500 error page. 7) Apply the patch. 8) Repeat steps 3-5. 9) Notice the error message "Guarantor relationship is invalid". Sponsored-by: Koha-Suomi Oy Signed-off-by: Olivier V <olivier.vezina@inLibro.com> Signed-off-by: Baptiste Wojtkowski <baptiste.wojtkowski@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #171542|0 |1 is obsolete| | --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 172894 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172894&action=edit Bug 37528: (follow-up) Tidyness Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #172560|0 |1 is obsolete| | --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- Created attachment 172895 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=172895&action=edit Bug 37528: (follow-up) Add help strings for blank borrowerrelationship Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com --- Comment #16 from Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> --- QA'd in conjunction with bug 37892. I'm happy this all works well and is clear to the end user once the follow-up bug is also taken into account. Happy to PQA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! 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=37528 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31470 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31470 [Bug 31470] Incorrect selector for relationship dropdown used in members.js -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Pushed to stable Version(s)|24.11.00 |24.11.00,24.05.06 released in| | --- Comment #18 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Backported to 24.05.x for upcoming 24.05.06 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable Version(s)|24.11.00,24.05.06 |24.11.00,24.05.06,23.11.11 released in| | CC| |fridolin.somers@biblibre.co | |m --- Comment #19 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37528 Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz Status|Pushed to oldstable |Needs documenting --- Comment #20 from Wainui Witika-Park <wainuiwitikapark@catalyst.net.nz> --- Not backporting to 22.11 unless requested -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org