Bug 14036 - "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscription detail
Summary: "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscript...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Rolando Isidoro
QA Contact: Testopia
URL:
Keywords:
Depends on: 13109
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-21 13:45 UTC by Rolando Isidoro
Modified: 2020-11-30 21:45 UTC (History)
6 users (show)

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


Attachments
Patch that adds date validation (939 bytes, patch)
2015-04-23 11:00 UTC, Rolando Isidoro
Details | Diff | Splinter Review
Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscription detail (2.87 KB, patch)
2015-04-23 19:30 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036 - GetNextDate.t lacking test cases. (1.59 KB, patch)
2015-04-23 19:43 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036 - GetNextDate.t lacking test cases. (1.59 KB, patch)
2015-04-23 19:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036 - GetNextDate.t lacking test cases. (1.59 KB, patch)
2015-04-23 19:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscription detail (2.26 KB, patch)
2015-04-23 19:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036 - GetNextDate.t lacking test cases. (1.64 KB, patch)
2015-04-23 19:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscription detail (2.31 KB, patch)
2015-04-23 19:53 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Rolando Isidoro 2015-04-21 13:45:32 UTC
When accessing the subscription detail page for a serial with unknown "Planned date" and/or "Published date" the following error occurs:

subscription-detail.pl: Date::Calc::PP::Add_Delta_YM(): Date::Calc::Add_Delta_YM(): not a valid date at /usr/share/koha/lib/C4/Serials.pm line 2607
 at /usr/share/perl/5.14/CGI/Carp.pm line 379
	CGI::Carp::realdie('[Tue Apr 21 14:43:06 2015] subscription-detail.pl: Date::Calc...') called at /usr/share/perl/5.14/CGI/Carp.pm line 475
	CGI::Carp::die('Date::Calc::PP::Add_Delta_YM(): Date::Calc::Add_Delta_YM(): n...') called at /usr/share/perl5/Carp/Clan.pm line 225
	Carp::Clan::__ANON__('Date::Calc::Add_Delta_YM(): not a valid date') called at /usr/share/perl5/Date/Calc/PP.pm line 408
	Date::Calc::PP::DATECALC_DATE_ERROR('Add_Delta_YM') called at /usr/share/perl5/Date/Calc/PP.pm line 817
	Date::Calc::PP::Add_Delta_YM(0000, 00, 00, 0, 1) called at /usr/share/koha/lib/C4/Serials.pm line 2607
	C4::Serials::GetNextDate('HASH(0x6244de0)', '0000-00-00') called at /usr/share/koha/lib/C4/Serials.pm line 2382
	C4::Serials::abouttoexpire(525) called at /usr/share/koha/intranet/cgi-bin/serials/subscription-detail.pl line 104
Comment 1 Rolando Isidoro 2015-04-23 11:00:12 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2015-04-23 11:52:33 UTC
Comment on attachment 38399 [details] [review]
Patch that adds date validation

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

::: C4/Serials.pm
@@ +2246,5 @@
>          my $expirationdate = GetExpirationDate($subscriptionid);
>          my ($res) = $dbh->selectrow_array('select max(planneddate) from serial where subscriptionid = ?', undef, $subscriptionid);
>          my $nextdate = GetNextDate($subscription, $res);
> +
> +        return unless ($nextdate and $expirationdate);

See also bug 13109.
Comment 3 Mark Tompsett 2015-04-23 12:05:40 UTC
Comment on attachment 38399 [details] [review]
Patch that adds date validation

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

::: C4/Serials.pm
@@ +2386,4 @@
>      if ($freqdata->{'unit'}) {
>          my ( $year, $month, $day ) = split /-/, $publisheddate;
>  
> +        return unless check_date($year, $month, $day);

Not necessary, because:
return unless $subscription and $publisheddate;
Comment 4 Mark Tompsett 2015-04-23 12:06:55 UTC

*** This bug has been marked as a duplicate of bug 13109 ***
Comment 5 Rolando Isidoro 2015-04-23 12:48:09 UTC
Hi M. Tompsett, the bugs are very similar indeed but this problem occurs for 'planneddate' while #13109 refers to 'startdate' or 'enddate'.

As for the patch I submitted, both validation are required, the check_date one you mentioned as not being needed is mandatory to solve the problem as the error occurs in the GetNextDate function.

I'm reverting the status to 'REOPENED', can you please take a second look at it?
Comment 6 Mark Tompsett 2015-04-23 19:12:28 UTC
(In reply to Rolando Isidoro from comment #5)
> As for the patch I submitted, both validation are required, the check_date
> one you mentioned as not being needed is mandatory to solve the problem as
> the error occurs in the GetNextDate function.
> 
> I'm reverting the status to 'REOPENED', can you please take a second look at
> it?

GetNextDate receives a parameter $publisheddate (which it may or may not be). As far as I can tell, it is either:
NULL, 0000-00-00 (MySQL badness), or a valid MySQL date (YYYY-MM-DD).
I have confirmed that 0000-00-00 triggers an error. I'll be attaching tests that were missing.

Please remove the first part of this patch, as 13109 resolves that issue. Perhaps add a dependency?
Comment 7 Mark Tompsett 2015-04-23 19:30:20 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2015-04-23 19:43:35 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2015-04-23 19:50:45 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2015-04-23 19:51:17 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-04-23 19:51:23 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2015-04-23 19:53:07 UTC
Created attachment 38467 [details] [review]
Bug 14036 - GetNextDate.t lacking test cases.

Two test cases were added to improve testing. This catches the
problem encountered by Rolando Isidoro.

TEST PLAN
---------
1) Apply this test patch first.
2) prove t/db_dependent/Serials/GetNextDate.t
   -- this should bomb horribly.
