Bug 23151

Summary: Patron self modification sends null dateofbirth
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: OPACAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: blocker    
Priority: P5 - low CC: bhenry, cbrannon, eveal, fridolin.somers, george, jdemuth, jonathan.druart, kyle, lucas, m.de.rooy, margaret, mtompset, nick, wizzyrea
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23431
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25769
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.11.00,19.05.02,18.11.08
Bug Depends on: 11853    
Bug Blocks: 23428, 23579    
Attachments: Bug 23151: Properly clear/not clear date of birth.
Bug 23151: Add tests
Bug 23151: Properly clear/not clear date of birth.
Bug 23151: Add tests
Bug 23151: Properly clear/not clear date of birth.
Bug 23151: Add tests
Bug 23151: Add tests
Bug 23151: Modifiy database structure
Bug 23151: Schema changes
Bug 23151: Tweak to use the new database structure
Bug 23151: Add tests
Bug 23151: (follow-up) tweaked tests
Bug 23151: (QA follow-up) Fix atomic update
Bug 23151: Modifiy database structure
Bug 23151: Schema changes
Bug 23151: Tweak to use the new database structure
Bug 23151: Add tests
Bug 23151: (follow-up) tweaked tests
Bug 23151: (QA follow-up) Fix atomic update
Bug 23151: Modifiy database structure
Bug 23151: Schema changes
Bug 23151: Tweak to use the new database structure
Bug 23151: Add tests
Bug 23151: (follow-up) tweaked tests
Bug 23151: (QA follow-up) Fix atomic update
Bug 23151: (QA follow-up) Remove debug leftover

Description Andrew Fuerste-Henry 2019-06-18 18:41:01 UTC
To Test:
- confirm OPACPatronDetails is set to Allow
- log into opac
- submit change request without changing date of birth
- look at change request on staff, see that request includes removal of date of birth

I've confirm this bug occurs regardless of whether or not dateofbirth is included in either PatronSelfRegistrationBorrowerMandatoryField or BorrowerUnwantedField.

Bug exists in 18.11.06 but not in 18.11.05
Comment 1 Nick Clemens 2019-06-18 19:05:35 UTC
Caused by commit 6692800794cbf403e8be8c8062608b60148c0d56

Raising severity as can result in data loss
Comment 2 Mark Tompsett 2019-06-19 01:50:20 UTC
Created attachment 90764 [details] [review]
Bug 23151: Properly clear/not clear date of birth.

The problem is caused by how change is stored: {changed value} || NULL.
NULL means no change, but in the case of clearing a date of birth
one wants to set it to NULL, which is indistinguishable from the
no change value.

By adding a DUMMY_DATE '9999-12-31', and using it as an intermediate
check, we can set the value to NULL. This required storing the
DUMMY_DATE as the change value, then in the case of date of birth
displaying blank when listing the changes, and saving undef when
the changes are stored.

