Bug 25322 - Adding a guarantor with no relationship defaults to first available relationship name
Summary: Adding a guarantor with no relationship defaults to first available relations...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Peter Vashchuk
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on:
Blocks: 25946
  Show dependency treegraph
 
Reported: 2020-04-29 19:46 UTC by David Roberts
Modified: 2020-07-16 05:30 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.02


Attachments
Bug 25322: fix for don't default "relationship" dropdown to father (4.48 KB, patch)
2020-05-28 15:21 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25322: fix for don't default "relationship" dropdown to father (4.53 KB, patch)
2020-05-28 15:34 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25322: fix for not selected "relationship" defaults to father (7.15 KB, patch)
2020-06-10 09:36 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25322: fix for not selected "relationship" defaults to father (7.98 KB, patch)
2020-06-17 13:52 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 25322: fix for not selected "relationship" defaults to father (8.05 KB, patch)
2020-06-22 22:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25322: fix for not selected "relationship" defaults to father (8.10 KB, patch)
2020-06-26 08:45 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Roberts 2020-04-29 19:46:53 UTC
If you create a patron guarantor on members/memberentry.pl, but fail to enter a relationship, the relationship defaults to father. If no relationship is specified, then it ought not to be populated. 

To reproduce:

1) Create a new user or modify an existing one.
2) Under Guarantor Information, click on "Search to add" button
3) Perform your search, and select user to act as a guarantor
4) Do not use the dropdown menu to select a relationship.
5) Save the record

