Bug 21610 - Koha::Object->store needs to handle incorrect values
Summary: Koha::Object->store needs to handle incorrect values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 21601 21618 (view as bug list)
Depends on:
Blocks: 17258
  Show dependency treegraph
 
Reported: 2018-10-18 21:25 UTC by Jonathan Druart
Modified: 2019-10-14 19:59 UTC (History)
8 users (show)

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


Attachments
Bug 21610: Add tests (2.90 KB, patch)
2018-10-18 22:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21610: Remove specific changes we made previously (3.38 KB, patch)
2018-10-18 22:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21610: Fix integers and dates values at Koha::Object->store level (3.22 KB, patch)
2018-10-18 22:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21610: (QA follow-up) Better use columns_info from DBIx (1.01 KB, patch)
2018-10-19 09:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21610: (follow-up) Default value for dates (1.14 KB, patch)
2018-10-19 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21610: Improve default values for dates (1.32 KB, patch)
2018-10-19 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21610: Add tests (2.97 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: Remove specific changes we made previously (3.45 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: Fix integers and dates values at Koha::Object->store level (3.29 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: (QA follow-up) Better use columns_info from DBIx (1.05 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: (follow-up) Default value for dates (1.17 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: Improve default values for dates (1.39 KB, patch)
2018-10-23 13:16 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21610: Add tests (3.03 KB, patch)
2019-02-01 11:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Remove specific changes we made previously (3.50 KB, patch)
2019-02-01 11:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Fix integers and dates values at Koha::Object->store level (3.28 KB, patch)
2019-02-01 11:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: (QA follow-up) Better use columns_info from DBIx (1.03 KB, patch)
2019-02-01 11:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: (follow-up) Default value for dates (1.16 KB, patch)
2019-02-01 11:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Improve default values for dates (1.38 KB, patch)
2019-02-01 11:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Add tests (3.03 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Remove specific changes we made previously (3.56 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Fix integers and dates values at Koha::Object->store level (3.33 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: (QA follow-up) Better use columns_info from DBIx (1.08 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: (follow-up) Default value for dates (1.21 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21610: Improve default values for dates (1.43 KB, patch)
2019-02-01 19:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-10-18 21:25:27 UTC
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)
Comment 1 Jonathan Druart 2018-10-18 21:25:51 UTC
*** Bug 21601 has been marked as a duplicate of this bug. ***
Comment 2 Jonathan Druart 2018-10-18 22:05:52 UTC
Created attachment 80880 [details] [review]
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'
Comment 3 Jonathan Druart 2018-10-18 22:05:55 UTC
Created attachment 80881 [details] [review]
Bug 21610: Remove specific changes we made previously

We handled integers and dates for Koha::Patron and Koha::ItemType, we
want now Koha::Object to deal with.
Comment 4 Jonathan Druart 2018-10-18 22:05:59 UTC
Created attachment 80882 [details] [review]
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!
Comment 5 Marcel de Rooy 2018-10-19 07:48:09 UTC
Testing here now
Comment 6 Marcel de Rooy 2018-10-19 08:53:51 UTC
This test triggers the warn resolved in bug 21617
Comment 7 Marcel de Rooy 2018-10-19 09:09:43 UTC
Created attachment 80906 [details] [review]
Bug 21610: (QA follow-up) Better use columns_info from DBIx

We should better use the published method than a private hash key.
And shorter to go via _result than adding a new schema call.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-10-19 09:38:50 UTC
Still working here
Comment 9 Marcel de Rooy 2018-10-19 10:12:41 UTC
About this code:
                if ( $columns_info->{$col}->{is_nullable} ) {
                    # If nullable, default to null
                    $self->$col(undef);
                } else {
                    # If cannot be null, get the default value
                    # What if cannot be null and does not have a default value? Possible?
                    $self->$col($columns_info->{$col}->{default_value});

TRY:
alter table borrowers add column notpossible int NOT NULL;
So yes, it is possible.
For now, I would recommend to change the last statement to:
$self->$col( $columns_info->{$col}->{default_value} // 0 );
Comment 10 Marcel de Rooy 2018-10-19 10:21:52 UTC
Created attachment 80926 [details] [review]
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>
Comment 11 Marcel de Rooy 2018-10-19 10:24:16 UTC
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 ?
Comment 12 Marcel de Rooy 2018-10-19 10:24:41 UTC
So some additional feedback is still welcome !
Comment 13 Jonathan Druart 2018-10-19 11:02:38 UTC
*** Bug 21618 has been marked as a duplicate of this bug. ***
Comment 14 Jonathan Druart 2018-10-19 11:07:20 UTC
(In reply to Marcel de Rooy from comment #10)
> Created attachment 80926 [details] [review] [review]
> Bug 21610: (follow-up) Default value for dates

This patch fixes t/Auth_with_shibboleth.t but breaks t/db_dependent/Koha/Patrons/Import.t
Comment 15 Jonathan Druart 2018-10-19 11:23:22 UTC
Created attachment 80927 [details] [review]
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
Comment 16 Kyle M Hall 2018-10-23 13:14:04 UTC
Comment on attachment 80882 [details] [review]
Bug 21610: Fix integers and dates values at Koha::Object->store level

Review of attachment 80882 [details] [review]:
-----------------------------------------------------------------

::: Koha/Object.pm
@@ +308,5 @@
> +        'date',
> +        'datetime'
> +    );
> +
> +    return ( grep { $column_type eq $_ } @dt_types) ? 1 : 0;

I think this may be quite inefficient as it uses grep. Perhaps we could use https://perldoc.perl.org/List/Util.html#any instead? Or just a set of ternary operators or elsifs if we really want to keep it fast and simple.
Comment 17 Kyle M Hall 2018-10-23 13:16:02 UTC
Created attachment 81016 [details] [review]
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>
Comment 18 Kyle M Hall 2018-10-23 13:16:13 UTC
Created attachment 81017 [details] [review]
Bug 21610: Remove specific changes we made previously

We handled integers and dates for Koha::Patron and Koha::ItemType, we
want now Koha::Object to deal with.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2018-10-23 13:16:16 UTC
Created attachment 81018 [details] [review]
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>
Comment 20 Kyle M Hall 2018-10-23 13:16:20 UTC
Created attachment 81019 [details] [review]
Bug 21610: (QA follow-up) Better use columns_info from DBIx

We should better use the published method than a private hash key.
And shorter to go via _result than adding a new schema call.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2018-10-23 13:16:23 UTC
Created attachment 81020 [details] [review]
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>
Comment 22 Kyle M Hall 2018-10-23 13:16:26 UTC
Created attachment 81021 [details] [review]
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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2018-10-23 13:17:52 UTC
Please address or rebut by critique of the use of grep in comment 16 and then set to back to Signed Off!
Comment 24 Tomás Cohen Arazi 2018-10-23 13:28:49 UTC
What about things like this:

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

Also, it feels DBIX::Class should have some way to tell 'use the default when undef is passed'. I'm searching.
Comment 25 Jonathan Druart 2018-10-23 13:43:09 UTC
(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 :)
Comment 26 Tomás Cohen Arazi 2018-10-23 13:51:18 UTC
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.
Comment 27 Jonathan Druart 2018-10-23 13:52:34 UTC
(In reply to Kyle M Hall from comment #16)
> Comment on attachment 80882 [details] [review] [review]
> Bug 21610: Fix integers and dates values at Koha::Object->store level
> 
> Review of attachment 80882 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Object.pm
> @@ +308,5 @@
> > +        'date',
> > +        'datetime'
> > +    );
> > +
> > +    return ( grep { $column_type eq $_ } @dt_types) ? 1 : 0;
> 
> I think this may be quite inefficient as it uses grep. Perhaps we could use
> https://perldoc.perl.org/List/Util.html#any instead? Or just a set of
> ternary operators or elsifs if we really want to keep it fast and simple.

Hi Kyle, thanks for your feedback!

I would consider this as out of the scope for now, I reused what _datetime_column_type and other methods already did:

Koha/Object.pm:        unless ( grep {/^$p$/} @columns ) {
Koha/Object.pm:    return ( grep { $column_type eq $_ } @dt_types) ? 1 : 0;
Koha/Object.pm:    return ( grep { $column_type eq $_ } @numeric_types) ? 1 : 0;
Koha/Object.pm:    if ( grep {/^$method$/} @columns ) {
Koha/Object.pm:    ) unless grep { /^$method$/ } @known_methods;

However I agree we should replace them but, then, all of them at the same time :)
I am going to open a separate bug report to talk about that.
Comment 28 Jonathan Druart 2018-10-23 13:53:21 UTC
(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.
Comment 29 Tomás Cohen Arazi 2018-10-23 13:56:16 UTC
(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.
Comment 30 Tomás Cohen Arazi 2018-10-23 13:57:17 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> (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.

Mm, the OpenAPI validator would take care if our specs are correct...
Comment 31 Martin Renvoize 2018-10-23 13:58:00 UTC
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.
Comment 32 Martin Renvoize 2018-10-23 13:58:21 UTC
Seems Tomas just expressed the same concern as me there really.
Comment 33 Marcel de Rooy 2018-10-23 14:10:26 UTC
Isn't the problem that we ignored too many warnings in the past that we cannot let it explode on such things now ?
Comment 34 Jonathan Druart 2018-10-23 14:11:27 UTC
(In reply to Martin Renvoize from comment #31)
> 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.

So maybe you are both missing the point of this approach: we do not want the callers (controllers) to validate the data, it will be horrible to write and maintain.

I am waiting for your suggestions, I do not have another one.
Comment 35 Jonathan Druart 2018-10-23 15:02:04 UTC
We could imagine a { strict => 1 } param that would skip these checks (?)
Comment 36 Marcel de Rooy 2018-10-25 10:21:51 UTC
(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'.
Comment 37 Jonathan Druart 2018-12-05 22:05:17 UTC
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
Comment 38 Marcel de Rooy 2018-12-13 14:48:52 UTC
Tomas, 
It seems that we still need your answer on the strict parameter question of Joubu now?
Comment 39 Tomás Cohen Arazi 2018-12-13 15:00:20 UTC
(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.
Comment 40 Tomás Cohen Arazi 2018-12-24 10:52:13 UTC
(In reply to Jonathan Druart from comment #35)
> We could imagine a { strict => 1 } param that would skip these checks (?)

I agree with this middle ground approach. And with Marcel saying the chosen word is strange, but probably correct, we are asking it to raise exceptions instead of fixing the passed data.

I would also love a 'warn' line when we fix something. I like the idea of users complaining about their logs, because it will make removing this problem faster. <-- this wouldn't be a blocker from my end if there's no consensus.
Comment 41 Tomás Cohen Arazi 2019-02-01 11:02:37 UTC
Created attachment 84601 [details] [review]
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>
Comment 42 Tomás Cohen Arazi 2019-02-01 11:02:45 UTC
Created attachment 84602 [details] [review]
Bug 21610: Remove specific changes we made previously

We handled integers and dates for Koha::Patron and Koha::ItemType, we
want now Koha::Object to deal with.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Tomás Cohen Arazi 2019-02-01 11:02:50 UTC
Created attachment 84603 [details] [review]
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>
Comment 44 Tomás Cohen Arazi 2019-02-01 11:02:54 UTC
Created attachment 84604 [details] [review]
Bug 21610: (QA follow-up) Better use columns_info from DBIx

We should better use the published method than a private hash key.
And shorter to go via _result than adding a new schema call.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Tomás Cohen Arazi 2019-02-01 11:03:00 UTC
Created attachment 84605 [details] [review]
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>
Comment 46 Tomás Cohen Arazi 2019-02-01 11:03:04 UTC
Created attachment 84606 [details] [review]
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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Tomás Cohen Arazi 2019-02-01 19:22:21 UTC
Created attachment 84642 [details] [review]
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>
Comment 48 Tomás Cohen Arazi 2019-02-01 19:22:26 UTC
Created attachment 84643 [details] [review]
Bug 21610: Remove specific changes we made previously

We handled integers and dates for Koha::Patron and Koha::ItemType, we
want now Koha::Object to deal with.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 49 Tomás Cohen Arazi 2019-02-01 19:22:30 UTC
Created attachment 84644 [details] [review]
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>
Comment 50 Tomás Cohen Arazi 2019-02-01 19:22:34 UTC
Created attachment 84645 [details] [review]
Bug 21610: (QA follow-up) Better use columns_info from DBIx

We should better use the published method than a private hash key.
And shorter to go via _result than adding a new schema call.

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>
Comment 51 Tomás Cohen Arazi 2019-02-01 19:22:38 UTC
Created attachment 84646 [details] [review]
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>
Comment 52 Tomás Cohen Arazi 2019-02-01 19:22:43 UTC
Created attachment 84647 [details] [review]
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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Nick Clemens 2019-02-01 20:34:48 UTC
Awesome work all!

Pushed to master for 19.05
Comment 54 Martin Renvoize 2019-02-04 11:14:24 UTC
Pushed to 18.11.x for 18.11.03
Comment 55 Lucas Gass 2019-02-08 00:41:02 UTC
cant cleanly apply this patch set. if needed in 18.05.x please rebase. thanks