Bug 30987 - Adding relationship to PatronQuickAddFields causes it to be added 2x
Summary: Adding relationship to PatronQuickAddFields causes it to be added 2x
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 36719
  Show dependency treegraph
 
Reported: 2022-06-17 16:02 UTC by Lucas Gass
Modified: 2024-05-28 19:32 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.06,23.05.12


Attachments
Bug 30987: Fix relationship fields for patron full and quick add forms (11.13 KB, patch)
2024-02-26 16:11 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 30987: Fix relationship fields for patron full and quick add forms (11.17 KB, patch)
2024-03-07 00:45 UTC, Myka Kennedy Stephens
Details | Diff | Splinter Review
Bug 30987: Fix relationship fields for patron full and quick add forms (11.31 KB, patch)
2024-04-17 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2022-06-17 16:02:56 UTC
While working on Bug 29303 I noticed a small problem. If you add relationship to  the PatronQuickAddFields system preference it will be added to the quick form two times. It looks like it is due to some JS that is no longer needed:

if ( $("#memberentry_guarantor").length ) {
    $("#memberentry_guarantor").appendTo("#quick_add_form");
}
Comment 1 Lucas Gass 2022-06-17 16:11:19 UTC
This is still a bug but I am completely wrong about the JS I mentioned. It is not the cause.
Comment 2 Lucas Gass 2022-06-17 16:20:18 UTC
If you add relationship to the QuickAddFields, go to the quick add form, and press "+ Add guarantor" the relationship field will appear 3 times.
Comment 3 Lucas Gass 2024-01-25 21:06:33 UTC
Still valid.
Comment 4 Lucas Gass 2024-02-15 17:55:56 UTC
The problem here seems to be that we have 2 ways of adding the Relationship field. 

1. If  borrowerRelationship is NOT empty we assume the field should be added.
2. It can also be added via PatronQuickAddFields

I think we need to remove the logic that automatically adds the field if borrowerRelationship is NOT empty:

104 my @relations = split /\|/, C4::Context->preference('borrowerRelationship'), -1;
105 @relations = ('') unless @relations;
106 my $empty_relationship_allowed = grep {$_ eq ""} @relations;
107 $template->param( empty_relationship_allowed => $empty_relationship_allowed );


We can rely on BorrowerUnwantedField, BorrowerMandatoryField, and  PatronQuickAddFields to setup this field in the desired way.
Comment 5 Lucas Gass 2024-02-26 16:11:59 UTC
Created attachment 162461 [details] [review]
Bug 30987: Fix relationship fields for patron full and quick add forms

To test:
1. APPLY PATACH and restart services.
2. Find the borrowerRelationship system preference. The description should no longer include the words "Leave empty to deactivate."
3. Populate the system preference with at least 1 choice.
4. Find a patron category with can_be_guarantee set to 'Yes'.
5. Quick add a patron of that type, making sure the relationship field shows in the Patron guarantor section. ( You have to +Add gaurantor before this field will show )
6. The values in the dropdown should refelct the borrowerRelationship values.
7. With BorrowerMandatoryField make relationship mandatory.
8. Try step 5 again, this time the Relationship field should be mandatory.
9. Remove the field from  BorrowerMandatoryField and add it to BorrowerUnwantedField.
10. Do step 5 again, the relationship field should not show on the quick add form.
Comment 6 Myka Kennedy Stephens 2024-03-07 00:45:01 UTC
Created attachment 162860 [details] [review]
Bug 30987: Fix relationship fields for patron full and quick add forms

