Bug 21596 - Handle default values when storing Koha::Patron
Summary: Handle default values when storing Koha::Patron
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
: 16858 (view as bug list)
Depends on: 20287
Blocks: 21597 21618
  Show dependency treegraph
 
Reported: 2018-10-18 11:14 UTC by Nick Clemens
Modified: 2019-01-02 09:48 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 21596: Don't set a default of "" for NULL columns during patron import (1.30 KB, patch)
2018-10-18 11:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21596: Handle empty string for date_renewed when storing a patron (1.64 KB, patch)
2018-10-18 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: Handle empty string for flags when storing a patron (911 bytes, patch)
2018-10-18 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: Handle empty string for other attributes when storing a patron (1.83 KB, patch)
2018-10-18 11:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: Handle empty string for lost when storing a patron (904 bytes, patch)
2018-10-18 11:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: Handle empty string for date_renewed when storing a patron (1.68 KB, patch)
2018-10-18 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21596: Handle empty string for flags when storing a patron (961 bytes, patch)
2018-10-18 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21596: Handle empty string for other attributes when storing a patron (1.88 KB, patch)
2018-10-18 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21596: Handle empty string for lost when storing a patron (954 bytes, patch)
2018-10-18 13:20 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 21596: Handle empty string for date_renewed when storing a patron (1.76 KB, patch)
2018-10-18 13:52 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21596: Handle empty string for flags when storing a patron (1.01 KB, patch)
2018-10-18 13:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21596: Handle empty string for other attributes when storing a patron (1.95 KB, patch)
2018-10-18 13:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21596: Handle empty string for lost when storing a patron (1.00 KB, patch)
2018-10-18 13:53 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21596: Handle updated_on (1.41 KB, patch)
2018-10-24 17:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: (follow-up) Handle updated_on (1.18 KB, patch)
2018-10-25 13:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21596: (follow-up 2) Handle updated_on (1.20 KB, patch)
2018-10-29 20:29 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2018-10-18 11:14:06 UTC
Some columns require integer values,other are datetimes - we should not set these to a default of "" during import when they are undefined
Comment 1 Nick Clemens 2018-10-18 11:17:49 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
Comment 2 Jonathan Druart 2018-10-18 11:56:46 UTC
Created attachment 80807 [details] [review]
Bug 21596: Handle empty string for date_renewed when storing a patron

Incorrect date value: '' for column 'date_renewed'
Comment 3 Jonathan Druart 2018-10-18 11:56:51 UTC
Created attachment 80808 [details] [review]
Bug 21596: Handle empty string for flags when storing a patron

Incorrect integer value: '' for column 'flags'
Comment 4 Jonathan Druart 2018-10-18 11:56:55 UTC
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
Comment 5 Jonathan Druart 2018-10-18 11:57:00 UTC
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
Comment 6 Jonathan Druart 2018-10-18 11:59:30 UTC
We certainly want/need to fix these issues at Koha::Patron level instead, to make sure we cover all insert cases.
Comment 7 Nick Clemens 2018-10-18 12:49:10 UTC
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
Comment 8 Nick Clemens 2018-10-18 13:20:11 UTC
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>
Comment 9 Nick Clemens 2018-10-18 13:20:15 UTC
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>
Comment 10 Nick Clemens 2018-10-18 13:20:19 UTC
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>
Comment 11 Nick Clemens 2018-10-18 13:20:23 UTC
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>
Comment 12 Martin Renvoize 2018-10-18 13:52:57 UTC
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>
Comment 13 Martin Renvoize 2018-10-18 13:53:01 UTC
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>
Comment 14 Martin Renvoize 2018-10-18 13:53:05 UTC
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>
Comment 15 Martin Renvoize 2018-10-18 13:53:10 UTC
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>
Comment 16 Martin Renvoize 2018-10-18 13:53:50 UTC
I've worked through these now too.. looking good.

Passed QA
Comment 17 Nick Clemens 2018-10-18 14:32:26 UTC
Pushed to master for 18.11
Comment 18 Jonathan Druart 2018-10-24 16:37:54 UTC
*** Bug 16858 has been marked as a duplicate of this bug. ***
Comment 19 Jonathan Druart 2018-10-24 17:37:36 UTC
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
Comment 20 Jonathan Druart 2018-10-24 17:38:24 UTC
Please review and push this last patch to make Jenkins happy.
Comment 21 Martin Renvoize 2018-10-25 12:27:10 UTC
I agree with this followup patch for now.. it makes sense to get the tests green again
Comment 22 Nick Clemens 2018-10-25 12:38:45 UTC
(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
Comment 23 Jonathan Druart 2018-10-25 13:26:12 UTC
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*
Comment 24 Nick Clemens 2018-10-25 17:06:57 UTC
(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 :-)
Comment 25 Jonathan Druart 2018-10-29 20:29:40 UTC
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>
Comment 26 Martin Renvoize 2018-11-05 12:16:40 UTC
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.