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
Caused by commit 6692800794cbf403e8be8c8062608b60148c0d56 Raising severity as can result in data loss
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.
Is this a bug in master as well or a problem with the backport?
(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.
Created attachment 90769 [details] [review] Bug 23151: Add tests
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.
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.
Created attachment 90796 [details] [review] Bug 23151: Add tests
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>
Created attachment 90798 [details] [review] Bug 23151: Add tests Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
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>
Created attachment 90805 [details] [review] Bug 23151: Modifiy database structure This modifies a fresh install structure, and provides an atomic update.
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
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.
Created attachment 90808 [details] [review] Bug 23151: Add tests
Created attachment 90809 [details] [review] Bug 23151: (follow-up) tweaked tests This forces the tests to use the new changed_fields field.
Sorry, I forgot to change the status before Lucas and Ed signed off. DOH! Feel free to try again.
Mark, what about the update process? We need to have an atomic update file and update the existing rows in DB.
It's in the first patch as a .perl, Jonathan. :)
(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.
Created attachment 91070 [details] [review] Bug 23151: (QA follow-up) Fix atomic update
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>
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>
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>
Created attachment 91074 [details] [review] Bug 23151: Add tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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>
Created attachment 91076 [details] [review] Bug 23151: (QA follow-up) Fix atomic update Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(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.
(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.
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
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
QA: Looking here now
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>
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>
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>
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>
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>
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>
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>
Nice work! Pushed to master for 19.11.00
Great work folks, can this be back ported for those of us that need the fix now? Ed
backported to 19.05.x for 19.05.02
backported to 18.11.x for 18.11.08
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.
(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.
This bug seems to be back in 19.11!
(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?
(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.
(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.
(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.