The guarantor just added will be listed as the guarantees father.
Comment 1 Andrew Fuerste-Henry 2020-04-29 21:21:04 UTC
It looks like it doesn't default to "father" specifically. It defaults to whatever the first value in the syspref borrowerRelationship is. In the default data, that's "father|mother." When I change it to "|father|mother," so the first possible value is nothing, then it saves with no relationship.
Comment 2 David Roberts 2020-04-29 21:28:36 UTC
(In reply to Andrew Fuerste-Henry from comment #1)
> It looks like it doesn't default to "father" specifically. It defaults to
> whatever the first value in the syspref borrowerRelationship is. In the
> default data, that's "father|mother." When I change it to "|father|mother,"
> so the first possible value is nothing, then it saves with no relationship.

That makes sense - I didn't notice that. However, on my system this is exactly as delivered from the install, so I think that most people will probably find that father is first. Either way, I think it is still incorrect behaviour. Perhaps a sensible workaround might be to make the field required, so that you can't leave it blank? I can't immediately think of a scenario where you wouldn't want to specify what the relationship is.
Comment 3 Andrew Fuerste-Henry 2020-04-30 12:11:27 UTC
I'd be hesitant to make any field hardcoded to be required, but can see why one would want the option. And right now there's no way to make it required as BorrowerMandatoryField only covers things in the borrowers table and guarantor relationship has moved into the borrower_relationships table.
Comment 4 Katrin Fischer 2020-04-30 22:39:12 UTC
If you make it mandatory or leave it - in both situations we will need an empty value added to the pull down, so that it's a conscious choice. I think we should add an empty value for now and a new bug for making it optionally mandatory?
Comment 5 David Roberts 2020-04-30 22:51:41 UTC
(In reply to Katrin Fischer from comment #4)
> If you make it mandatory or leave it - in both situations we will need an
> empty value added to the pull down, so that it's a conscious choice. I think
> we should add an empty value for now and a new bug for making it optionally
> mandatory?

That sounds sensible - I'd be happy with that solution. At the moment, it looks like there is a blank option, but it doesn't appear to be functional.
Comment 6 Katrin Fischer 2020-05-03 21:03:37 UTC
That's odd - after testing I see what you mean. There is an empty option in the pull downs, but if you leave it empty, it still picks the first. Upping severity a bit as this changes data unexpectedly (not quite data loss... but similar)
Comment 7 Peter Vashchuk 2020-05-05 13:41:25 UTC
Yes, there is <select> with 2 defined options: father and mother.
When I added empty <option> and tried to save the form I got 500 error "Invalid relationship passed, '' is not defined.", because:

File: /Koha/Patron/Relationship.pm
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;

Which means there is no possibility to store empty relationship, as well any relationship should be in @valid_relationships.

My proposal is to anyway append empty <option selected></option>, and then either:
- if mandatory: perform form validation and force user to choose one of the defined options,
- if relationship is not mandatory, then the code above (line 53-55) should be changed to allow storing empty relationship (but I am not sure about business logic)

I can implement any of these, but I need your expertise to decide which is the correct one.
Comment 8 Katrin Fischer 2020-05-05 19:56:09 UTC
I just tested this in 18.11 to be able to see how it was before the recent changes. In 18.11 it's also not possible to set the relationship to an empty value - I guess in this case we should make it mandatory?

Would be great to have Kyle's opinion on this maybe.
Comment 9 Jonathan Druart 2020-05-12 11:41:06 UTC
Why not simply select the first option of the select?
Comment 10 David Roberts 2020-05-12 11:44:16 UTC
(In reply to Jonathan Druart from comment #9)
> Why not simply select the first option of the select?

Because you don't *have* to select it - if you don't want a relationship and select it then that's great - it'll do what you want. But if you don't want want a relationship and don't select it because it already looks like the field is already empty, it populates it with data incorrectly.
Comment 11 Jonathan Druart 2020-05-12 11:57:57 UTC
If you have "|father|mother" in the pref (instead of the default "father|mother"), then you can select the empty option, save, and it is kept with "".
This behaviour answers your need.

The problematic situation is when "empty" is not in the pref list. In that case we should select the first entry "father" and force the user to select a value.
Comment 12 Katrin Fischer 2020-05-12 12:45:58 UTC
(In reply to Jonathan Druart from comment #9)
> Why not simply select the first option of the select?

And because it's error prone if you don't make people select something and just assume something - especially in this case as everyone will turn out as a 'dad'
Comment 13 Katrin Fischer 2020-05-12 21:31:08 UTC
(In reply to Jonathan Druart from comment #11)
> If you have "|father|mother" in the pref (instead of the default
> "father|mother"), then you can select the empty option, save, and it is kept
> with "".
> This behaviour answers your need.
> 
> The problematic situation is when "empty" is not in the pref list. In that
> case we should select the first entry "father" and force the user to select
> a value.

I don't think that's correct beahviour. Because we ship Koha with father|mother and we already have an empty value displaying. The problem is that when you use the empty value, father gets added to your record (the first entry). That's clearly unexpected behaviour (select empty, get something else saved).
Comment 14 Andrew Nugged 2020-05-12 21:49:40 UTC
(In reply to Katrin Fischer from comment #13)
> I don't think that's correct beahviour. Because we ship Koha with
> father|mother and we already have an empty value displaying. The problem is
> that when you use the empty value, father gets added to your record (the
> first entry). That's clearly unexpected behaviour (select empty, get
> something else saved).

I support you and support what Peter proposed: 

to append empty <option selected=selected></option>, but in code/DB to have only two options, so validate:

and because of this to force the user to do conscious choice, not allowing empty value to pass-through. Can it be the solution?
Comment 15 Katrin Fischer 2020-05-23 11:20:05 UTC
Do you mean to make the field mandatory with the empty selected not allowing you to save? Then yes :)
Comment 16 Peter Vashchuk 2020-05-28 15:21:21 UTC Comment hidden (obsolete)
Comment 17 ByWater Sandboxes 2020-05-28 15:34:37 UTC
Created attachment 105420 [details] [review]
Bug 25322: fix for don't default "relationship" dropdown to father

When you create a patron's guarantor on cgi-bin/koha/members/memberentry.pl but don't select the relationship from a dropdown, the relationship defaults to first default value, which in this case is father that may or may not be correct as that is not a conscious choice from the user.
The solution is to make the "Relationship" field mandatory, starting with an empty option but not allowing the user to save empty, so choice will be conscious.

To reproduce:
    1) Create a new patron who is assumed to have a guarantor or modify the existing one.
    2) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    3) Observe that relationship is set as "father".
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that it doesn't allow you to save the form until you pick a relationship type.

Mentored-by: Andrew Nugged <nugged@gmail.com>

Signed-off-by: Holly Cooper <hc@interleaf.ie>
Comment 18 Jonathan Druart 2020-05-29 09:01:16 UTC
Did not we want to allow "no relationship"?
Comment 19 Katrin Fischer 2020-05-29 09:45:43 UTC
(In reply to Jonathan Druart from comment #18)
> Did not we want to allow "no relationship"?

It's not possible now, maybe we should discuss this as a future feature separately.
Comment 20 David Roberts 2020-05-29 10:52:33 UTC
(In reply to Jonathan Druart from comment #18)
> Did not we want to allow "no relationship"?

I can't really think of a use case for this. Anybody acting as guarantor must have some sort of relationship with the guarantee, even if it is a non-familial relationship. Couldn't users in any case create an auth value that covers "no relationship" as an option?
Comment 21 Jonathan Druart 2020-06-05 08:49:56 UTC
QA, please test with "|father|mother" in the syspref.
Comment 22 Alex Arnaud 2020-06-05 09:17:16 UTC
Agree with Andrew and Jonathan.

"|father|mother" in borrowerRelationship syspref allows to save unspecified relationship.

As the main issue described here is "If no relationship is specified, then it ought not to be populate", i have to say that i don't find any bug.
Comment 23 Alex Arnaud 2020-06-05 09:20:41 UTC
However, it may not be very clear for users to set the syspref with an empty value. 

I would agree with a patch allowing empty value with the same in the syspref
Comment 24 Alex Arnaud 2020-06-05 09:21:22 UTC
(In reply to Alex Arnaud from comment #23)
> I would agree with a patch allowing empty value with the same in the syspref

Without** the same in the syspref
Comment 25 Jonathan Druart 2020-06-05 09:42:23 UTC
Can we agree on this?

* If the pref does not contain an empty entry then we add a preselected empty value in the dropdown list and make it mandatory

* If the pref contain an empty entry ("|father|mother"), then we make it optional.
Comment 26 Andrew Fuerste-Henry 2020-06-05 11:55:24 UTC
(In reply to Jonathan Druart from comment #25)
> Can we agree on this?
> 
> * If the pref does not contain an empty entry then we add a preselected
> empty value in the dropdown list and make it mandatory
> 
> * If the pref contain an empty entry ("|father|mother"), then we make it
> optional.

Works for me!
Comment 27 Katrin Fischer 2020-06-06 22:57:20 UTC
Hi, I'll try to test, but just to make sure there is no misunderstanding: the issue is not the missing empty option and that it could be configured. The issue is, that you can select empty and save, but it won't save as empty, but select the first of your values (most of the time father, if you haven't changed defaults) That's the unexpected behaviour.

1) Create a new user or modify an existing one.
2) Under Guarantor Information, click on "Search to add" button
3) Perform your search, and select user to act as a guarantor
4) Do not use the dropdown menu to select a relationship.
5) Save the record
6) It will show the relationship as 'father'

