Bug 10067 - decreaseLoanHighHolds messes with specify due date
Summary: decreaseLoanHighHolds messes with specify due date
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Eivin Giske Skaaren
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 16527
  Show dependency treegraph
 
Reported: 2013-04-16 16:25 UTC by Nicole C. Engard
Modified: 2018-06-04 20:10 UTC (History)
11 users (show)

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


Attachments
Bug 10067 fix due date issue (1.42 KB, patch)
2015-05-24 22:28 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (6.64 KB, patch)
2015-05-25 17:25 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (7.14 KB, patch)
2015-06-29 04:12 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (11.55 KB, patch)
2015-07-15 19:17 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Shortened loan (89.85 KB, image/png)
2015-08-31 20:33 UTC, Eivin Giske Skaaren
Details
Specify due date not changed (36.76 KB, image/png)
2015-08-31 20:34 UTC, Eivin Giske Skaaren
Details
Bug 10067 - decreaseLoanHighHolds messes with specify due date (11.60 KB, patch)
2015-09-03 03:14 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (8.29 KB, patch)
2015-09-07 16:46 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (7.70 KB, patch)
2015-09-07 16:48 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (7.61 KB, patch)
2015-09-21 19:00 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (11.55 KB, patch)
2015-09-22 16:12 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (11.55 KB, patch)
2015-09-22 16:21 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (8.04 KB, patch)
2015-09-22 16:28 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (7.96 KB, patch)
2015-11-02 17:31 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
warning appears (113.80 KB, image/png)
2015-11-02 17:52 UTC, Nicole Engard (ByWater)
Details
specify due date filled in (42.62 KB, image/png)
2015-11-02 17:53 UTC, Nicole Engard (ByWater)
Details
Bug 10067 - decreaseLoanHighHolds messes with specify due date (8.36 KB, patch)
2015-11-02 21:15 UTC, Eivin Giske Skaaren
Details | Diff | Splinter Review
Bug 10067 - decreaseLoanHighHolds messes with specify due date (8.42 KB, patch)
2015-11-02 21:22 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[PASSED QA] Bug 10067 - decreaseLoanHighHolds messes with specify due date (8.48 KB, patch)
2015-11-17 12:54 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 Nicole C. Engard 2013-04-16 16:25:14 UTC
It appears that when the criterion for the shortened loan period is met, not only does that particular item get the shortened loan period, so does any subsequent item checked out in that session. It appears that decreaseLoanHighHolds functionally affects Specify due date, and requires the staff member to click on Clear in order to make sure the correct loan period is applied for the next item(s).
Comment 1 Owen Leonard 2013-05-10 15:47:30 UTC
Confirmed in master.
Comment 2 Galen Charlton 2014-06-04 20:20:56 UTC
Summarizing some discussion in IRC today, it would be reasonable for the high holds functionality to simply announce that it has shortened the loan period, and not hook into the confirmation mechanism.  That way, it would need to touch the due date input widget at all.
Comment 3 Christopher Brannon 2014-06-04 20:33:09 UTC
How would this relate to a scenario where staff are actually using the Specify due date field?  For example, a longer loan period is specified.  Or perhaps a shorter loan period is specified.

Christopher
Comment 4 Viktor Sarge 2014-06-09 20:02:19 UTC
I've been giving this some thought and I don't mind the current workflow where you get a chance to tell a patron that the book will have a shorter loan period than usual. But the problem is that decreaseLoanHighHolds hooks into duedatespec and overwrites it. So even if you auto clear after using highholds you will lose any date you had entered manually. 

My two cents is that this could be solved with this workflow: 
1) When highholds is active pick upp anything stored in "duedatespec" and stick it into the new parameter "specifiedDueDateToRestore" 
2) Let highholds overwrite duedatespec as it currently do. 
3) When circulation.pl finds a non empty specifiedDueDateToRestore parameter stick this back into duedatespec. 

This way the due date from highholds will be the one off value it's supposed to be and manually defined due dates will be restored. 

