Bug 37528 - Using borrowerRelationship while guarantor relationship is unchecked from BorrowerMandatoryField results in error
Summary: Using borrowerRelationship while guarantor relationship is unchecked from Bor...
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Johanna Räisä
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
: 37890 37896 (view as bug list)
Depends on:
Blocks: 31470 37892
  Show dependency treegraph
 
Reported: 2024-07-30 13:57 UTC by Katariina Pohto
Modified: 2024-10-18 12:59 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 37528: check if selected relationship is valid (4.12 KB, patch)
2024-08-01 06:16 UTC, Johanna Räisä
Details | Diff | Splinter Review
Bug 37528: check if selected relationship is valid (4.17 KB, patch)
2024-09-10 16:51 UTC, Olivier Vezina
Details | Diff | Splinter Review
Bug 37528: check if selected relationship is valid (4.23 KB, patch)
2024-09-16 13:31 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37528: (follow-up) Tidyness (1.14 KB, patch)
2024-09-16 13:32 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37528: (follow-up) Add help strings for blank borrowerrelationship (2.38 KB, patch)
2024-10-09 08:09 UTC, Baptiste Wojtkowski (bwoj)
Details | Diff | Splinter Review
Bug 37528: check if selected relationship is valid (4.31 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37528: (follow-up) Tidyness (1.21 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37528: (follow-up) Add help strings for blank borrowerrelationship (2.46 KB, patch)
2024-10-17 15:08 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katariina Pohto 2024-07-30 13:57:03 UTC
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.
Comment 1 Johanna Räisä 2024-08-01 06:16:49 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
Comment 2 Olivier Vezina 2024-09-10 16:51:47 UTC
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>
Comment 3 Baptiste Wojtkowski (bwoj) 2024-09-16 13:31:29 UTC
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>
Comment 4 Baptiste Wojtkowski (bwoj) 2024-09-16 13:32:51 UTC
Created attachment 171542 [details] [review]
Bug 37528: (follow-up) Tidyness
Comment 5 Baptiste Wojtkowski (bwoj) 2024-09-16 13:34:07 UTC
Note: if you add a pipe at the end of the syspref while doing 1., this will not result in an arror
Comment 6 Baptiste Wojtkowski (bwoj) 2024-09-16 13:35:21 UTC
*** Bug 37890 has been marked as a duplicate of this bug. ***
Comment 7 Baptiste Wojtkowski (bwoj) 2024-09-23 10:00:05 UTC
*** Bug 37896 has been marked as a duplicate of this bug. ***
Comment 8 Lucas Gass (lukeg) 2024-10-07 22:23:40 UTC
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?
Comment 9 Baptiste Wojtkowski (bwoj) 2024-10-08 10:20:26 UTC
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.
Comment 10 Johanna Räisä 2024-10-09 06:05:52 UTC
(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.
Comment 11 Baptiste Wojtkowski (bwoj) 2024-10-09 08:09:36 UTC
Created attachment 172560 [details] [review]
Bug 37528: (follow-up) Add help strings for blank borrowerrelationship
Comment 12 Baptiste Wojtkowski (bwoj) 2024-10-09 08:13:23 UTC
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.
Comment 13 Martin Renvoize (ashimema) 2024-10-17 15:08:23 UTC
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>
Comment 14 Martin Renvoize (ashimema) 2024-10-17 15:08:25 UTC
Created attachment 172894 [details] [review]
Bug 37528: (follow-up) Tidyness

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2024-10-17 15:08:28 UTC
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>
Comment 16 Martin Renvoize (ashimema) 2024-10-17 15:10:01 UTC
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
Comment 17 Katrin Fischer 2024-10-18 12:27:17 UTC
Pushed for 24.11!

Well done everyone, thank you!