Bug 17080 - Koha::Object->new should handle default values for NOT NULL columns
Summary: Koha::Object->new should handle default values for NOT NULL columns
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 16917 16960 17069
Blocks:
  Show dependency treegraph
 
Reported: 2016-08-08 13:32 UTC by Jonathan Druart
Modified: 2017-06-14 22:15 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17080: Handle default values for NOT NULL columns from Koha::Object->new (3.66 KB, patch)
2016-08-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17080: Make C4::Members::AddMember uses Koha::Patron->store (1.21 KB, patch)
2016-08-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17080: borrowers table - use default values defined in the DBIx::Class schema (1.27 KB, patch)
2016-08-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17080: borrowers.checkprevcheckout - use the default value defined in the DBIx::Class schema (1.25 KB, patch)
2016-08-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17080: categories.checkprevcheckout - use the default value defined in the DBIx::Class schema (996 bytes, patch)
2016-08-08 13:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17080: Handle default values for NOT NULL columns from Koha::Object->new (3.84 KB, patch)
2016-08-15 12:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17080: Make C4::Members::AddMember uses Koha::Patron->store (1.35 KB, patch)
2016-08-15 12:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17080: borrowers table - use default values defined in the DBIx::Class schema (1.40 KB, patch)
2016-08-15 12:41 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17080: borrowers.checkprevcheckout - use the default value defined in the DBIx::Class schema (1.38 KB, patch)
2016-08-15 12:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17080: categories.checkprevcheckout - use the default value defined in the DBIx::Class schema (1.10 KB, patch)
2016-08-15 12:43 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17080: Handle default values for NOT NULL columns from Koha::Object->new (4.00 KB, patch)
2016-10-14 08:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17080: Make C4::Members::AddMember uses Koha::Patron->store (1.40 KB, patch)
2016-10-14 08:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17080: borrowers table - use default values defined in the DBIx::Class schema (1.46 KB, patch)
2016-10-14 08:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17080: borrowers.checkprevcheckout - use the default value defined in the DBIx::Class schema (1.43 KB, patch)
2016-10-14 08:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17080: categories.checkprevcheckout - use the default value defined in the DBIx::Class schema (1.16 KB, patch)
2016-10-14 08:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17080 [QA Followup] - Fix number of tests (640 bytes, patch)
2016-10-21 17:34 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-08-08 13:32:43 UTC
Recently we face the same issue on different modules after we moved them to the Koha namespace using Koha::Object of using DBIx::Class directly.

1/ Koha::Patron::Modification on bug 16960 comment 14 and 15
2/ Koha::Patron::Category from bug 17069
3/ C4::Members::AddMember (which does not use Koha::Object) on bug 16917

If a DB column is defined as NOT NULL and has a default value, the DBIx::Class $rs->update_or_insert method won't use the default value if the column name has been passed to the constructor.

We do that almost everywhere as we retrieve the data from the HTML forms without checking/cleaning them.

There are several ways to fix that:
1/ Continue to fix them case by case (what we did for the recent issues)
2/ Try to fix them globally (existing ones and the next ones)