TEST PLAN
---------
0) running this test plan without the patch results in bad data.
1) Ensure the Global System Preference OPACPatronDetails is set to Allow.
2) reset_all in the kohadevbox.
3) log into admin/admin.
4) open up localhost:8080 in one tab, and localhost:8081 in another.
5) In the opac tab, click on the user name, and then the 'your personal
details' tab.
5a) Set the first name, so it won't complain.
6) Set the salutation to Mr. and the birth date to any date.
7) scroll down and submit
8) In the staff tab, click home
9) click on the 'patrons requesting modifications' at the bottom of the screen.
-- you should see the changes you just made.
10) click the 'Approve' radio button, and click Submit.
11) In the opac tab, click on the 'your personal details' tab.
-- the values should be as expected
12) Set the salutation to blank and leave the birth date alone.
13) scroll down and submit
14) repeat steps 8-11.
-- the values should be as expected
15) Clear the birth date
16) scroll down and submit
17) repeat steps 8-11.
18) Set the salutation to Mr. and leave the birth date blank.
19) scroll down and submit.
20) repeat steps 8-11.
21) Repeat steps 6-11 for good measure.
Comment 3 Jonathan Druart 2019-06-19 03:40:00 UTC
Is this a bug in master as well or a problem with the backport?
Comment 4 Jonathan Druart 2019-06-19 03:42:18 UTC
(In reply to Jonathan Druart from comment #3)
> Is this a bug in master as well or a problem with the backport?

Confirmed on master.
Comment 5 Jonathan Druart 2019-06-19 03:55:52 UTC
Created attachment 90769 [details] [review]
Bug 23151: Add tests
Comment 6 Jonathan Druart 2019-06-19 03:56:43 UTC
I have tried to suggest a counter patch as I do not like the "dummy value", but failed.
However I wrote tests before, and they do not pass. Also, a lot of warnings are generated.
Comment 7 Mark Tompsett 2019-06-19 14:54:30 UTC
Created attachment 90795 [details] [review]
Bug 23151: Properly clear/not clear date of birth.

The problem is caused by how change is stored: {changed value} || NULL.
NULL means no change, but in the case of clearing a date of birth
one wants to set it to NULL, which is indistinguishable from the
no change value.

By adding a DUMMY_DATE '9999-12-31', and using it as an intermediate
check, we can set the value to NULL. This required storing the
DUMMY_DATE as the change value, then in the case of date of birth
displaying blank when listing the changes, and saving undef when
the changes are stored.

TEST PLAN
---------
0) running this test plan without the patch results in bad data.
1) Ensure the Global System Preference OPACPatronDetails is set to Allow.
2) reset_all in the kohadevbox.
3) log into admin/admin.
4) open up localhost:8080 in one tab, and localhost:8081 in another.
5) In the opac tab, click on the user name, and then the 'your personal
details' tab.
5a) Set the first name, so it won't complain.
6) Set the salutation to Mr. and the birth date to any date.
7) scroll down and submit
8) In the staff tab, click home
9) click on the 'patrons requesting modifications' at the bottom of the screen.
-- you should see the changes you just made.
10) click the 'Approve' radio button, and click Submit.
11) In the opac tab, click on the 'your personal details' tab.
-- the values should be as expected
12) Set the salutation to blank and leave the birth date alone.
13) scroll down and submit
14) repeat steps 8-11.
-- the values should be as expected
15) Clear the birth date
16) scroll down and submit
17) repeat steps 8-11.
18) Set the salutation to Mr. and leave the birth date blank.
19) scroll down and submit.
20) repeat steps 8-11.
21) Repeat steps 6-11 for good measure.
Comment 8 Mark Tompsett 2019-06-19 14:54:33 UTC
Created attachment 90796 [details] [review]
Bug 23151: Add tests
Comment 9 Lucas Gass 2019-06-19 15:02:21 UTC
Created attachment 90797 [details] [review]
Bug 23151: Properly clear/not clear date of birth.

The problem is caused by how change is stored: {changed value} || NULL.
NULL means no change, but in the case of clearing a date of birth
one wants to set it to NULL, which is indistinguishable from the
no change value.

By adding a DUMMY_DATE '9999-12-31', and using it as an intermediate
check, we can set the value to NULL. This required storing the
DUMMY_DATE as the change value, then in the case of date of birth
displaying blank when listing the changes, and saving undef when
the changes are stored.

TEST PLAN
---------
0) running this test plan without the patch results in bad data.
1) Ensure the Global System Preference OPACPatronDetails is set to Allow.
2) reset_all in the kohadevbox.
3) log into admin/admin.
4) open up localhost:8080 in one tab, and localhost:8081 in another.
5) In the opac tab, click on the user name, and then the 'your personal
details' tab.
5a) Set the first name, so it won't complain.
6) Set the salutation to Mr. and the birth date to any date.
7) scroll down and submit
8) In the staff tab, click home
9) click on the 'patrons requesting modifications' at the bottom of the screen.
-- you should see the changes you just made.
10) click the 'Approve' radio button, and click Submit.
11) In the opac tab, click on the 'your personal details' tab.
-- the values should be as expected
12) Set the salutation to blank and leave the birth date alone.
13) scroll down and submit
14) repeat steps 8-11.
-- the values should be as expected
15) Clear the birth date
16) scroll down and submit
17) repeat steps 8-11.
18) Set the salutation to Mr. and leave the birth date blank.
19) scroll down and submit.
20) repeat steps 8-11.
21) Repeat steps 6-11 for good measure.

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 10 Lucas Gass 2019-06-19 15:02:38 UTC
Created attachment 90798 [details] [review]
Bug 23151: Add tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 11 ByWater Sandboxes 2019-06-19 15:12:15 UTC
Created attachment 90799 [details] [review]
Bug 23151: Add tests

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Ed Veal <eveal@mckinneytexas.org>
Comment 12 Mark Tompsett 2019-06-19 16:37:13 UTC
Created attachment 90805 [details] [review]
Bug 23151: Modifiy database structure

This modifies a fresh install structure, and provides
an atomic update.
Comment 13 Mark Tompsett 2019-06-19 16:37:15 UTC
Created attachment 90806 [details] [review]
Bug 23151: Schema changes