3) Apply the other patch.
4) prove t/db_dependent/Serials/GetNextDate.t
   -- this should succeed.
5) koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 13 Mark Tompsett 2015-04-23 19:53:15 UTC
Created attachment 38468 [details] [review]
Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing subscription detail

When accessing the subscription detail page for a serial with unknown "Planned date" and/or "Published date" the following error occurs:

subscription-detail.pl: Date::Calc::PP::Add_Delta_YM(): Date::Calc::Add_Delta_YM(): not a valid date at /usr/share/koha/lib/C4/Serials.pm line 2607
 at /usr/share/perl/5.14/CGI/Carp.pm line 379
	CGI::Carp::realdie('[Tue Apr 21 14:43:06 2015] subscription-detail.pl: Date::Calc...') called at /usr/share/perl/5.14/CGI/Carp.pm line 475
	CGI::Carp::die('Date::Calc::PP::Add_Delta_YM(): Date::Calc::Add_Delta_YM(): n...') called at /usr/share/perl5/Carp/Clan.pm line 225
	Carp::Clan::__ANON__('Date::Calc::Add_Delta_YM(): not a valid date') called at /usr/share/perl5/Date/Calc/PP.pm line 408
	Date::Calc::PP::DATECALC_DATE_ERROR('Add_Delta_YM') called at /usr/share/perl5/Date/Calc/PP.pm line 817
	Date::Calc::PP::Add_Delta_YM(0000, 00, 00, 0, 1) called at /usr/share/koha/lib/C4/Serials.pm line 2607
	C4::Serials::GetNextDate('HASH(0x6244de0)', '0000-00-00') called at /usr/share/koha/lib/C4/Serials.pm line 2382
	C4::Serials::abouttoexpire(525) called at /usr/share/koha/intranet/cgi-bin/serials/subscription-detail.pl line 104

This patch adds validation to prevent the error from occurring.
Follow the test plan in the subsequent patch.

This patch deals solely with this.
> C4::Serials::GetNextDate('HASH(0x6244de0)', '0000-00-00') called at /usr/share/koha/lib/C4/Serials.pm line 2382

abouttoexpire issues exist, but may be resolved by applying bug 13109 first.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 14 Mark Tompsett 2015-04-23 19:54:21 UTC
Leaving as 'Needs Signoff' since I shouldn't be signing off on my own tests. Feel free to sign off, Rolando.
Comment 15 Rolando Isidoro 2015-04-24 10:16:45 UTC
Signed off and added bug 13109 dependency.
Comment 16 Katrin Fischer 2015-04-29 05:34:56 UTC
I am sorry, but I can't reproduce the problem on current master. I tried with an irregular subscription that will leave planneddate and publisheddateempty and I manually changed the database values to 0000-00-00 for both. 

Please include a test plan.
Comment 17 Mark Tompsett 2015-04-29 08:52:46 UTC
Please see the test plan in comment #12, Katrin.
Comment 18 Katrin Fischer 2015-04-29 08:55:37 UTC
Mark, this is what I did.
Comment 19 Mark Tompsett 2015-04-29 09:04:36 UTC
mtompset@debian:~/kohaclone$ git reset --hard origin/master
HEAD is now at 381959a Bug 13109: Serial failure for received and general viewin                                                                             g.
mtompset@debian:~/kohaclone$ git bz apply 14036
Bug 14036 - "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing                                                                              subscription detail

