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.
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