It is best to regenerate these from scratch.
On a kohadevbox:
$ misc/devel/update_dbix_class_files.pl --db_name=koha_kohadev --db_user=koha_kohadev --db_passwd=password
Comment 14 Mark Tompsett 2019-06-19 16:37:18 UTC
Created attachment 90807 [details] [review]
Bug 23151: Tweak to use the new database structure

This will use changed_fields to know whether a
borrower field is actually modified.
Comment 15 Mark Tompsett 2019-06-19 16:37:21 UTC
Created attachment 90808 [details] [review]
Bug 23151: Add tests
Comment 16 Mark Tompsett 2019-06-19 16:37:24 UTC
Created attachment 90809 [details] [review]
Bug 23151: (follow-up) tweaked tests

This forces the tests to use the new changed_fields
field.
Comment 17 Mark Tompsett 2019-06-19 16:42:30 UTC
Sorry, I forgot to change the status before Lucas and Ed signed off. DOH! Feel free to try again.
Comment 18 Jonathan Druart 2019-06-19 17:31:31 UTC
Mark, what about the update process? We need to have an atomic update file and update the existing rows in DB.
Comment 19 Mark Tompsett 2019-06-19 17:46:18 UTC
It's in the first patch as a .perl, Jonathan. :)
Comment 20 Jonathan Druart 2019-06-19 18:06:58 UTC
(In reply to M. Tompsett from comment #19)
> It's in the first patch as a .perl, Jonathan. :)

Sorry, I missed it.

However the column need to be set for existing installations.
Comment 21 Kyle M Hall 2019-06-28 14:15:50 UTC
Created attachment 91070 [details] [review]
Bug 23151: (QA follow-up) Fix atomic update
Comment 22 Kyle M Hall 2019-06-28 14:21:01 UTC
Created attachment 91071 [details] [review]
Bug 23151: Modifiy database structure

This modifies a fresh install structure, and provides
an atomic update.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2019-06-28 14:21:10 UTC
Created attachment 91072 [details] [review]
Bug 23151: Schema changes

It is best to regenerate these from scratch.
On a kohadevbox:
$ misc/devel/update_dbix_class_files.pl --db_name=koha_kohadev --db_user=koha_kohadev --db_passwd=password

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2019-06-28 14:21:13 UTC
Created attachment 91073 [details] [review]
Bug 23151: Tweak to use the new database structure

This will use changed_fields to know whether a
borrower field is actually modified.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2019-06-28 14:21:16 UTC
Created attachment 91074 [details] [review]
Bug 23151: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2019-06-28 14:21:19 UTC
Created attachment 91075 [details] [review]
Bug 23151: (follow-up) tweaked tests

This forces the tests to use the new changed_fields
field.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2019-06-28 14:21:22 UTC
Created attachment 91076 [details] [review]
Bug 23151: (QA follow-up) Fix atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Jonathan Druart 2019-06-28 16:52:37 UTC
(In reply to Jonathan Druart from comment #20)
> However the column need to be set for existing installations.

Still valid, I do not think we should push it without updating the existing entries.
Comment 29 Kyle M Hall 2019-07-17 14:26:54 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Jonathan Druart from comment #20)
> > However the column need to be set for existing installations.
> 
> Still valid, I do not think we should push it without updating the existing
> entries.

I don't believe this update is even possible given we don't have the data we'd need to fix it, and if we did we wouldn't have this bug to begin with ;)

In addition, this is a serious problem that is happening *now*. I think we need to get this patched post-haste and we can solve this db update issue on followup report. Right now libraries are losing data that they wouldn't not have lost if this hadn't been set to failed qa.
Comment 30 Liz Rea 2019-07-17 14:28:31 UTC
Hi Jonathan,

This is causing pretty severe data loss that affects libraries ability to do things like change patron categories based on birth date - if a patron loses their DOB, that process can no longer happen.

I propose we let this code through to fix the potential for data loss, and follow it up with a solution that potentially checks the pending modifications at upgrade time for DOB, and if the patron has one, insert that into the pending modification assuming that the patron didn't mean to null their birthdate. This could, of course, err on the side of the library (the patron *might* have meant to delete their DOB, yes. They can always process that modification again if they find it to be incorrect.)

This really is a serious problem for libraries with automatic age based category management, and I don't think we can let it sit. The current solution fixes the data loss problem, and there will be a follow up to fix the currently pending at time of upgrade modifications, but I don't see why they have to go in together.

I'm happy to file another bug for that 2nd part, if you like, or we can do it on this bug, but I do feel this bug needs to get moving on through.

Cheers,
Liz
Comment 31 Ed Veal 2019-07-17 16:24:25 UTC
I agree with Liz, if we have a patch to solve the problem moving forward let's get that out there. As we address the issues around the data loss. 

Ed
Comment 32 Marcel de Rooy 2019-07-19 06:54:32 UTC
QA: Looking here now
Comment 33 Marcel de Rooy 2019-07-19 08:06:19 UTC
Created attachment 91614 [details] [review]
Bug 23151: Modifiy database structure

This modifies a fresh install structure, and provides
an atomic update.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2019-07-19 08:06:24 UTC
Created attachment 91615 [details] [review]
Bug 23151: Schema changes

It is best to regenerate these from scratch.
On a kohadevbox:
$ misc/devel/update_dbix_class_files.pl --db_name=koha_kohadev --db_user=koha_kohadev --db_passwd=password

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2019-07-19 08:06:30 UTC
Created attachment 91616 [details] [review]
Bug 23151: Tweak to use the new database structure

This will use changed_fields to know whether a
borrower field is actually modified.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2019-07-19 08:06:36 UTC
Created attachment 91617 [details] [review]
Bug 23151: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2019-07-19 08:06:41 UTC
Created attachment 91618 [details] [review]
Bug 23151: (follow-up) tweaked tests

This forces the tests to use the new changed_fields
field.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2019-07-19 08:06:46 UTC
Created attachment 91619 [details] [review]
Bug 23151: (QA follow-up) Fix atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2019-07-19 08:06:51 UTC
Created attachment 91620 [details] [review]
Bug 23151: (QA follow-up) Remove debug leftover

Removes: Running... Running... Ran

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Martin Renvoize 2019-07-19 09:10:13 UTC
Nice work!

Pushed to master for 19.11.00
Comment 41 Ed Veal 2019-07-21 15:53:03 UTC
Great work folks, can this be back ported for those of us that need the fix now? 

Ed
Comment 42 Lucas Gass 2019-07-23 15:16:04 UTC
backported to 19.05.x for 19.05.02
Comment 43 Lucas Gass 2019-07-23 15:16:24 UTC
backported to 18.11.x for 18.11.08
Comment 44 Liz Rea 2019-08-05 14:35:03 UTC
It appears this patch isn't commpatible with hiding the date of birth using  PatronSelfModificationBorrowerUnwantedField - if the field isn't on the screen, it's still going to blank the DOB.
Comment 45 Jonathan Druart 2019-08-05 19:27:53 UTC
(In reply to Kyle M Hall from comment #29)
> (In reply to Jonathan Druart from comment #28)
> > (In reply to Jonathan Druart from comment #20)
> > > However the column need to be set for existing installations.
> > 
> > Still valid, I do not think we should push it without updating the existing
> > entries.
> 
> I don't believe this update is even possible given we don't have the data
> we'd need to fix it, and if we did we wouldn't have this bug to begin with ;)
> 
> In addition, this is a serious problem that is happening *now*. I think we
> need to get this patched post-haste and we can solve this db update issue on
> followup report. Right now libraries are losing data that they wouldn't not
> have lost if this hadn't been set to failed qa.

% git checkout before these patches
% Create patron's modifications
% git checkout master
% Approve the patron's modifications
=> The modifications are lost.

So we replaced a data lost, with another...

That's why I was insisting.
Comment 46 Christopher Brannon 2020-06-12 15:03:14 UTC
This bug seems to be back in 19.11!
Comment 47 Katrin Fischer 2020-06-13 18:17:07 UTC
(In reply to Christopher Brannon from comment #46)
> This bug seems to be back in 19.11!

Can you please file a new bug with a link to this one?
Comment 48 Kyle M Hall 2020-06-16 11:09:35 UTC
(In reply to Christopher Brannon from comment #46)
> This bug seems to be back in 19.11!

FYI, I was unable to reproduce on master. Will try 19.11 next.
Comment 49 Kyle M Hall 2020-06-16 11:24:04 UTC
(In reply to Kyle M Hall from comment #48)
> (In reply to Christopher Brannon from comment #46)
> > This bug seems to be back in 19.11!
> 
> FYI, I was unable to reproduce on master. Will try 19.11 next.

Unable to reproduce on 19.11.x either.
Comment 50 Kyle M Hall 2020-06-16 11:37:51 UTC
(In reply to Kyle M Hall from comment #49)
> (In reply to Kyle M Hall from comment #48)
> > (In reply to Christopher Brannon from comment #46)
> > > This bug seems to be back in 19.11!
> > 
> > FYI, I was unable to reproduce on master. Will try 19.11 next.
> 
> Unable to reproduce on 19.11.x either.

Found it! This bug *requires* dateofbirth in BorrowerUnwantedField which is clearly *not* the same as this bug.