Summary: | Using borrowerRelationship while guarantor relationship is unchecked from BorrowerMandatoryField results in error | ||
---|---|---|---|
Product: | Koha | Reporter: | Katariina Pohto <katariina.pohto> |
Component: | Patrons | Assignee: | Johanna Räisä <johanna.raisa> |
Status: | Pushed to oldstable --- | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | normal | ||
Priority: | P5 - low | CC: | baptiste.wojtkowski, fridolin.somers, gmcharlt, johanna.raisa, kyle.m.hall, lucas, madamyk, martin.renvoize, nick, ovezina |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | Sponsored | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
24.11.00,24.05.06,23.11.11
|
|
Circulation function: | |||
Bug Depends on: | |||
Bug Blocks: | 31470, 37892 | ||
Attachments: |
Bug 37528: check if selected relationship is valid
Bug 37528: check if selected relationship is valid Bug 37528: check if selected relationship is valid Bug 37528: (follow-up) Tidyness Bug 37528: (follow-up) Add help strings for blank borrowerrelationship Bug 37528: check if selected relationship is valid Bug 37528: (follow-up) Tidyness Bug 37528: (follow-up) Add help strings for blank borrowerrelationship |
Description
Katariina Pohto
2024-07-30 13:57:03 UTC
Created attachment 169925 [details] [review] 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 Created attachment 171277 [details] [review] 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> Created attachment 171541 [details] [review] 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> Created attachment 171542 [details] [review] Bug 37528: (follow-up) Tidyness Note: if you add a pipe at the end of the syspref while doing 1., this will not result in an arror *** Bug 37890 has been marked as a duplicate of this bug. *** *** Bug 37896 has been marked as a duplicate of this bug. *** 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? 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. (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. Created attachment 172560 [details] [review] Bug 37528: (follow-up) Add help strings for blank borrowerrelationship 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. Created attachment 172893 [details] [review] 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> Created attachment 172894 [details] [review] Bug 37528: (follow-up) Tidyness Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 172895 [details] [review] Bug 37528: (follow-up) Add help strings for blank borrowerrelationship Signed-off-by: Martin Renvoize <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 Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.06 Pushed to 23.11.x for 23.11.11 |