Comment#25 would work for me, but I am still not sure it should block this patch, which is a bug fix. We will also need a clear explanation on the system preference.
Comment 28 Katrin Fischer 2020-06-07 10:59:33 UTC
Ok, I understand the problem with this patch now.

borrowerRelationship: |father|mother

With the patch:
When you add an empty entry to the system preference, you end up with 2 empty entries in the list. None of them can be saved.

Without the patch:
The | has no effect - it shows an empty entry with |father|mother and with father|mother. You can save the empty entry, but it will result in father.

I am leaning towards comment#25 now.
Comment 29 Peter Vashchuk 2020-06-10 09:36:15 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2020-06-17 11:51:50 UTC
There is a conflict:
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt

Also, this is invalid:
<label for="guarantor_relationship"[% UNLESS empty_relationship_allowed %] class="required"[% END %]>Relationship: </label>

It breaks our translation tool, you should not add TT tag inside an HTML tag.
You should write instead:
[% UNLESS empty_relationship_allowed %]
    <label for="guarantor_relationship" class="required">Relationship: </label>
[% ELSE %]
    <label for="guarantor_relationship">Relationship: </label>
[% END %]
Comment 31 Peter Vashchuk 2020-06-17 13:52:39 UTC
Created attachment 105972 [details] [review]
Bug 25322: fix for not selected "relationship" defaults to father

When a user creates a patron's guarantor on /cgi-bin/koha/members/memberentry.pl but doesn't select the relationship from a dropdown, the relationship defaults to first value, which in default sysprefs is "father". This may or may not be correct as this is not a conscious choice from the user.

