Bug 15087 - hardduedate should not be inserted as 0000-00-00 but NULL
Summary: hardduedate should not be inserted as 0000-00-00 but NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 14949
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-29 22:57 UTC by Bernardo Gonzalez Kriegel
Modified: 2016-12-05 21:34 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 15087: make dt_from_string return undef for '0000-00-00' (942 bytes, patch)
2015-10-29 23:04 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15087: Fix GetHardDueDate return value (1.06 KB, patch)
2015-10-30 11:17 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL (1.51 KB, patch)
2015-10-30 11:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL (1.82 KB, patch)
2015-10-30 12:04 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL (1.88 KB, patch)
2015-10-30 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15087 - Follow for koha-create (1.29 KB, patch)
2016-06-03 13:31 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Bernardo Gonzalez Kriegel 2015-10-29 22:57:04 UTC
Current master, define circulation rule All, All any number of days,
but left 'Hard due date' EMPTY.

Try to checkout something to a patron, you get

Can't locate object method "truncate" via package "-1" (perhaps you forgot to load "-1"?) at ....

Problem comes from GetHardDueDate, it returns

return (dt_from_string($rule->{hardduedate}, 'iso'),$rule->{hardduedatecompare});

But dt_from_string returns without value in case of '0000-00-00' date,

return if $date_string and $date_string =~ m|^0000-0|

which evaluates to an empty list in this case, making the final return value for GetHardDueDate ( -1 ) instead of ( undef, -1 )
Comment 1 Bernardo Gonzalez Kriegel 2015-10-29 23:04:06 UTC Comment hidden (obsolete)
Comment 2 Héctor Eduardo Castro Avalos 2015-10-30 01:25:43 UTC
Hi Bernardo

I'll add here Marc and Jonathan, since they have more experience with Koha::DateUtils. I feel scared when you mention that can have side effects.
Comment 3 Marc Véron 2015-10-30 08:29:18 UTC
At other places we did

return ( eval { dt_from_string($rule->{hardduedate}, 'iso'),$rule->{hardduedatecompare}); };

...to avoid such errors.

I propose to do it here as well and postpone the change to dt_from_string until the C4::Dates removal is finished.

It is in C4/Circulation.pm
Line 1498
Comment 4 Jonathan Druart 2015-10-30 10:51:49 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #0)
> Current master, define circulation rule All, All any number of days,
> but left 'Hard due date' EMPTY.
> 
> Try to checkout something to a patron, you get
> 
> Can't locate object method "truncate" via package "-1" (perhaps you forgot
> to load "-1"?) at ....

Outch!

> Problem comes from GetHardDueDate, it returns
> 
> return (dt_from_string($rule->{hardduedate},
> 'iso'),$rule->{hardduedatecompare});

Yep.

> But dt_from_string returns without value in case of '0000-00-00' date,

It usually return undef when not called in a list context.

(In reply to Bernardo Gonzalez Kriegel from comment #1)
> Created attachment 44181 [details] [review] [review]
> Bug 15087: make dt_from_string return undef for '0000-00-00'

No! :)
It is usually a bad idea to explicitly return undef, and you get a perlcritic warning.

> I don't know if this could have side effects

That's why it's preferable not to change the return of the subroutine but the caller(s?).

(In reply to Marc Véron from comment #3)
> At other places we did
> 
> return ( eval { dt_from_string($rule->{hardduedate},
> 'iso'),$rule->{hardduedatecompare}); };

It's a weird hack :)
dt_from_string does not return an error.

> I propose to do it here as well and postpone the change to dt_from_string
> until the C4::Dates removal is finished.

We cannot wait for a fix here as it makes the application explodes.
Comment 5 Jonathan Druart 2015-10-30 11:08:27 UTC
Bernardo, I don't reproduce the problem you described.
My issuingrules table is empty.
Please detail.
Comment 6 Bernardo Gonzalez Kriegel 2015-10-30 11:17:34 UTC Comment hidden (obsolete)
Comment 7 Bernardo Gonzalez Kriegel 2015-10-30 11:21:16 UTC
(In reply to Jonathan Druart from comment #5)
> Bernardo, I don't reproduce the problem you described.
> My issuingrules table is empty.
> Please detail.

Problem arises when you create a rule
 mysql kohadev -e "select categorycode,itemtype,hardduedate from issuingrules"
+--------------+----------+-------------+
| categorycode | itemtype | hardduedate |
+--------------+----------+-------------+
| *            | *        | 0000-00-00  |
+--------------+----------+-------------+

Changed patch and bug description
Comment 8 Bernardo Gonzalez Kriegel 2015-10-30 11:23:17 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Bernardo Gonzalez Kriegel from comment #0)
> > Current master, define circulation rule All, All any number of days,
> > but left 'Hard due date' EMPTY.
> > 
> > Try to checkout something to a patron, you get
> > 
> > Can't locate object method "truncate" via package "-1" (perhaps you forgot
> > to load "-1"?) at ....
> 
> Outch!
> 

The problem was discovered by Hector.
Comment 9 Jonathan Druart 2015-10-30 11:39:52 UTC
Bernardo, ok thanks, I am on it!
Comment 10 Jonathan Druart 2015-10-30 11:53:46 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-10-30 11:54:43 UTC
To restore the behavior that existed before bug 14949, I think this is the way to go.
Comment 12 Jonathan Druart 2015-10-30 11:55:00 UTC
And this is critical
Comment 13 Bernardo Gonzalez Kriegel 2015-10-30 12:04:03 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2015-10-30 12:06:23 UTC
Signed and changed description... again :)
(In reply to Jonathan Druart from comment #11)
> To restore the behavior that existed before bug 14949, I think this is the
> way to go.

Agreed

Signed and changed summary... again :)
Comment 15 Kyle M Hall 2015-10-30 18:13:24 UTC
Created attachment 44244 [details] [review]
[PASSED QA] Bug 15087: hardduedate should not be inserted as 0000-00-00 but NULL

This has been introduced by bug 14949, recently pushed.
So I don't think we need to update the DB, only devs will face this
problem.

The hardduedate was inserted as 0000-00-00 because was en empty string.
To be inserted as NULL, the value should be undef.

Test plan:
0/ Don't apply this patch
1/ Edit a circ rules and don't fill the hard due date
2/ Look at the DB, the value is 0000-00-00
3/ Apply this patch and confirm that the value is now NULL

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Confirmed
+--------------+----------+-------------+
| categorycode | itemtype | hardduedate |
+--------------+----------+-------------+
| *            | *        | NULL        |
+--------------+----------+-------------+

No koha-qa errors

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Tomás Cohen Arazi 2015-11-02 14:37:16 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 17 Chris Cormack 2016-06-03 13:31:08 UTC Comment hidden (obsolete)