Bug 14969 - Remove C4::Dates from serials/*.pl files
Summary: Remove C4::Dates from serials/*.pl files
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 15166 15168 15171
Blocks: 14870
  Show dependency treegraph
 
Reported: 2015-10-06 21:51 UTC by Marc Véron
Modified: 2017-06-14 22:05 UTC (History)
5 users (show)

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


Attachments
Bug 14969 - Remove C4::Dates from serials/*.pl files (19.32 KB, patch)
2015-10-06 22:43 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969 - Remove C4::Dates from serials/*.pl files (19.69 KB, patch)
2015-11-04 22:57 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Handle invalid date input (5.77 KB, patch)
2015-11-05 23:13 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Handle invalid date input (6.09 KB, patch)
2015-11-05 23:26 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Handle invalid date input (6.14 KB, patch)
2015-11-05 23:54 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969 - Remove C4::Dates from serials/*.pl files (19.74 KB, patch)
2015-11-06 12:25 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14969: (followup) Handle invalid date input (6.20 KB, patch)
2015-11-06 12:25 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14969: (followup) Remove C4::Dates from C4/Serials.pm (6.06 KB, patch)
2015-11-08 09:05 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Remove C4::Dates from C4/Serials.pm (6.11 KB, patch)
2015-11-08 09:15 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969:(followup) Remove C4::Dates from serials (3.62 KB, patch)
2015-11-10 12:42 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: Remove C4::Dates from serials/*.pl files (27.92 KB, patch)
2015-11-11 11:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14969: Remove C4::Dates from serials/*.pl files (27.69 KB, patch)
2015-11-11 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14969: Remove C4::Dates from serials/*.pl files (28.06 KB, patch)
2015-11-11 16:34 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Today's date withoute time when receiving (960 bytes, patch)
2015-11-12 09:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14969: (followup) Today's date withoute time when receiving (1.00 KB, patch)
2015-11-12 12:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 14969: Remove C4::Dates from serials/*.pl files (28.11 KB, patch)
2015-11-17 17:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14969: (followup) Today's date withoute time when receiving (1.07 KB, patch)
2015-11-17 17:59 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-10-06 21:51:21 UTC
Bug to remove C4::Dates from files:

- serials/acqui-search-result.pl
- serials/checkexpiration.pl
- serials/routing-preview.pl
- serials/routing.pl
- serials/serial-issues.pl
- serials/serials-collection.pl
- serials/serials-edit.pl
- serials/showpredictionpattern.pl
- serials/subscription-add.pl
- serials/subscription-detail.pl
- serials/subscription-history.pl
- serials/subscription-renew.pl
Comment 1 Marc Véron 2015-10-06 22:43:24 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2015-10-23 09:38:33 UTC
Still applies
Comment 3 Marc Véron 2015-10-29 14:08:29 UTC
Still applies
Comment 4 Marc Véron 2015-11-04 22:57:41 UTC Comment hidden (obsolete)
Comment 5 Mirko Tietgen 2015-11-05 14:17:47 UTC
When adding a suscription, after "Add a new subscription (2/2)" (where dates are entered) I get

> Software error:
>
> Can't call method "ymd" without a package or object reference at /home/mirko/koha/Koha/DateUtils.pm line 206.

Works without the patch.
Comment 6 Marc Véron 2015-11-05 16:50:28 UTC
Without the patch, invalid dates like 33/33/2099 produced some fantays dates in the future.

It is rather difficult to introduce a good date validation here. I'm trying, but if in the meantime soebody presents a valid solution I would not oppose :-)
Comment 7 Marc Véron 2015-11-05 23:13:00 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2015-11-05 23:26:16 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2015-11-05 23:26:49 UTC
Without the patch, invalid dates like 33/33/2099 produced some fantays dates in the future.

It is rather difficult to introduce a good date validation here. I'm trying, but if in the meantime soebody presents a valid solution I would not oppose :-)
Comment 10 Marc Véron 2015-11-05 23:54:49 UTC Comment hidden (obsolete)
Comment 11 Mirko Tietgen 2015-11-06 12:25:40 UTC Comment hidden (obsolete)
Comment 12 Mirko Tietgen 2015-11-06 12:25:48 UTC Comment hidden (obsolete)
Comment 13 Marc Véron 2015-11-08 09:05:37 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2015-11-08 09:15:39 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-11-10 11:43:36 UTC
1/ " =   "
=> Useless spaces.

2/ $tmpPlannedDate is not necessary, don't use this tmp var.

3/ my $date  = '';
my $date; #is enough

4/
+        $expirationdate = eval { output_pref( { dt => dt_from_string( $expirationdate ), dateonly => 1, dateformat => 'iso' }); }
+            if ( $expirationdate );
+
+        next unless ( $expirationdate );
+
         $subscription->{expirationdate} = $expirationdate;
-        next if $expirationdate !~ /\d{4}-\d{2}-\d{2}/; # next if not in ISO format.

This change is not needed.

5/
-             Date_to_Days(split "-",$expirationdate) > Date_to_Days(&Today) &&

I don't understand why you removed this line.


There are still other some inconsistencies, I would like to know what methodology you use to replace these occurrences.
For instance format_date always expect an iso formatted date, but you replace these occurrences with
  output_pref( { dt => dt_from_string( $date ), dateonly => 1 })
you should specify the dateformat param to dt_from_string.

And sometimes you replace them with

$date = eval { dt_from_string( $date ) } if $date;
$date = output_pref( { dt => $date, dateonly => 1 } ) if $date;

The changes are coming complicated and more and more errors are found on the QA step.
I would like to see a migration plan with a test file to highlight the changes you are doing.
If I remember correctly, I have attached a file doing that some weeks ago (but don't find it anymore). We need to test several values:
undef, empty string, valid date, invalid date
Comment 16 Marc Véron 2015-11-10 12:42:01 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-11-10 12:50:17 UTC
(In reply to Jonathan Druart from comment #15)
> If I remember correctly, I have attached a file doing that some weeks ago
> (but don't find it anymore). We need to test several values:
> undef, empty string, valid date, invalid date

I was thinking about bug 13813 comment 82
Comment 18 Jonathan Druart 2015-11-11 08:48:24 UTC
Comment on attachment 44618 [details] [review]
Bug 14969: (followup) Remove C4::Dates from C4/Serials.pm

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

::: C4/Serials.pm
@@ +468,5 @@
>      my @res;
>      while ( my $subs = $sth->fetchrow_hashref ) {
> +        $subs->{startdate}     = output_pref( { dt => dt_from_string( $subs->{startdate}), dateonly => 1 } );
> +        $subs->{histstartdate} =  output_pref( { dt => dt_from_string( $subs->{startdate}), dateonly => 1 } );
> +        $subs->{histenddate}   =  output_pref( { dt => dt_from_string( $subs->{startdate}), dateonly => 1 } );

Compulsive c/p :)
Comment 19 Jonathan Druart 2015-11-11 11:10:24 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2015-11-11 13:07:58 UTC
Jonathan, I wanted to sign-off on top of 15167 15168 15171 but patch does not apply:

Auto-merging serials/checkexpiration.pl
CONFLICT (content): Merge conflict in serials/checkexpiration.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/checkexpiration.tt
Failed to merge in the changes.
Comment 21 Jonathan Druart 2015-11-11 13:42:14 UTC
I have missed bug 15166
Comment 22 Marc Véron 2015-11-11 14:07:00 UTC
Jonathan, on top of 15166 15167 15168 15171 I get still:

Auto-merging serials/checkexpiration.pl
CONFLICT (content): Merge conflict in serials/checkexpiration.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/checkexpiration.tt

I is related to checkexpiration
Comment 23 Jonathan Druart 2015-11-11 14:21:17 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-11-11 14:22:23 UTC
It seems that I messed something up with 15171.
It should apply cleanly now.
Comment 25 Marc Véron 2015-11-11 15:51:49 UTC
Moved 15167 from "Depends on" to "See also"
Comment 26 Marc Véron 2015-11-11 16:34:35 UTC Comment hidden (obsolete)
Comment 27 Katrin Fischer 2015-11-11 22:47:46 UTC
I am sorry, but this page seems to break 'test prediction pattern'.
Test: Add a new subscription

This is what was sent:
add1	1
add2	1
add3	1
custompattern	0
every1	48
every2	4
every3	1
firstacquidate	01/11/2015
frequency	4
lastvalue1	2015
lastvalue2	1
lastvalue3	1
numberingmethod	Vol. {X}, Number {Y}, Issue {Z}
setto1	1
setto2	1
setto3	1
sublength	15
subtype	weeks
to	31/12/2015
whenmorethan1	99999
whenmorethan2	12
whenmorethan3	4

This is the error I could extract from Firebug:
<h1>Software error:</h1>
<pre>Usage: Date::Calc::XS::Day_of_Week(year, month, day) at /home/katrin/kohaclone/serials/showpredictionpattern
.pl line 115.
</pre>
Comment 28 Katrin Fischer 2015-11-11 22:48:50 UTC
Ah, and I tested this one on top of 15166 and 15171
Comment 29 Katrin Fischer 2015-11-11 22:51:10 UTC
... or I thought I did *sigh*... but of course I was missing 15166.
Comment 30 Katrin Fischer 2015-11-11 23:02:06 UTC
I think I found a real, but tiny bug now:
- When switching the issue status from expected to arrived from the serials-edit page, today's page is shown including the time - but should only be a date.
Comment 31 Marc Véron 2015-11-12 09:58:58 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2015-11-12 10:10:07 UTC
(In reply to Marc Véron from comment #31)
> Created attachment 44794 [details] [review] [review]
> Bug 14969: (followup) Today's date withoute time when receiving
> 
> To test: Change status of a serial from Expected to Received,
>          today's date in column "Expected" should appear without time.

I was going to submit exactly the same patch. Thanks Marc.
Comment 33 Jonathan Druart 2015-11-12 12:38:47 UTC Comment hidden (obsolete)
Comment 34 Katrin Fischer 2015-11-17 14:53:13 UTC
Sorry Marc - a recent change causes a merge conflict in checkexpiration.pl. 
Can you please take a look and fix?
Comment 35 Marc Véron 2015-11-17 15:14:53 UTC
(In reply to Katrin Fischer from comment #34)
> Sorry Marc - a recent change causes a merge conflict in checkexpiration.pl. 
> Can you please take a look and fix?

Hi Katrin, thanks for taking care of this bug :-)

I tested, both patches apply on my server.
(I did a git pull some minutes ago, HEAD is at 72d0dc6 Bug 15159: TestBuilder behaviour on AI values should be tested.)
Comment 36 Katrin Fischer 2015-11-17 17:59:03 UTC
Created attachment 44930 [details] [review]
[PASSED QA] Bug 14969: Remove C4::Dates from serials/*.pl files

Warning: This area is a mess, especially with date management.

For the same reasons as other "Remove C4::Dates from*" patches, this one
will deal with the serials module.

Note that this patch add the datepicker on 3 input fields (on
serials/subscription-detail.tt and serials/subscription-renew.tt).
There was also weird behavior when the dates were invalid. Now default
to undefined. But with the datepicker it will reduce the number of
cases even if the user is still allowed to fill invalid dates.

Test plan:
1/ Create a subscription, fill the first issue pub date and the
subscription start date.
You can fill or not the end date.
Play with the numbering pattern and confirm that there are generated as
before this patch.
2/ On the check expiration page, confirm that the dates are correctly
displayed.
3/ Renew the subscription and confirm that the behavior is correct.
4/ Go on the subscription history page and confirm you have now the
datepicker plugin set on the 2 date fields.

QA notes: The startdate, histstartdate and enddate are not used in
the serials-collection template, this patch removes them from the pl
script.

Tested on top of 15166 15168 15171, full browser reload for date pickers.
Note for 4/: It is the link in tab planning on subscription
detail page, near "Manual history" (if manual history is enabled).
Works as advertised.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2015-11-17 17:59:16 UTC
Created attachment 44931 [details] [review]
[PASSED QA] Bug 14969: (followup) Today's date withoute time when receiving

To test: Change status of a serial from Expected to Received,
         today's date in column "Expected" should appear without time.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Tomás Cohen Arazi 2015-11-17 18:53:21 UTC
Patches pushed to master.

Thanks Jonathan and Marc!
Comment 39 Dobrica Pavlinusic 2016-03-08 14:15:13 UTC
Is there reason
Comment 40 Dobrica Pavlinusic 2016-03-08 14:18:03 UTC
Is there reason why we use

output_pref( { dt => dt_from_string( $subs->{startdate} ),     dateonly => 1 } );

instead of

output_pref( { str => subs->{startdate}, dateonly => 1 } );

Second one has nice side-effect that it supports empty dates (which return as undef from database) in serial module. In our case, empty dates for planneddate and publisheddate produce application error base hash

{ dt => dt_from_string( $line->{planneddate} ),   dateonly => 1 }

folds to

{ dt , dateonly => 1 }

which makes dt = 'dateonly' which in turn is not object and result in application error.

Should I prepare separate bug for this change? Does it make sense?
Comment 41 Jonathan Druart 2016-03-08 14:30:42 UTC
(In reply to Dobrica Pavlinusic from comment #40)
> Is there reason why we use
> 
> output_pref( { dt => dt_from_string( $subs->{startdate} ),     dateonly => 1
> } );
> 
> instead of
> 
> output_pref( { str => subs->{startdate}, dateonly => 1 } );
> 
> Second one has nice side-effect that it supports empty dates (which return
> as undef from database) in serial module. In our case, empty dates for
> planneddate and publisheddate produce application error base hash
> 
> { dt => dt_from_string( $line->{planneddate} ),   dateonly => 1 }
> 
> folds to
> 
> { dt , dateonly => 1 }
> 
> which makes dt = 'dateonly' which in turn is not object and result in
> application error.
> 
> Should I prepare separate bug for this change? Does it make sense?

Dobrica,
The removal of C4::Dates was quite long, and bug 15166 (which introduced the str param) has been done during this process.
Of course we should remove *all* occurrences of output_pref({dt = dt_from_string}); as it is dangerous.
It's on my todo list, I just need some motivation...
Please open the bug report and assign it to me, maybe it will bring me some energy to do it :)