Bug 16527 - Sticky due date calendar unexpected behaviour
Summary: Sticky due date calendar unexpected behaviour
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 10067
Blocks:
  Show dependency treegraph
 
Reported: 2016-05-16 12:56 UTC by Nick Clemens
Modified: 2019-06-27 09:24 UTC (History)
10 users (show)

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


Attachments
Bug 16527: Restore sticky due date behavior (1.59 KB, patch)
2016-05-21 08:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16527: Restore sticky due date behavior (1.66 KB, patch)
2016-05-22 19:47 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 16527: Restore sticky due date behavior (1.67 KB, patch)
2016-06-18 12:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 16527: Restore sticky due date behavior (1.74 KB, patch)
2016-06-19 15:43 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 Nick Clemens 2016-05-16 12:56:16 UTC
Seeing a few things here that seem unexpected:

1 - Set a date and click "Remember for session" and checkout a book
2 - Click the calendar to change date, today's date is highlighted, not the current selection
3 - Select a new date, make sure 'remember for session is still checked' and checkout a book
4 - The book recieves the correct due date, however, the new date is not remembered, reverts to last saved date
5 - Attempt to set a due date prior to current date, the notice that pops up reverts to the last saved date and not the selected date
Comment 1 Jonathan Druart 2016-05-17 13:29:55 UTC
It seems that it is a regression introduced by bug 10067.

I'd say that the following diff will fix it:

diff --git a/circ/circulation.pl b/circ/circulation.pl
index 66b3dcf..d7fcfbb 100755
--- a/circ/circulation.pl
+++ b/circ/circulation.pl
@@ -166,7 +166,7 @@ my $stickyduedate  = $query->param('stickyduedate') || $session->param('stickydu
 my $duedatespec    = $query->param('duedatespec')   || $session->param('stickyduedate');
 $duedatespec = eval { output_pref( { dt => dt_from_string( $duedatespec ), dateformat => 'iso' }); }
     if ( $duedatespec );
-my $restoreduedatespec  = $query->param('restoreduedatespec') || $session->param('stickyduedate') || $duedatespec;
+my $restoreduedatespec  = $query->param('restoreduedatespec') || $duedatespec || $session->param('stickyduedate');
 if ($restoreduedatespec eq "highholds_empty") {
     undef $restoreduedatespec;
 }

But I have no idea if it won't break bug 10067.

Eivin, could you have a look please?
Comment 2 Jonathan Druart 2016-05-20 17:52:58 UTC
This should be fixed before the next release (soon!)
Comment 3 Jonathan Druart 2016-05-21 08:14:43 UTC Comment hidden (obsolete)
Comment 4 Eivin Giske Skaaren 2016-05-21 11:03:03 UTC
I am without a koha dev env because my laptop died.. Installing another computer today.

The stickyduedate behaviour and 10067 is confusing because there is some jquery code that intercepts the request and I had to send that data to serverside somehow to be able to manipulate. (from the top of my head..)

If I am able to get a dev env up and running I will look into it.
Comment 5 Eivin Giske Skaaren 2016-05-22 19:47:42 UTC Comment hidden (obsolete)
Comment 6 Eivin Giske Skaaren 2016-05-22 19:57:27 UTC
This change seems to fix the issues raised by Nick but I also noticed when testing and trying to understand 10067 again that there has been other changes to the parts of Koha we are dealing with here.. (And the mix of TT, jQuery and Perl to handle this together with the highholds is pretty confusing.)

Another regression might be that the decreaseLoanHighHolds does not work anymore. There was several changes to that since last I looked into this and when testing on a new kohadevbox the checkout length was not decreased, even when the new checkbox "Don't decrease checkout length based on holds" was not selected.

That has nothing to do with this patch however so I signed off.
Comment 7 Jonathan Druart 2016-06-18 12:09:30 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2016-06-19 15:43:18 UTC
Created attachment 52550 [details] [review]
[PASSED QA] Bug 16527: Restore sticky due date behavior

Bug 10067 has introduced a regression on the sticky due date behavior:
If "remember for session" is checked and the due date is changed from 1
checkout to another, the old due date will be used.

The code added by bug 10067 is really confusing and it's hard to say
this patch won't introduce another regression.
The manipulation of $restoreduedatespec, $duedatespec and $stickyduedate
is not easy to follow.

Test plan:
Confirm that the regression is fixed and that this patch will not
introduced a regression on bug 10067 (good luck!)

Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Kyle M Hall 2016-06-24 11:46:41 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 10 Kirsten 2016-07-14 20:52:26 UTC
Can we have this fix backported to 3.22?
Comment 11 Frédéric Demians 2016-08-01 11:49:11 UTC
Pushed in 16.05. Will be in 16.05.02.
Comment 12 Julian Maurice 2016-08-16 12:20:51 UTC
Patch pushed to 3.22.x, will be in 3.22.10