I've been looking at 10067 from a few different angles and this is what I can come up with that does'nt change peoples current workflow. I have yet to discover any fatal flaws with this approach althoguh there might be several for all I know.
Comment 5 Christopher Brannon 2014-06-09 20:31:54 UTC
I would also suggest considering http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11565 while looking at this.  There are times you don't want to allow this feature to change the date.  For example, we have a bookmobile that visits a location once a month.  There needs to be a way this shortened due date can be overridden on the fly, or perhaps an issuing library can ignore this rule in the circulation rules.

I know this is an entirely separate functionality, but I think it should be kept in mind for something to work towards as this is being worked out.

That's my two cents.

Christopher
Comment 6 Eivin Giske Skaaren 2015-05-24 22:28:42 UTC Comment hidden (obsolete)
Comment 7 Eivin Giske Skaaren 2015-05-24 22:34:42 UTC
To test:

Apply the patch and try to check out items both that have enough holds that the decreaseLoanHighHolds function kicks in and without. See that the due date now is cleared if sticky is not selected and that the picked date stays put when sticky is selected. Also when the due date is restricted it is checked out with the correct date but the due date in the "Specify due date" box either is cleared or kept to the picked date depending on if sticky is selected.

Known limitation: Does not work too well when jumping between different parts of the system as it relies on the values posted to the circulation.pl script.

Sponsored-by: Halland County Library
Comment 8 Katrin Fischer 2015-05-24 22:38:21 UTC
Hi Eivin, could you make your last comment be part of your commit message please? And to make it easier for testers, could you please also add a step by step test plan to the commit message? Thx!
Comment 9 Eivin Giske Skaaren 2015-05-25 17:25:10 UTC Comment hidden (obsolete)
Comment 10 Viktor Sarge 2015-06-23 06:15:39 UTC
I've tested it and it does what it's supposed to do. Even with the listed limitations it's a bliss compared to the current situation. 

I will still leave it in "Needs signoff" however since Eivin is working on our behalf. Anyone else that's up for having a look at it?
Comment 11 Chris Cormack 2015-06-29 04:12:46 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2015-07-09 00:57:42 UTC
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
> 
> This does what it says it will do, however I think it might be a
> change of behaviour, in terms of the specify due date. It now clears
> it the date after ever item is checked out.
> 
> Previously the due date would stay, until the current borrower is
> dealt with. And remember for session sets it so that it stays until
> the staff member logs out, or clears it.
> 
> With the patch, now the date is clear after every check out .. this is
> a change that may annoy some people.

Hm I agree with Chris that this change of behaviour might not be good for every library and use case - so people might get annoyed. 

I'd prefer if the date was only cleared out after checking out an item where the due date gets shortened. Would that work?
Comment 13 Eivin Giske Skaaren 2015-07-09 07:19:21 UTC
Obviuously I am rather new to Koha but I actually thought it was the other way. Since you have the "Remember for session" checkbox and also probably a default due date in the system (perhaps even on item level? have not checked/can not remember right now) I found it strange that the value was not cleared.

Do we want the default behavior to be that the value is cleared only when the due date is shortened?
Comment 14 Chris Cormack 2015-07-09 08:38:52 UTC
The rule for developing on Koha is that we don't change behaviour that people will have been using for years. 
(The specify due date feature is about 8 years old, so suddenly changing the way it works will most definitely catch some people out)

The reason we have this rule is so people won't be scared to upgrade (it's also the rule why new features go in switched off by default on upgrade)