To test:
1. APPLY PATACH and restart services.
2. Find the borrowerRelationship system preference. The description should no longer include the words "Leave empty to deactivate."
3. Populate the system preference with at least 1 choice.
4. Find a patron category with can_be_guarantee set to 'Yes'.
5. Quick add a patron of that type, making sure the relationship field shows in the Patron guarantor section. ( You have to +Add gaurantor before this field will show )
6. The values in the dropdown should refelct the borrowerRelationship values.
7. With BorrowerMandatoryField make relationship mandatory.
8. Try step 5 again, this time the Relationship field should be mandatory.
9. Remove the field from  BorrowerMandatoryField and add it to BorrowerUnwantedField.
10. Do step 5 again, the relationship field should not show on the quick add form.

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>
Comment 7 Myka Kennedy Stephens 2024-03-07 00:45:37 UTC
Patch works as described. Great work!
Comment 8 Katrin Fischer 2024-04-04 10:01:01 UTC
Hm there is also an additional issue here: the one that is there first always appears as mandatory. QA please? :)
Comment 9 Martin Renvoize 2024-04-17 12:41:11 UTC
Created attachment 165005 [details] [review]
Bug 30987: Fix relationship fields for patron full and quick add forms

To test:
1. APPLY PATACH and restart services.
2. Find the borrowerRelationship system preference. The description should no longer include the words "Leave empty to deactivate."
3. Populate the system preference with at least 1 choice.
4. Find a patron category with can_be_guarantee set to 'Yes'.
5. Quick add a patron of that type, making sure the relationship field shows in the Patron guarantor section. ( You have to +Add gaurantor before this field will show )
6. The values in the dropdown should refelct the borrowerRelationship values.
7. With BorrowerMandatoryField make relationship mandatory.
8. Try step 5 again, this time the Relationship field should be mandatory.
9. Remove the field from  BorrowerMandatoryField and add it to BorrowerUnwantedField.
10. Do step 5 again, the relationship field should not show on the quick add form.

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-04-17 12:41:39 UTC
Good fix, works as described and QA scripts happy.

Passing QA
Comment 11 Katrin Fischer 2024-04-18 14:06:29 UTC
How do we deactivate the feature if leaving it empty is no longer an option? 

--> I guess we are referring to the *Unwanted* prefs then?

What happens with libraries who left it empty, do we need to handle that somehow?
Comment 12 Katrin Fischer 2024-04-18 14:14:43 UTC
As we check the category in order to display the relationship or not, should we remove "relationship" as an option from PatronQuickAddFields?
Comment 13 Katrin Fischer 2024-04-18 14:51:57 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 14 Katrin Fischer 2024-04-26 20:40:16 UTC
(In reply to Katrin Fischer from comment #12)
> As we check the category in order to display the relationship or not, should
> we remove "relationship" as an option from PatronQuickAddFields?

*ping*
Comment 15 Lucas Gass 2024-04-29 17:13:50 UTC
(In reply to Katrin Fischer from comment #12)
> As we check the category in order to display the relationship or not, should
> we remove "relationship" as an option from PatronQuickAddFields?

In my opinion, yes.
Comment 16 Katrin Fischer 2024-04-29 17:16:15 UTC
(In reply to Lucas Gass from comment #15)
> (In reply to Katrin Fischer from comment #12)
> > As we check the category in order to display the relationship or not, should
> > we remove "relationship" as an option from PatronQuickAddFields?
> 
> In my opinion, yes.

Just running out - could you file a bug OR follow-up and remove additional_work_needed? Thanks!
Comment 17 Lucas Gass 2024-04-29 17:40:49 UTC
(In reply to Katrin Fischer from comment #16)
> (In reply to Lucas Gass from comment #15)
> > (In reply to Katrin Fischer from comment #12)
> > > As we check the category in order to display the relationship or not, should
> > > we remove "relationship" as an option from PatronQuickAddFields?
> > 
> > In my opinion, yes.
> 
> Just running out - could you file a bug OR follow-up and remove
> additional_work_needed? Thanks!

Filed Bug 36719, add will add a patch asap.
Comment 18 Fridolin Somers 2024-05-23 13:25:38 UTC
Pushed to 23.11.x for 23.11.06
Comment 19 Lucas Gass 2024-05-28 19:32:36 UTC
Backported to 23.05.x for upcoming 23.05.12