Description
Marc Véron
2015-10-06 21:51:21 UTC
Created attachment 43182 [details] [review] Bug 14969 - Remove C4::Dates from serials/*.pl files 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 To test: - Go to Home > Serials - Add, edit serials and routing lists with focus on date entry and display - Verify that information for serials on item detail pages look fine in OPAC and staff client as well - Try to find regressions Still applies Still applies Created attachment 44441 [details] [review] Bug 14969 - Remove C4::Dates from serials/*.pl files 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 To test: - Go to Home > Serials - Add, edit serials and routing lists with focus on date entry and display - Verify that information for serials on item detail pages look fine in OPAC and staff client as well - Try to find regressions Amended to make it apply ( serials/checkexpiration.pl ) 04.11.2015 / mv 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. 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 :-) Created attachment 44526 [details] [review] Bug 14969: (followup) Handle invalid date input This patch fixes software errors if invalid dates are given while adding or editing a subscription (see comment #5). Subscriptions with invalid dates will be saved with empty dates. A warning will appear on the subscription detail page Note: Before C4::Dates removal, wrong dates like 33/33/2033 created valid fantasy dates in the future. To test: - Create and edit subscriptions with valid dates, verify that subscriptions are correctly defined - Bypass datepickers by manually entering dates like 00/00/0000 or 33/33/2999, verify that subscriptions are saved, but a warning appears on the subscription's detail page. Created attachment 44527 [details] [review] Bug 14969: (followup) Handle invalid date input This patch fixes software errors if invalid dates are given while adding or editing a subscription (see comment #5). Subscriptions with invalid dates will be saved with empty dates. A warning will appear on the subscription detail page. This will be a rare case because normally datepickers are used for date entry. Notes: - Before C4::Dates removal, wrong dates like 33/33/2033 did not complain, they created valid but fantasy dates in the future. - A more sophisticated date validation can be implemented in a separate bug after completion of C4::Dates removal. To test: - Create and edit subscriptions with valid dates, verify that subscriptions are correctly defined - Bypass datepickers by manually entering dates like 00/00/0000 or 33/33/2999, verify that subscriptions are saved, but a warning appears on the subscription's detail page. 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 :-) Created attachment 44528 [details] [review] Bug 14969: (followup) Handle invalid date input This patch fixes software errors if invalid dates are given while adding or editing a subscription (see comment #5). Subscriptions with invalid dates will be saved with empty dates. A warning will appear on the subscription detail page. This will be a rare case because normally datepickers are used for date entry. Notes: - Before C4::Dates removal, wrong dates like 33/33/2033 did not complain, they created valid but fantasy dates in the future, e.g. 32/22/2099 resulted in 03/11/2100 - A more sophisticated date validation can be implemented in a separate bug after completion of C4::Dates removal. To test: - Create and edit subscriptions with valid dates, verify that subscriptions are correctly defined - Bypass datepickers by manually entering dates like 00/00/0000 or 33/33/2999, verify that subscriptions are saved, but a warning appears on the subscription's detail page. Created attachment 44549 [details] [review] Bug 14969 - Remove C4::Dates from serials/*.pl files 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 To test: - Go to Home > Serials - Add, edit serials and routing lists with focus on date entry and display - Verify that information for serials on item detail pages look fine in OPAC and staff client as well - Try to find regressions Amended to make it apply ( serials/checkexpiration.pl ) 04.11.2015 / mv Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 44550 [details] [review] Bug 14969: (followup) Handle invalid date input This patch fixes software errors if invalid dates are given while adding or editing a subscription (see comment #5). Subscriptions with invalid dates will be saved with empty dates. A warning will appear on the subscription detail page. This will be a rare case because normally datepickers are used for date entry. Notes: - Before C4::Dates removal, wrong dates like 33/33/2033 did not complain, they created valid but fantasy dates in the future, e.g. 32/22/2099 resulted in 03/11/2100 - A more sophisticated date validation can be implemented in a separate bug after completion of C4::Dates removal. To test: - Create and edit subscriptions with valid dates, verify that subscriptions are correctly defined - Bypass datepickers by manually entering dates like 00/00/0000 or 33/33/2999, verify that subscriptions are saved, but a warning appears on the subscription's detail page. Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net> Created attachment 44617 [details] [review] Bug 14969: (followup) Remove C4::Dates from C4/Serials.pm This is a follow-up removing stray C4::Dates The patch removes C4::Dates from file C4/Serials.pm. To test: - Go to Home > Serials - Add, edit serials and routing lists with focus on date entry and display - Verify that information for serials on item detail pages look fine in OPAC and staff client as well - Try to find regressions Created attachment 44618 [details] [review] Bug 14969: (followup) Remove C4::Dates from C4/Serials.pm This is a follow-up removing stray C4::Dates The patch removes C4::Dates from file C4/Serials.pm. To test: - Go to Home > Serials - Add, edit serials and routing lists with focus on date entry and display - Verify that information for serials on item detail pages look fine in OPAC and staff client as well - Try to find regressions 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 Created attachment 44694 [details] [review] Bug 14969:(followup) Remove C4::Dates from serials Follow up for comment #15 - 1/ Useless spaces removed (I used them to better align the code for readability - 2/ $tmpPlannedDate removed. I used it for better readibility. - 3/ Initializition of $date to '' removed - 4/ and 5/ Changes removed (they beloged together). (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 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 :) Created attachment 44735 [details] [review] 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. 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. I have missed bug 15166 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 Created attachment 44763 [details] [review] 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. It seems that I messed something up with 15171. It should apply cleanly now. Moved 15167 from "Depends on" to "See also" Created attachment 44775 [details] [review] 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> 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> Ah, and I tested this one on top of 15166 and 15171 ... or I thought I did *sigh*... but of course I was missing 15166. 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. Created attachment 44794 [details] [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. (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. Created attachment 44799 [details] [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. Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Sorry Marc - a recent change causes a merge conflict in checkexpiration.pl. Can you please take a look and fix? (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.) 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> 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> Patches pushed to master. Thanks Jonathan and Marc! Is there reason 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? (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 :) |