Hope this makes sense
Comment 15 Eivin Giske Skaaren 2015-07-15 19:17:06 UTC Comment hidden (obsolete)
Comment 16 Eivin Giske Skaaren 2015-07-16 09:24:13 UTC
Change in expected result: The date is not cleared anymore.
Comment 17 Nicole C. Engard 2015-07-16 17:27:27 UTC
This looks like it's signed off
Comment 18 Chris Cormack 2015-07-16 22:18:18 UTC
(In reply to Nicole C. Engard from comment #17)
> This looks like it's signed off

Hmm wrong bug by accident Nicole?
Comment 19 Christopher Brannon 2015-08-09 01:12:53 UTC
If I am testing correctly, and reading correctly, I am not seeing any benefit to this patch.  In it's current state, it is doing what it always did.  Can someone tell me what should be different in behavior with the patch applied?  Sorry.  Don't know if it just me or what.

Christopher
Comment 20 Eivin Giske Skaaren 2015-08-09 12:12:41 UTC
The difference is that the loan period is not shortened for the next scanned book.

Without the patch if you get a shortened loan period then the next scanned book will also get that same shortened period automatically.
Comment 21 Eivin Giske Skaaren 2015-08-31 20:33:13 UTC
I have just retested this and the difference is what I said earlier:

With the patch you can set the "specify due date" and when the item get shortened loan date "specify due date" will not be changed to the shortened date and you can keep on checking out items with the original date.

I have attached some screenshots.
Comment 22 Eivin Giske Skaaren 2015-08-31 20:33:53 UTC
Created attachment 42127 [details]
Shortened loan
Comment 23 Eivin Giske Skaaren 2015-08-31 20:34:29 UTC
Created attachment 42128 [details]
Specify due date not changed
Comment 24 Chris Cormack 2015-09-03 03:14:21 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-09-03 08:35:01 UTC
Please resubmit the same patch but without the code cleaning:

-my $stickyduedate  = $query->param('stickyduedate') || 
+my $stickyduedate       = $query->param('stickyduedate') || $session->param('stickyduedate');

This make 1/ the patch hard to read and 2/ all the other patches in the queue in conflict and won't apply anymore (13 at the moment).
Comment 26 Eivin Giske Skaaren 2015-09-07 16:46:50 UTC Comment hidden (obsolete)
Comment 27 Eivin Giske Skaaren 2015-09-07 16:48:49 UTC Comment hidden (obsolete)
Comment 28 Eivin Giske Skaaren 2015-09-07 16:51:13 UTC
I think the readability is not better when it is very inconsistent but I have restored the original inconsistencies :)
Comment 29 Jonathan Druart 2015-09-14 12:02:41 UTC
Comment on attachment 42438 [details] [review]
Bug 10067 - decreaseLoanHighHolds messes with specify due date

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

::: circ/circulation.pl
@@ +524,5 @@
> +
> +    if ($stickyduedate eq 'on') {
> +        $session->param( 'stickyduedate', $duedatespec );
> +    }
> +}

Without a good reason, this should be merged into the following block:

565 # save stickyduedate to session
566 if ($stickyduedate) { 
567     $session->param( 'stickyduedate', $duedatespec );                                      
568 } 

Something like:
if ( $stickyduedate or $restoreduedatespec ) {
    $duedatespec = $restoreduedatespec || $duedatespec;
    if ($stickyduedate) {
        $session->param( 'stickyduedate', $duedatespec );
    }
}
(not tested)

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
@@ +388,3 @@
>      <input type="hidden" name="borrowernumber" value="[% borrowernumber %]" />
>      <input type="hidden" name="duedatespec" value="[% duedatespec %]" />
> +    <input type="hidden" name="restoreduedatespec" />

Duplicated.
Comment 30 Eivin Giske Skaaren 2015-09-21 19:00:15 UTC Comment hidden (obsolete)
Comment 31 Eivin Giske Skaaren 2015-09-21 19:03:59 UTC
Hi Jonathan,

Yes there is a reason for that bit of code being outside of that block. 

