Bug 25946

Summary: borrowerRelationship can no longer be empty
Product: Koha Reporter: Owen Leonard <oleonard>
Component: PatronsAssignee: Peter Vashchuk <stalkernoid>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: major    
Priority: P5 - low CC: alex.arnaud, andrewfh, cbrannon, david.roberts, fridolin.somers, gmcharlt, hc, jonathan.druart, kyle.m.hall, martin.renvoize, nugged, sally.healey, slavashishkin, stalkernoid
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05
Bug Depends on: 25322    
Bug Blocks:    
Attachments: Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: borrowerRelationship can be empty
Bug 25946: (QA follow-up) Add Unit tests

Description Owen Leonard 2020-07-07 17:00:33 UTC
Since Bug 25322 it is not possible to have an empty borrowerRelationship system preference. The empty relationship dropdown is treated as mandatory.
Comment 1 David Roberts 2020-07-07 23:07:23 UTC
I can't think of a reason why you would want to leave this empty - if the patron has a guarantor then they automatically must have some sort of relationship with the guarantee, even if it isn't a familial one.
Comment 2 Owen Leonard 2020-07-07 23:49:19 UTC
(In reply to David Roberts from comment #1)
> I can't think of a reason why you would want to leave this empty

It's not unreasonable to think that a library might not want to or care about storing the relationship information.

But the bottom line is that the system preference can be empty so we should expect that it might be empty.
Comment 3 Jonathan Druart 2020-07-27 13:04:40 UTC
Peter, can you take a look at this one please?
Comment 4 Andrii Nugged 2020-07-27 13:46:19 UTC
He's in different timezone so I'll take initiative in communication here - yes, we'll (me too to understand and mentoring) take over this today/tomorrow.
Comment 5 Peter Vashchuk 2020-07-28 15:24:41 UTC
Created attachment 107469 [details] [review]
Bug 25946: borrowerRelationship can be empty

When borrowerRelationship is empty in system preferences, Relationship
dropdown is not required and we accept empty value.

Also fixes bug that didn't let you to pick empty value even when you
specified that it should be possible in system preferences but in the
end of the string (i.e. "|father|mother" worked,
but "father|mother|" don't).

To reproduce (borrowerRelationship can be empty):
    1) Go to system preferences and make borrowerRelationship empty.
    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. Try to save your changes.
    4) Observe that relationship field is required in order to save but
    you can't actually choose anything as it doesn't contain anything.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

To reproduce (can't choose empty value bug):
    1) Go to system preferences and set borrowerRelationship exactly
    to "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.
    4) Observe that there's no option to leave relationship field empty.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it has empty option that you can choose and save
    the form.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 6 Sally 2020-07-31 15:24:27 UTC
Sorry, couldn't get this one to work - it fails at step 7 below:  

To reproduce (borrowerRelationship can be empty):
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

Invalid relationship passed, '' is not defined. at /usr/share/perl5/Exception/Class/Base.pm line 88
Comment 7 Peter Vashchuk 2020-08-11 09:41:17 UTC
Created attachment 108048 [details] [review]
Bug 25946: borrowerRelationship can be empty

When borrowerRelationship is empty in system preferences, Relationship
dropdown is not required and we accept empty value.

Also fixes bug that didn't let you to pick empty value even when you
specified that it should be possible in system preferences but in the
end of the string (i.e. "|father|mother" worked,
but "father|mother|" don't).

To reproduce (borrowerRelationship can be empty):
    1) Go to system preferences and make borrowerRelationship empty.
    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. Try to save your changes.
    4) Observe that relationship field is required in order to save but
    you can't actually choose anything as it doesn't contain anything.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

To reproduce (can't choose empty value bug):
    1) Go to system preferences and set borrowerRelationship exactly
    to "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.
    4) Observe that there's no option to leave relationship field empty.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it has empty option that you can choose and save
    the form.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 8 Katrin Fischer 2020-09-16 06:28:57 UTC
