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.

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

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.

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?

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

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)

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.

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.

Why not simply select the first option of the select?

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

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.

(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'

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

(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?

Do you mean to make the field mandatory with the empty selected not allowing you to save? Then yes :)

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>

Did not we want to allow "no relationship"?

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

(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?

QA, please test with "|father|mother" in the syspref.

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.

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

(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

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.

(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!

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.

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.

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 %]

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.

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>

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>

Pushed to master for 20.11, thanks to everybody involved!

backported to 20.05.x for 20.05.02

Hi all, this does not apply cleanly on 19.11.x, please rebase if you want me to backport.

RMaints, please also take into account the depending bug 25946 - I think we might want to fix this first!