This report will propose a global solution to avoid future issues of this kind.
Comment 1 Jonathan Druart 2016-08-08 13:59:28 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-08-08 13:59:31 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-08-08 13:59:39 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-08-08 13:59:42 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2016-08-08 13:59:45 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2016-08-12 04:52:38 UTC
I believe this needs rebasing
Comment 7 Jonathan Druart 2016-08-12 08:20:12 UTC
(In reply to Srdjan Jankovic from comment #6)
> I believe this needs rebasing

It applies cleanly on top of bug 16960.
Comment 8 Srdjan Jankovic 2016-08-15 00:51:13 UTC
Sorry, my bad, missed that one.
Comment 9 Marc Véron 2016-08-15 12:39:52 UTC
Created attachment 54465 [details] [review]
Bug 17080: Handle default values for NOT NULL columns from Koha::Object->new

Recently we face the same issue on different modules after we moved them to the
Koha namespace using Koha::Object of using DBIx::Class directly.

1/ Koha::Patron::Modification on bug 16960 comment 14 and 15
2/ Koha::Patron::Category from bug 17069
3/ C4::Members::AddMember (which does not use Koha::Object) on bug 16917

If a DB column is defined as NOT NULL and has a default value, the DBIx::Class
$rs->update_or_insert method won't use the default value if the column name
has been passed to the constructor.

We do that almost everywhere as we retrieve the data from the HTML forms without
checking/cleaning them.

There are several ways to fix that:
1/ Continue to fix them case by case (what we did for the recent issues)
2/ Try to fix them globally (existing ones and the next ones)

This patch propose a global solution to avoid future issues of this kind.

The idea is not to pass the undefined values which cannot be nullable to the
DBIx::Class constructor.

Tested all patches together. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Marc Véron 2016-08-15 12:40:50 UTC
Created attachment 54466 [details] [review]
Bug 17080: Make C4::Members::AddMember uses Koha::Patron->store

These calls have been added from commit e196f19 (Bug 12633: Remove SQLHelper
in C4::Members) which added a TODO:
"This patch replaced the calls to SQLHelper to use DBIX::Class.
TODO: Move them to Koha::Borrower."

This is now done by this patch: instead of calling DBIx::Class directly
we use Koha::Patron

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 11 Marc Véron 2016-08-15 12:41:39 UTC
Created attachment 54467 [details] [review]
Bug 17080: borrowers table - use default values defined in the DBIx::Class schema

This patch basically just revert bug 16960 - Fix error on validating the
registration

This job is now done by the Koha::Object constructor, no need to clean
the hash before sending it to AddMember.

Test plan:
Make sure there is no regression on bug 16960 (validate a self registration).

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2016-08-15 12:42:27 UTC
Created attachment 54468 [details] [review]
Bug 17080: borrowers.checkprevcheckout - use the default value defined in the DBIx::Class schema

This line has been added by bug 16917 to define the default value of
borrowers.checkprevcheckout.
As this value is defined in the DBIx::Class schema, we should not have
to handle it here.

Test plan:
Confirm that this patch does not reintroduced the bug fixed by bug
16917.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 13 Marc Véron 2016-08-15 12:43:19 UTC
Created attachment 54469 [details] [review]
Bug 17080: categories.checkprevcheckout - use the default value defined in the DBIx::Class schema

This patch reverts bug 17069

Test plan:
Create a new patron category

  prove t/db_dependent/Koha/Patron/Categories.t
should return green

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 14 Nick Clemens 2016-10-14 08:14:22 UTC
Created attachment 56464 [details] [review]
Bug 17080: Handle default values for NOT NULL columns from Koha::Object->new

Recently we face the same issue on different modules after we moved them to the
Koha namespace using Koha::Object of using DBIx::Class directly.

1/ Koha::Patron::Modification on bug 16960 comment 14 and 15
2/ Koha::Patron::Category from bug 17069
3/ C4::Members::AddMember (which does not use Koha::Object) on bug 16917

If a DB column is defined as NOT NULL and has a default value, the DBIx::Class
$rs->update_or_insert method won't use the default value if the column name
has been passed to the constructor.

We do that almost everywhere as we retrieve the data from the HTML forms without
checking/cleaning them.

There are several ways to fix that:
1/ Continue to fix them case by case (what we did for the recent issues)
2/ Try to fix them globally (existing ones and the next ones)

This patch propose a global solution to avoid future issues of this kind.

The idea is not to pass the undefined values which cannot be nullable to the
DBIx::Class constructor.

Tested all patches together. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2016-10-14 08:14:28 UTC
Created attachment 56465 [details] [review]
Bug 17080: Make C4::Members::AddMember uses Koha::Patron->store

These calls have been added from commit e196f19 (Bug 12633: Remove SQLHelper
in C4::Members) which added a TODO:
"This patch replaced the calls to SQLHelper to use DBIX::Class.
TODO: Move them to Koha::Borrower."

This is now done by this patch: instead of calling DBIx::Class directly
we use Koha::Patron

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens 2016-10-14 08:14:33 UTC
Created attachment 56466 [details] [review]
Bug 17080: borrowers table - use default values defined in the DBIx::Class schema

This patch basically just revert bug 16960 - Fix error on validating the
registration

This job is now done by the Koha::Object constructor, no need to clean
the hash before sending it to AddMember.

Test plan:
Make sure there is no regression on bug 16960 (validate a self registration).

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2016-10-14 08:14:38 UTC
Created attachment 56467 [details] [review]
Bug 17080: borrowers.checkprevcheckout - use the default value defined in the DBIx::Class schema

This line has been added by bug 16917 to define the default value of
borrowers.checkprevcheckout.
As this value is defined in the DBIx::Class schema, we should not have
to handle it here.

Test plan:
Confirm that this patch does not reintroduced the bug fixed by bug
16917.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Nick Clemens 2016-10-14 08:14:43 UTC
Created attachment 56468 [details] [review]
Bug 17080: categories.checkprevcheckout - use the default value defined in the DBIx::Class schema

This patch reverts bug 17069

Test plan:
Create a new patron category

  prove t/db_dependent/Koha/Patron/Categories.t
should return green

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Nick Clemens 2016-10-14 08:15:24 UTC
Cleared a small conflict in tests
Comment 20 Martin Renvoize 2016-10-18 05:48:58 UTC
Looks reasonably to me. Logic is all sound, we're making sure it only affects non-nullable columns. I can't see any problems.  I'd also have passed qa if someone hadn't beaten me to it ;)
Comment 21 Kyle M Hall 2016-10-21 17:34:40 UTC
Created attachment 56728 [details] [review]
Bug 17080 [QA Followup] - Fix number of tests
Comment 22 Kyle M Hall 2016-10-21 17:35:16 UTC
Pushed to master for 16.11, thanks Jonathan!