Hi Andrew,

you write:
> When borrowerRelationship is empty in system preferences, Relationship
    dropdown is not required and we accept empty value.

I think that's correct. But when I leave the preference empty and try to add a guarantor the pull down still shows with only an empty entry. I feel like we should remove it in this case.
Comment 9 Peter Vashchuk 2021-03-30 11:43:42 UTC
Created attachment 118991 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not split the config using comma.

From syspref description of borrowerRelationship preference:

> Guarantors can be the following of those they guarantee:
> (input multiple choices separated by |). Leave empty to deactivate.

As it doesn't mention comma at all, I removed ',' from split.
Of course if comma is actually a viable way to split separate choices,
I can obsolete this patch and append to the syspref description that it
also can be separated by comma.
Comment 10 Peter Vashchuk 2021-03-30 11:43:46 UTC
Created attachment 118992 [details] [review]
Bug 25946: borrowerRelationship can be empty

When borrowerRelationship is empty in system preferences, Relationship
dropdown is not required and we accept empty value.

Also fixes bug that didn't let you to pick empty value even when you
specified that it should be possible in system preferences but in the
end of the string (i.e. "|father|mother" worked,
but "father|mother|" don't).

To reproduce (borrowerRelationship can be empty):
    1) Go to system preferences and make borrowerRelationship empty.
    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. Try to save your changes.
    4) Observe that relationship field is required in order to save but
    you can't actually choose anything as it doesn't contain anything.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

To reproduce (can't choose empty value bug):
    1) Go to system preferences and set borrowerRelationship exactly
    to "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.
    4) Observe that there's no option to leave relationship field empty.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it has empty option that you can choose and save
    the form.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 11 Peter Vashchuk 2021-03-30 11:43:51 UTC
Created attachment 118993 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not show the empty dropdown with no options if borrowerRelationship
is disabled (empty) in the syspref.

To reproduce:
    1) Go to system preferences and disable borrowerRelationship.
    2) Create a new patron or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button.
    After performing the search, select different patron to act as
    guarantor.
    4) Observe that there's empty dropdown for the relation type.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that dropdown is no longer there.
Comment 12 Andrew Fuerste-Henry 2021-03-30 12:12:20 UTC
Created attachment 118997 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not split the config using comma.

From syspref description of borrowerRelationship preference:

> Guarantors can be the following of those they guarantee:
> (input multiple choices separated by |). Leave empty to deactivate.

As it doesn't mention comma at all, I removed ',' from split.
Of course if comma is actually a viable way to split separate choices,
I can obsolete this patch and append to the syspref description that it
also can be separated by comma.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Andrew Fuerste-Henry 2021-03-30 12:12:23 UTC
Created attachment 118998 [details] [review]
Bug 25946: borrowerRelationship can be empty

When borrowerRelationship is empty in system preferences, Relationship
dropdown is not required and we accept empty value.

Also fixes bug that didn't let you to pick empty value even when you
specified that it should be possible in system preferences but in the
end of the string (i.e. "|father|mother" worked,
but "father|mother|" don't).

To reproduce (borrowerRelationship can be empty):
    1) Go to system preferences and make borrowerRelationship empty.
    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. Try to save your changes.
    4) Observe that relationship field is required in order to save but
    you can't actually choose anything as it doesn't contain anything.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

To reproduce (can't choose empty value bug):
    1) Go to system preferences and set borrowerRelationship exactly
    to "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.
    4) Observe that there's no option to leave relationship field empty.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it has empty option that you can choose and save
    the form.

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 14 Andrew Fuerste-Henry 2021-03-30 12:12:27 UTC
Created attachment 118999 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not show the empty dropdown with no options if borrowerRelationship
is disabled (empty) in the syspref.

