Bug 14903 - Remove C4::Dates from circ/circulation.pl
Summary: Remove C4::Dates from circ/circulation.pl
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: Marc Véron
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 7899 (view as bug list)
Depends on:
Blocks: 14870 16780
  Show dependency treegraph
 
Reported: 2015-09-27 06:32 UTC by Marc Véron
Modified: 2016-06-23 15:05 UTC (History)
5 users (show)

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


Attachments
Bug 14903 - Remove C4::Dates from circ/circulation.pl (5.79 KB, patch)
2015-09-27 06:40 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - Remove C4::Dates from circ/circulation.pl (6.45 KB, patch)
2015-10-01 07:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) (13.20 KB, patch)
2015-10-09 16:59 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) (13.75 KB, patch)
2015-10-25 19:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) (13.80 KB, patch)
2015-10-27 14:13 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl (1.59 KB, patch)
2015-10-27 16:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl (1.72 KB, patch)
2015-10-28 16:23 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more) (13.87 KB, patch)
2015-10-29 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl (1.78 KB, patch)
2015-10-29 08:35 UTC, Jonathan Druart
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-09-27 06:32:58 UTC
Patch to remove deprectated C4::Dates from circ/circulation.pl
Comment 1 Marc Véron 2015-09-27 06:40:31 UTC Comment hidden (obsolete)
Comment 2 Héctor Eduardo Castro Avalos 2015-09-30 22:20:26 UTC
Before applying the patch I search and found the followin lines:

