Description
Nick Clemens (kidclamp)
2018-10-18 11:14:06 UTC
Created attachment 80804 [details] [review] Bug 21596: Don't set a default of "" for NULL columns during patron import To test: 1 - dev_install must be set to 1 2 - prove -v t/db_dependent/Koha/Patrons/Import.t 3 - It fails 4 - Apply patch 5 - It passes Created attachment 80807 [details] [review] Bug 21596: Handle empty string for date_renewed when storing a patron Incorrect date value: '' for column 'date_renewed' Created attachment 80808 [details] [review] Bug 21596: Handle empty string for flags when storing a patron Incorrect integer value: '' for column 'flags' Created attachment 80809 [details] [review] Bug 21596: Handle empty string for other attributes when storing a patron lastseen, updated_on => dates gonenoaddress, login_attempts, privacy_guarantor_checkouts => [tiny]int Created attachment 80810 [details] [review] Bug 21596: Handle empty string for lost when storing a patron lost - should default to 0 at DB level, not null But let fix it as it for now We certainly want/need to fix these issues at Koha::Patron level instead, to make sure we cover all insert cases. Test plan: 1 - dev_install must be set to 1 2 - prove -v t/db_dependent/Koha/Patrons/Import.t 3 - It fails 4 - Apply patch 5 - It passes 6 - Test importing patrons, it should work Simple way to import patrons (backup your db first please or let it go): 1 - Run a SQL report "SELECT * FROM borrowers" 2 - Save as csv 3 - Remove the borrowernumber column 4 - Import the file, patrons should match 5 - On the DB delete * FROM borrowers 6 - Import the file, should get your patrons back Created attachment 80837 [details] [review] Bug 21596: Handle empty string for date_renewed when storing a patron Incorrect date value: '' for column 'date_renewed' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 80838 [details] [review] Bug 21596: Handle empty string for flags when storing a patron Incorrect integer value: '' for column 'flags' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 80839 [details] [review] Bug 21596: Handle empty string for other attributes when storing a patron lastseen, updated_on => dates gonenoaddress, login_attempts, privacy_guarantor_checkouts => [tiny]int Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 80840 [details] [review] Bug 21596: Handle empty string for lost when storing a patron lost - should default to 0 at DB level, not null But let fix it as it for now Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 80844 [details] [review] Bug 21596: Handle empty string for date_renewed when storing a patron Incorrect date value: '' for column 'date_renewed' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 80845 [details] [review] Bug 21596: Handle empty string for flags when storing a patron Incorrect integer value: '' for column 'flags' Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 80846 [details] [review] Bug 21596: Handle empty string for other attributes when storing a patron lastseen, updated_on => dates gonenoaddress, login_attempts, privacy_guarantor_checkouts => [tiny]int Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 80847 [details] [review] Bug 21596: Handle empty string for lost when storing a patron lost - should default to 0 at DB level, not null But let fix it as it for now Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> I've worked through these now too.. looking good. Passed QA Pushed to master for 18.11 *** Bug 16858 has been marked as a duplicate of this bug. *** Created attachment 81080 [details] [review] Bug 21596: Handle updated_on This has been picked from the solution proposed on bug 21610. It may not be pushed soon and so we need a fix for the failing tests (t/Auth_with_shibboleth.t) Test plan: prove -r t/Auth_with_shibboleth.t t/db_dependent/Koha/Patrons* must return green Please review and push this last patch to make Jenkins happy. I agree with this followup patch for now.. it makes sense to get the tests green again (In reply to Jonathan Druart from comment #19) > Created attachment 81080 [details] [review] [review] > Bug 21596: Handle updated_on > > This has been picked from the solution proposed on bug 21610. It may not > be pushed soon and so we need a fix for the failing tests > (t/Auth_with_shibboleth.t) > > Test plan: > prove -r t/Auth_with_shibboleth.t t/db_dependent/Koha/Patrons* > must return green Pushed to master for 18.11 Created attachment 81160 [details] [review] Bug 21596: (follow-up) Handle updated_on Default value should only used if not exist. We got a failing test with the previous patch: # Failed test 'borrowers.updated_on should have been kept to what we set on creating' # at t/db_dependent/Patrons.t line 78. # got: '1' # expected: '0' Test plan: prove -r t/Auth_with_shibboleth.t t/db_dependent/Patrons.t t/db_dependent/Koha/Patron* (In reply to Jonathan Druart from comment #23) > Created attachment 81160 [details] [review] [review] > Bug 21596: (follow-up) Handle updated_on > > Default value should only used if not exist. > We got a failing test with the previous patch: > # Failed test 'borrowers.updated_on should have been kept to what we set > on creating' > # at t/db_dependent/Patrons.t line 78. > # got: '1' > # expected: '0' > > Test plan: > prove -r t/Auth_with_shibboleth.t t/db_dependent/Patrons.t > t/db_dependent/Koha/Patron* Pushed to master, phew :-) Created attachment 81537 [details] [review] Bug 21596: (follow-up 2) Handle updated_on It seems that I have over complicated things here, now t/db_dependent/Koha/Object.t is failing Apparently everything is ok with this change. Test plan: prove -r prove -r t/db_dependent/Koha/Object.t t/Auth_with_shibboleth.t t/db_dependent/Patrons.t t/db_dependent/Koha/Patron* Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> This depends on 20287 and would need pretty significant work to apply to 18.05.x series... I'm not sure the benefit outweighs the work involved. |