The solution is to make the "Relationship" field mandatory when there is no empty entry in the system preferences, always starting with an empty option but not allowing the user to save an empty entry.
And if there is an empty option in sysprefs, it allows to save empty, as well as makes it default choice.

To reproduce with default system preferences:
    1) Create a new patron who is assumed to have a guarantor or modify the existing one.
    2) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    3) Observe that relationship is set as "father".
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that it doesn't allow you to save the form until you pick a relationship type.

To reproduce with empty entry added to system preferences:
    1) Add an empty entry to borrowerRelationship at /cgi-bin/koha/admin/preferences.pl?tab=patrons in Patron relationships section (example: "|father|mother").
    2) Create a new patron who is assumed to have a guarantor or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    4) Observe that relationship is set as "father".
    5) Apply the patch.
    6) Repeat steps 1, 2 and 3.
    7) Observe when you save the empty entry it does set the relationship as empty.
Comment 32 Katrin Fischer 2020-06-22 22:14:08 UTC
Created attachment 106170 [details] [review]
Bug 25322: fix for not selected "relationship" defaults to father

When a user creates a patron's guarantor on /cgi-bin/koha/members/memberentry.pl but doesn't select the relationship from a dropdown, the relationship defaults to first value, which in default sysprefs is "father". This may or may not be correct as this is not a conscious choice from the user.

The solution is to make the "Relationship" field mandatory when there is no empty entry in the system preferences, always starting with an empty option but not allowing the user to save an empty entry.
And if there is an empty option in sysprefs, it allows to save empty, as well as makes it default choice.

To reproduce with default system preferences:
    1) Create a new patron who is assumed to have a guarantor or modify the existing one.
    2) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    3) Observe that relationship is set as "father".
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that it doesn't allow you to save the form until you pick a relationship type.

To reproduce with empty entry added to system preferences:
    1) Add an empty entry to borrowerRelationship at /cgi-bin/koha/admin/preferences.pl?tab=patrons in Patron relationships section (example: "|father|mother").
    2) Create a new patron who is assumed to have a guarantor or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    4) Observe that relationship is set as "father".
    5) Apply the patch.
    6) Repeat steps 1, 2 and 3.
    7) Observe when you save the empty entry it does set the relationship as empty.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 33 Alex Arnaud 2020-06-26 08:45:11 UTC
Created attachment 106318 [details] [review]
Bug 25322: fix for not selected "relationship" defaults to father

When a user creates a patron's guarantor on /cgi-bin/koha/members/memberentry.pl but doesn't select the relationship from a dropdown, the relationship defaults to first value, which in default sysprefs is "father". This may or may not be correct as this is not a conscious choice from the user.

The solution is to make the "Relationship" field mandatory when there is no empty entry in the system preferences, always starting with an empty option but not allowing the user to save an empty entry.
And if there is an empty option in sysprefs, it allows to save empty, as well as makes it default choice.

To reproduce with default system preferences:
    1) Create a new patron who is assumed to have a guarantor or modify the existing one.
    2) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    3) Observe that relationship is set as "father".
    4) Apply the patch.
    5) Repeat steps 1 and 2.
    6) Observe that it doesn't allow you to save the form until you pick a relationship type.

To reproduce with empty entry added to system preferences:
    1) Add an empty entry to borrowerRelationship at /cgi-bin/koha/admin/preferences.pl?tab=patrons in Patron relationships section (example: "|father|mother").
    2) Create a new patron who is assumed to have a guarantor or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button. After performing the search, select a user to act as guarantor. Don't use the dropdown menu to select a relationship. Save your changes.
    4) Observe that relationship is set as "father".
    5) Apply the patch.
    6) Repeat steps 1, 2 and 3.
    7) Observe when you save the empty entry it does set the relationship as empty.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 34 Jonathan Druart 2020-06-29 11:50:30 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 35 Lucas Gass 2020-07-13 16:40:44 UTC
backported to 20.05.x for 20.05.02
Comment 36 Aleisha Amohia 2020-07-16 04:47:39 UTC
Hi all, this does not apply cleanly on 19.11.x, please rebase if you want me to backport.
Comment 37 Katrin Fischer 2020-07-16 05:30:18 UTC
RMaints, please also take into account the depending bug 25946 - I think we might want to fix this first!