https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Koha::Object->store need to |Koha::Object->store needs |handle incorrect values |to handle incorrect values -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #2 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80880 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80880&action=edit Bug 21610: Add tests Adding few tests to Koha/Object.t in order to highlight what we are trying to fix. Note that Koha::ItemType->store was wrong for notforloan, the default value should be null `notforloan` smallint(6) DEFAULT NULL, but the previous fix (bug 21599) made it default to '0' -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #4 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80882 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80882&action=edit Bug 21610: Fix integers and dates values at Koha::Object->store level With strict SQL modes, we are facing several issues when objects are stored. The following errors can be raised by DBIx::Class when an object does not have a valid (as defined at DB level) value: - Incorrect integer value: '' for column COLUMN - Incorrect decimal value: '' for column COLUMN - Column 'COLUMN' cannot be null - Field 'COLUMN' doesn't have a default value - Incorrect date value: '' for column 'COLUMN' We already handled some of them for Koha::Patron. As we do not want to provide such changes for all Koha::* modules we are going to fix this at Koha::Object->store level in order to provide a global fix. This is related to bug 21599 (for item types) and bug 21596 (for patrons) Test plan: - Apply first patch, run the tests => Patch fail because of the previous error in Koha::ItemType->store, to make them pass you can replace $self->notforloan(0) unless $self->notforloan; with $self->notforloan(undef) unless $self->notforloan; - Apply second patch, run the tests => They fail because of dates and/or integers values - Apply third patch, run the tests => They now pass Deep code review wanted! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17258 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17258 [Bug 17258] [OMNIBUS] MySQL 5.7 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=21617 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- This test triggers the warn resolved in bug 21617 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |BLOCKED --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Still working here -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Needs Signoff Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Couldn't finish this in time. Last patch might still need a little bit of thought. Shouldnt we use the exact same logic for int and date? And sql uses the default value only for a new record. Should we use it at all when updating or bail out when NULL is not allowed ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- *** Bug 21618 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 80927 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=80927&action=edit Bug 21610: Improve default values for dates This patch fixes failures on t/db_dependent/Koha/Patrons/Import.t and keep t/Auth_with_shibboleth.t and t/db_dependent/Koha/Object.t green -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #80880|0 |1 is obsolete| | Attachment #80881|0 |1 is obsolete| | Attachment #80882|0 |1 is obsolete| | Attachment #80906|0 |1 is obsolete| | Attachment #80926|0 |1 is obsolete| | Attachment #80927|0 |1 is obsolete| | --- Comment #17 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 81016 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81016&action=edit Bug 21610: Add tests Adding few tests to Koha/Object.t in order to highlight what we are trying to fix. Note that Koha::ItemType->store was wrong for notforloan, the default value should be null `notforloan` smallint(6) DEFAULT NULL, but the previous fix (bug 21599) made it default to '0' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #19 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 81018 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81018&action=edit Bug 21610: Fix integers and dates values at Koha::Object->store level With strict SQL modes, we are facing several issues when objects are stored. The following errors can be raised by DBIx::Class when an object does not have a valid (as defined at DB level) value: - Incorrect integer value: '' for column COLUMN - Incorrect decimal value: '' for column COLUMN - Column 'COLUMN' cannot be null - Field 'COLUMN' doesn't have a default value - Incorrect date value: '' for column 'COLUMN' We already handled some of them for Koha::Patron. As we do not want to provide such changes for all Koha::* modules we are going to fix this at Koha::Object->store level in order to provide a global fix. This is related to bug 21599 (for item types) and bug 21596 (for patrons) Test plan: - Apply first patch, run the tests => Patch fail because of the previous error in Koha::ItemType->store, to make them pass you can replace $self->notforloan(0) unless $self->notforloan; with $self->notforloan(undef) unless $self->notforloan; - Apply second patch, run the tests => They fail because of dates and/or integers values - Apply third patch, run the tests => They now pass Deep code review wanted! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #21 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 81020 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=81020&action=edit Bug 21610: (follow-up) Default value for dates We have some dates with DEFAULT CURRENT_TIMESTAMP and some dates with default and UPDATE clause. Test plan: Run Object.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #23 from Kyle M Hall <kyle@bywatersolutions.com> --- Please address or rebut by critique of the use of grep in comment 16 and then set to back to Signed Off! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #25 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Tomás Cohen Arazi from comment #24)

What about things like this:

https://metacpan.org/release/DBIx-Class-Validation

If I understand the goal of this module correctly it's exactly what I am trying to avoid: 1 validation per field. The idea is to provide a generic way to validate data. I am sure we will have exceptions, but we will handle them separately.

Also, it feels DBIX::Class should have some way to tell 'use the default when undef is passed'. I'm searching.

Think "0" vs undef vs "", and also update (not only insert) How do you undef an existing value? It is why DBIx::Class takes the default value if the parameter is not passed, and it is what this patch is trying to achieve. It is already what we did for Koha::Patron, nothing new here :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #29 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Jonathan Druart from comment #28)