38467 - Bug 14036 - GetNextDate.t lacking test cases.
38468 - Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when acc                                                                             essing subscription detail

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 14036 - GetNextDate.t lacking test cases.
mtompset@debian:~/kohaclone$ prove -v t/db_dependent/Serials/GetNextDate.t
t/db_dependent/Serials/GetNextDate.t ..
1..88
Date::Calc::Delta_Days(): not a valid date at /home/mtompset/kohaclone/C4/Serial                                                                             s.pm line 2345.
# Looks like your test exited with 255 before it could output anything.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 88/88 subtests

Test Summary Report
-------------------
t/db_dependent/Serials/GetNextDate.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 88 tests but ran 0.
Files=1, Tests=0,  0 wallclock secs ( 0.02 usr  0.00 sys +  0.31 cusr  0.05 csys                                                                              =  0.38 CPU)
Result: FAIL
mtompset@debian:~/kohaclone$ git bz apply 14036
Bug 14036 - "Date::Calc::Add_Delta_YM(): not a valid date" error when accessing                                                                              subscription detail

38467 - Bug 14036 - GetNextDate.t lacking test cases.
38468 - Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when acc                                                                             essing subscription detail

Apply? [(y)es, (n)o, (i)nteractive] i
Applying: Bug 14036: "Date::Calc::Add_Delta_YM(): not a valid date" error when a                                                                             ccessing subscription detail
mtompset@debian:~/kohaclone$ prove -v t/db_dependent/Serials/GetNextDate.t
t/db_dependent/Serials/GetNextDate.t ..
1..88
ok 1 - 0000-00-00 is an expected undef date.
ok 2 - 9999-99-99 is an expected undef date.
[SNIP]
ok 88
ok
All tests successful.
Files=1, Tests=88,  1 wallclock secs ( 0.02 usr  0.00 sys +  0.37 cusr  0.00 csy                                                                             s =  0.39 CPU)
Result: PASS
Comment 20 Mark Tompsett 2015-04-29 09:25:09 UTC
BTW, I have no subscriptions before testing.
Comment 21 Katrin Fischer 2015-04-29 09:30:42 UTC
Can you still reproduce the issue in the GUI on current master?
What does your entry in subscription and serial look like?
Comment 22 Mark Tompsett 2015-04-29 09:39:25 UTC
(In reply to Katrin Fischer from comment #21)
> Can you still reproduce the issue in the GUI on current master?
> What does your entry in subscription and serial look like?

I will attempt to replicate it in the interface later, but I did not use the interface in my testing.
Comment 23 Katrin Fischer 2015-04-29 19:41:43 UTC
Please provide steps to make the problem visible outside of the tests.
Comment 24 Mark Tompsett 2015-04-29 19:49:50 UTC
(In reply to Katrin Fischer from comment #23)
> Please provide steps to make the problem visible outside of the tests.

I'll work on it, but it make require setting up bad data in an earlier version, upgrading, and then trying to edit.
Comment 25 Katrin Fischer 2015-04-29 20:05:04 UTC
If you or Rolando can tell me how to alter the data in order to make the problem appear that would work for me for now.
Comment 26 Mark Tompsett 2015-05-19 14:54:01 UTC
(In reply to Katrin Fischer from comment #25)
> If you or Rolando can tell me how to alter the data in order to make the
> problem appear that would work for me for now.

Haven't triggered the exact problem, but a similar one:
1) Set up stuff.
    a) Home -> Acquisitions
    b) Add Vendor
    c) Add Budget
    d) Add Fund for that Budget
       -- not all these steps may be necessary, but they are a good idea.
2) Home -> Serials
3) New Subscription
4) Fill in first page nicely, click Next
5) Fill in second page nicely, click 'Test prediction pattern'
6) EVIL STEP: Change the publication date to 00/00/0000
7) Click 'Save'
   -- KABOOM!
Comment 27 Mark Tompsett 2015-05-19 18:07:20 UTC
(In reply to Katrin Fischer from comment #25)
> If you or Rolando can tell me how to alter the data in order to make the
> problem appear that would work for me for now.

I have looked further, and believe the only way to trigger this problem is bad data.

However, as I just posted, there are validation failures at the template level which end up calling these functions in exploding ways.

GetNextSeq and GetNextDate in C4::Serials both call GetFictiveIssueNumber, and it is the second parameter as 0000-00-00 which causes the explosion.

This is easy to trigger as tests apart from the interface.

However, some validation on the template pages actually prevents such explosions. Though, as demonstrated in comment #26, there needs to be an improvement in validations done.

Would adding those validations to solve comment #26 be sufficient to pass all of this?
Comment 28 Marc Véron 2015-12-24 14:11:44 UTC
Is this bug still valid?
Comment 29 Jonathan Druart 2019-12-11 14:55:12 UTC
Date formatting is enforced template-side. This is certainly no longer valid. Closing, please reopen if still valid.