169  if ($duedatespec =~ C4::Dates->regexp('syspref')) {
178  our $todaysdate = C4::Dates->new->output('iso');

I do not know if there should be these lines!

If the intention in bug 14870 is to delete C4::Dates, this will launch an error or warning when the script will executed.
Comment 3 Héctor Eduardo Castro Avalos 2015-09-30 22:28:51 UTC
(In reply to Héctor Eduardo Castro Avalos from comment #2)
> Before applying the patch I search and found the followin lines:
> 
> 169  if ($duedatespec =~ C4::Dates->regexp('syspref')) {
> 178  our $todaysdate = C4::Dates->new->output('iso');
> 
> I do not know if there should be these lines!
> 
> If the intention in bug 14870 is to delete C4::Dates, this will launch an
> error or warning when the script will executed.

After applying the patch these lines continued there. The patch doesn't touch it.
Comment 4 Marc Véron 2015-10-01 07:58:41 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-10-09 16:29:30 UTC
Comment on attachment 42989 [details] [review]
Bug 14903 - Remove C4::Dates from circ/circulation.pl

New patch will follow with C4/Dates.pm included.
Comment 6 Marc Véron 2015-10-09 16:59:46 UTC Comment hidden (obsolete)
Comment 7 Marc Véron 2015-10-09 19:25:39 UTC
*** Bug 7899 has been marked as a duplicate of this bug. ***
Comment 8 Marc Véron 2015-10-23 09:34:34 UTC
Still applies / auto-merging
Comment 9 Mirko Tietgen 2015-10-23 13:34:51 UTC
Hi Marc,

it works mostly well for me. Among other things I tried changing the date format in between and using wrong preset dates in the URL like circulation.pl?borrowernumber=51&duedatespec=10%2F30%2F2015+23%3A59

While the old version shows warnings or even breaks ("30" in the "month" position leads to a software error because it is not between 1 and 12), with your patch the wrong format passed by URL is displayed in the calendar, but no warnings or errors appear and the correct date is used when checking out. 

(In reply to Marc Véron from comment #6)

> - Verify that data displays properly (including for users with a card
>   that expires in the  near future, see syspref 'NotifyBorrowerDeparture')

That breaks it for me:

> Template process failed: undef error - KohaDates: filter not found at /home/mirko/koha/C4/Templates.pm line 121.

As you seem to have tested this, is that taken care of by one of the other patches by any chance? I would sign off if it has a "depends on" that works.
Comment 10 Mirko Tietgen 2015-10-23 13:43:04 UTC
There is bug 14985 labelled as "part one". Maybe C4/Templates.pm is in a part two not yet on BZ?
Comment 11 Marc Véron 2015-10-23 21:44:16 UTC
Hi Mirko,

Thank you for the comments and for testing. 

Sorry, I have no time at teh moment to fined out what's wrong here; maybe I can come back to that on Sunday evening.

I think the circulation files are the most sensible ones,maybe it is a good idea to fix them in one of the very last steps of this loooong journey.

That said I really appreciate every sign-off and QA on the other C4::Dates Bugs.

BTW: If anybody wants to take one of the C4::Dates bugs to help to get them through, I'll be very grateful.

Marc
Comment 12 Marc Véron 2015-10-25 19:33:40 UTC
(In reply to Mirko Tietgen from comment #9)
 (...)
> ....using wrong preset dates in the URL like
> circulation.pl?borrowernumber=51&duedatespec=10%2F30%2F2015+23%3A59
> 
> While the old version shows warnings or even breaks ("30" in the "month"
> position leads to a software error because it is not between 1 and 12), with
> your patch the wrong format passed by URL is displayed in the calendar, but
> no warnings or errors appear and the correct date is used when checking out. 
(...)

MV: Found, will be fixed in amended patch: Date will not be displayed if params do not result in a correct date.

> (In reply to Marc Véron from comment #6)
> 
> > - Verify that data displays properly (including for users with a card
> >   that expires in the  near future, see syspref 'NotifyBorrowerDeparture')
> 
> That breaks it for me:
> 
> > Template process failed: undef error - KohaDates: filter not found at /home/mirko/koha/C4/Templates.pm line 121.

(...)
MV: Found, was due to a typo, will be fixed in amended patch.
Comment 13 Marc Véron 2015-10-25 19:38:20 UTC Comment hidden (obsolete)
Comment 14 Mirko Tietgen 2015-10-27 14:13:49 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-10-27 15:00:25 UTC
Comment on attachment 44072 [details] [review]
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more)

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

::: circ/circulation.pl
@@ +174,4 @@
>  
>  my $stickyduedate  = $query->param('stickyduedate') || $session->param('stickyduedate');
>  my $duedatespec    = $query->param('duedatespec')   || $session->param('stickyduedate');
> +$duedatespec = eval { output_pref( { dt => dt_from_string( $duedatespec ), dateformat => 'iso' }); };

No, only if duedatespec if given. Otherwise "Automatic due date" is always filled with now.

@@ +203,4 @@
>      $datedue = output_pref({ dt => dt_from_string, dateonly => 1, dateformat => 'iso' });
>      $datedue .= ' 23:59:00';
>  } elsif( $duedatespec_allow ) {
> +    if ($datedue) {

That's not the same code as before ($dateduespec vs $datedue), please double check.
Comment 16 Marc Véron 2015-10-27 16:53:25 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2015-10-27 16:54:47 UTC
Jonathan, is it OK to switch back to Signed-off with the followup?
Comment 18 Jonathan Druart 2015-10-28 11:15:20 UTC
(In reply to Marc Véron from comment #17)
> Jonathan, is it OK to switch back to Signed-off with the followup?

Yes because the following is still wrong:
$template->param( IMPOSSIBLE=>1, INVALID_DATE=>$datedue )

Test plan:
Set and force an invalid date for the due date and confirm you get a warning with the invalid date displayed.
Comment 19 Marc Véron 2015-10-28 16:23:44 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2015-10-28 16:27:25 UTC
Hi Jonathan,

I amended the patch.

However, I can not trigger the message. The datepicker does not allow to enter wrong dates (field is readonly).
Comment 21 Jonathan Druart 2015-10-29 08:35:12 UTC
Created attachment 44137 [details] [review]
Bug 14903 - Remove C4::Dates from circ/circulation.pl (and more)

Patch to remove deprectated C4::Dates from:
- circ/circulation.pl
- C4/Circulation.pm
- koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt

To test:
- Apply patch
- Go to the checkout site (Home > Circulation > Checkouts)
- Verify that data displays properly (including for users with a card
  that expires in the  near future, see syspref 'NotifyBorrowerDeparture')
- Search for regressions
- prove -v t/db_dependent/Circulation.t

(Amended following comment #9 / #11  25.10.2015 /mv)

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2015-10-29 08:35:18 UTC
Created attachment 44138 [details] [review]
Bug 14903 - (followup)Remove C4::Dates from circ/circulation.pl

Follow up to fix QA issues from comment #15
...and amended for comment #18

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Tomás Cohen Arazi 2015-10-29 15:04:57 UTC
Patches pushed to master.

Thanks Marc!