To reproduce:
    1) Go to system preferences and disable borrowerRelationship.
    2) Create a new patron or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button.
    After performing the search, select different patron to act as
    guarantor.
    4) Observe that there's empty dropdown for the relation type.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that dropdown is no longer there.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 15 Martin Renvoize 2021-03-30 14:18:07 UTC
Created attachment 119011 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not split the config using comma.

From syspref description of borrowerRelationship preference:

> Guarantors can be the following of those they guarantee:
> (input multiple choices separated by |). Leave empty to deactivate.

As it doesn't mention comma at all, I removed ',' from split.
Of course if comma is actually a viable way to split separate choices,
I can obsolete this patch and append to the syspref description that it
also can be separated by comma.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2021-03-30 14:18:10 UTC
Created attachment 119012 [details] [review]
Bug 25946: borrowerRelationship can be empty

When borrowerRelationship is empty in system preferences, Relationship
dropdown is not required and we accept empty value.

Also fixes bug that didn't let you to pick empty value even when you
specified that it should be possible in system preferences but in the
end of the string (i.e. "|father|mother" worked,
but "father|mother|" don't).

To reproduce (borrowerRelationship can be empty):
    1) Go to system preferences and make borrowerRelationship empty.
    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. Try to save your changes.
    4) Observe that relationship field is required in order to save but
    you can't actually choose anything as it doesn't contain anything.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it allows you to save the form now.

To reproduce (can't choose empty value bug):
    1) Go to system preferences and set borrowerRelationship exactly
    to "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.
    4) Observe that there's no option to leave relationship field empty.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that it has empty option that you can choose and save
    the form.

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

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-03-30 14:18:14 UTC
Created attachment 119013 [details] [review]
Bug 25946: borrowerRelationship can be empty

Do not show the empty dropdown with no options if borrowerRelationship
is disabled (empty) in the syspref.

To reproduce:
    1) Go to system preferences and disable borrowerRelationship.
    2) Create a new patron or modify the existing one.
    3) Under "Guarantor Information" click on "Search to add" button.
    After performing the search, select different patron to act as
    guarantor.
    4) Observe that there's empty dropdown for the relation type.
    5) Apply the patch.
    6) Repeat steps above.
    7) Observe that dropdown is no longer there.

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-03-30 14:48:10 UTC
Created attachment 119015 [details] [review]
Bug 25946: (QA follow-up) Add Unit tests

Add a unit test for the empty relationship availability.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-03-30 14:49:05 UTC
Works well, passes QA script.. I added some unit tests.

Passing QA
Comment 20 Jonathan Druart 2021-04-01 16:53:33 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 21 Fridolin Somers 2021-04-07 09:43:09 UTC
Pushed to 20.11.x for 20.11.05
Comment 22 Andrew Fuerste-Henry 2021-04-24 12:16:33 UTC
Doesn't apply cleanly to 20.05. Please rebase for backport.
Comment 23 Christopher Brannon 2021-12-22 19:28:23 UTC
(In reply to Andrew Fuerste-Henry from comment #22)
> Doesn't apply cleanly to 20.05. Please rebase for backport.

Is this going to be backported?
Comment 24 Andrew Fuerste-Henry 2021-12-22 19:37:05 UTC
(In reply to Christopher Brannon from comment #23)
> (In reply to Andrew Fuerste-Henry from comment #22)
> > Doesn't apply cleanly to 20.05. Please rebase for backport.
> 
> Is this going to be backported?

This isn't going to be backported any further, no. It's in what are now oldstable and oldoldstable.
Comment 25 Christopher Brannon 2021-12-22 21:57:56 UTC
(In reply to Andrew Fuerste-Henry from comment #24)
> (In reply to Christopher Brannon from comment #23)
> > (In reply to Andrew Fuerste-Henry from comment #22)
> > > Doesn't apply cleanly to 20.05. Please rebase for backport.
> > 
> > Is this going to be backported?
> 
> This isn't going to be backported any further, no. It's in what are now
> oldstable and oldoldstable

Got it.  Thanks.