It needs to be executed before the $template->param() that comes directly after on line 530.
Comment 32 Jonathan Druart 2015-09-22 09:16:27 UTC
(In reply to Eivin Giske Skaaren from comment #31)
> Hi Jonathan,
> 
> Yes there is a reason for that bit of code being outside of that block. 
> 
> It needs to be executed before the $template->param() that comes directly
> after on line 530.

Yes but you can move the existing block before the $template->param :)
So that the stickyduedate value is set to the session at only 1 place in the script.
Comment 33 Eivin Giske Skaaren 2015-09-22 16:12:47 UTC Comment hidden (obsolete)
Comment 34 Eivin Giske Skaaren 2015-09-22 16:21:22 UTC Comment hidden (obsolete)
Comment 35 Eivin Giske Skaaren 2015-09-22 16:28:59 UTC Comment hidden (obsolete)
Comment 36 Eivin Giske Skaaren 2015-09-22 16:30:53 UTC
Changed as suggested.
Comment 37 Jonathan Druart 2015-09-23 11:17:18 UTC
A signoff is needed for this last version.
Comment 38 Nicole Engard (ByWater) 2015-11-02 17:04:00 UTC
11:02 ~/kohaclone (master % u=)$ git qa 10067
From git://git.koha-community.org/koha
 * branch            master     -> FETCH_HEAD
Branch bug10067-qa set up to track remote branch master from origin.
Switched to a new branch 'bug10067-qa'
Bug 10067 - decreaseLoanHighHolds messes with specify due date

42794 - Bug 10067 - decreaseLoanHighHolds messes with specify due date

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 10067 - decreaseLoanHighHolds messes with specify due date
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
Auto-merging circ/circulation.pl
CONFLICT (content): Merge conflict in circ/circulation.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 10067 - decreaseLoanHighHolds messes with specify due date
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-10067---decreaseLoanHighHolds-messes-with-spec-ly2_w7.patch
11:03 ~/kohaclone (bug10067-qa *+%|AM u=)$
Comment 39 Eivin Giske Skaaren 2015-11-02 17:31:54 UTC Comment hidden (obsolete)
Comment 40 Nicole Engard (ByWater) 2015-11-02 17:52:50 UTC
Created attachment 44291 [details]
warning appears

The behavior hasn't changed. It still uses the set due date and so the date is filled in so the next check out I do, which has no holds on it is automatically due in 2 days instead of the original loan period.
Comment 41 Nicole Engard (ByWater) 2015-11-02 17:53:10 UTC
Created attachment 44292 [details]
specify due date filled in
Comment 42 Eivin Giske Skaaren 2015-11-02 21:15:13 UTC Comment hidden (obsolete)
Comment 43 Nicole C. Engard 2015-11-02 21:22:37 UTC Comment hidden (obsolete)
Comment 44 Katrin Fischer 2015-11-17 12:48:40 UTC
I am sorry, I have tested this up and down, but it doesn't work for me. The specified due date always gets set to the shortened due date and my previously specified date is lost :(
Comment 45 Katrin Fischer 2015-11-17 12:54:10 UTC
Created attachment 44908 [details] [review]
[PASSED QA] Bug 10067 - decreaseLoanHighHolds messes with specify due date

To test:

1. On a branch up to date with master: apply the patch.

2. Try to check out items both with and without holds.
(The decreaseLoanHighHolds function should be configured and kick in when testing an item.

3. Expected results are:
The due date date field should be cleared after checkout if "Remember for session"
is not selected. If it is selected then the chosen date should stay in the field even
if the warning about reduced loan time for an item with too many holds is displayed.
Also the correct due dates are shown in the table after a checkout.

Known limitation: Does not work too well when jumping between different parts
of the system as it relies on the values sent to the circulation.pl script.

Sponsored-by: Halland County Library

Refactored to both fix specific issue where date is changed
and still have the old behaviour with date kept intact.

Rebase
Remove code whitespace cleanup

Remove one more code whitespace cleanup

Removed duplicate tag

Refactored stickyduedate session parameter

Fix datefield set by highholds if blanc

Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 46 Katrin Fischer 2015-11-17 12:54:58 UTC
Ok, I tested again and after logging in and out again it started to work... I think my previous testing was flawed.
Comment 47 Eivin Giske Skaaren 2015-11-17 12:59:32 UTC
(In reply to Katrin Fischer from comment #46)
> Ok, I tested again and after logging in and out again it started to work...
> I think my previous testing was flawed.

Great, this has been redone and rebased a lot so I got a little bit worried there :)
Comment 48 Tomás Cohen Arazi 2015-11-17 13:06:52 UTC
Patch pushed to master.

Thanks Eivin!