[Bug 17080] New: Koha::Object-> new should handle default values for NOT NULL columns
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Bug ID: 17080 Summary: Koha::Object->new should handle default values for NOT NULL columns Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@bugs.koha-community.org Reporter: jonathan.druart@bugs.koha-community.org QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |16960, 17069, 16917 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16917 [Bug 16917] Error when importing patrons, Column 'checkprevcheckout' cannot be null https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16960 [Bug 16960] Patron::Modifications should be fixed https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17069 [Bug 17069] Can't create new patron category on the intranet -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54162 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54162&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54163 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54163&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54164 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54164&action=edit 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). -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54165 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54165&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #5 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 54166 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54166&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Srdjan Jankovic <srdjan@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |srdjan@catalyst.net.nz --- Comment #6 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- I believe this needs rebasing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Srdjan Jankovic from comment #6)
I believe this needs rebasing
It applies cleanly on top of bug 16960. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #8 from Srdjan Jankovic <srdjan@catalyst.net.nz> --- Sorry, my bad, missed that one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54162|0 |1 is obsolete| | --- Comment #9 from Marc Véron <veron@veron.ch> --- Created attachment 54465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54465&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54163|0 |1 is obsolete| | --- Comment #10 from Marc Véron <veron@veron.ch> --- Created attachment 54466 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54466&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54164|0 |1 is obsolete| | --- Comment #11 from Marc Véron <veron@veron.ch> --- Created attachment 54467 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54467&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54165|0 |1 is obsolete| | --- Comment #12 from Marc Véron <veron@veron.ch> --- Created attachment 54468 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54468&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54166|0 |1 is obsolete| | --- Comment #13 from Marc Véron <veron@veron.ch> --- Created attachment 54469 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54469&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54465|0 |1 is obsolete| | Attachment #54466|0 |1 is obsolete| | Attachment #54467|0 |1 is obsolete| | Attachment #54468|0 |1 is obsolete| | Attachment #54469|0 |1 is obsolete| | --- Comment #14 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 56464 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56464&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 56465 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56465&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 56466 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56466&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #17 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 56467 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56467&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #18 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 56468 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56468&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Cleared a small conflict in tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 56728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56728&action=edit Bug 17080 [QA Followup] - Fix number of tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17080 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #22 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jonathan! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org