(In reply to Tomás Cohen Arazi from comment #26)

...

I feel like (overall) I don't agree with the approach (the general one, or the various fixes on the Koha:: namespace that have been pushed to master). DBIC should explode, we should catch the explosion and handle that correctly. Or maybe I'm missing something else.

No, you do not want to deal with "" in .pl scripts, really.

Maybe, but I would love the API to return 400 bad request if someone sends "" when it shouldn't. And this should come from the object class. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #31 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- This approach scares me I'm afraid.. it feels like we're hiding errors that really ought to be getting thrown up to the callers and for developers to make informed decisions. Falling back to a 'create time' default feels bad.. if a field should be getting passed from a caller then we should be validating that it is and passing back an error to the caller if it is not to give the developer a chance of making an informed decision and passing the right data in the caller. Just my two pence. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #33 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Isn't the problem that we ignored too many warnings in the past that we cannot let it explode on such things now ? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |In Discussion -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #36 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Jonathan Druart from comment #35)

We could imagine a { strict => 1 } param that would skip these checks (?)

Sounds strange strict skipping checks, but yes could imagine that REST API would like to skip these 'fixes'. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_18_11_candidate | --- Comment #37 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Guys, please do not make this wait too long for a solution. If you disagree, suggest something else. We need to keep this* moving forward I think. * The whole "strict modes" stuff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #39 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (In reply to Marcel de Rooy from comment #38)

Tomas, It seems that we still need your answer on the strict parameter question of Joubu now?

I don't like it, generally. But I see no reason not to move forward and see how things go. At this stage I expected to have an API-driven UI which wouldn't require this sort of things as the data validation is done by the OpenAPI plugin itself. My opinion is that we should move forward and eventually drop this if something better shows up. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #81016|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #81018|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #81020|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #41 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84601 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84601&action=edit Bug 21610: Add tests Adding few tests to Koha/Object.t in order to highlight what we are trying to fix. Note that Koha::ItemType->store was wrong for notforloan, the default value should be null `notforloan` smallint(6) DEFAULT NULL, but the previous fix (bug 21599) made it default to '0' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #43 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84603 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84603&action=edit Bug 21610: Fix integers and dates values at Koha::Object->store level With strict SQL modes, we are facing several issues when objects are stored. The following errors can be raised by DBIx::Class when an object does not have a valid (as defined at DB level) value: - Incorrect integer value: '' for column COLUMN - Incorrect decimal value: '' for column COLUMN - Column 'COLUMN' cannot be null - Field 'COLUMN' doesn't have a default value - Incorrect date value: '' for column 'COLUMN' We already handled some of them for Koha::Patron. As we do not want to provide such changes for all Koha::* modules we are going to fix this at Koha::Object->store level in order to provide a global fix. This is related to bug 21599 (for item types) and bug 21596 (for patrons) Test plan: - Apply first patch, run the tests => Patch fail because of the previous error in Koha::ItemType->store, to make them pass you can replace $self->notforloan(0) unless $self->notforloan; with $self->notforloan(undef) unless $self->notforloan; - Apply second patch, run the tests => They fail because of dates and/or integers values - Apply third patch, run the tests => They now pass Deep code review wanted! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #45 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84605 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84605&action=edit Bug 21610: (follow-up) Default value for dates We have some dates with DEFAULT CURRENT_TIMESTAMP and some dates with default and UPDATE clause. Test plan: Run Object.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #84601|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #84603|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #84605|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #47 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84642 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84642&action=edit Bug 21610: Add tests Adding few tests to Koha/Object.t in order to highlight what we are trying to fix. Note that Koha::ItemType->store was wrong for notforloan, the default value should be null `notforloan` smallint(6) DEFAULT NULL, but the previous fix (bug 21599) made it default to '0' Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #49 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84644 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84644&action=edit Bug 21610: Fix integers and dates values at Koha::Object->store level With strict SQL modes, we are facing several issues when objects are stored. The following errors can be raised by DBIx::Class when an object does not have a valid (as defined at DB level) value: - Incorrect integer value: '' for column COLUMN - Incorrect decimal value: '' for column COLUMN - Column 'COLUMN' cannot be null - Field 'COLUMN' doesn't have a default value - Incorrect date value: '' for column 'COLUMN' We already handled some of them for Koha::Patron. As we do not want to provide such changes for all Koha::* modules we are going to fix this at Koha::Object->store level in order to provide a global fix. This is related to bug 21599 (for item types) and bug 21596 (for patrons) Test plan: - Apply first patch, run the tests => Patch fail because of the previous error in Koha::ItemType->store, to make them pass you can replace $self->notforloan(0) unless $self->notforloan; with $self->notforloan(undef) unless $self->notforloan; - Apply second patch, run the tests => They fail because of dates and/or integers values - Apply third patch, run the tests => They now pass Deep code review wanted! Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #51 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 84646 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=84646&action=edit Bug 21610: (follow-up) Default value for dates We have some dates with DEFAULT CURRENT_TIMESTAMP and some dates with default and UPDATE clause. Test plan: Run Object.t again. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 --- Comment #53 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21610 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable --- Comment #54 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Pushed to 18.11.x for 18.11.03 -- You are receiving this mail because: You are watching all bug changes.