Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date
Summary: Talking Tech doesn't account for holidays when calculating a holds last picku...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on: 8735
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-01 12:08 UTC by Kyle M Hall
Modified: 2016-12-05 21:27 UTC (History)
8 users (show)

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


Attachments
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date. (8.79 KB, patch)
2012-11-01 15:38 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script. (8.37 KB, patch)
2012-11-02 12:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check (2.37 KB, patch)
2012-11-02 12:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script. (8.38 KB, patch)
2016-04-07 18:27 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check (2.38 KB, patch)
2016-04-07 18:27 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 9004: (QA followup) Fix pod (1.01 KB, patch)
2016-04-08 13:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9004: Use Koha::Calendar instead of C4::Calendar (2.91 KB, patch)
2016-04-08 14:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9004: Use Koha::Calendar instead of C4::Calendar (2.96 KB, patch)
2016-04-08 15:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script. (8.38 KB, patch)
2016-04-11 06:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check (2.38 KB, patch)
2016-04-11 06:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9004: (QA followup) Fix pod (1.00 KB, patch)
2016-04-11 06:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9004: Use Koha::Calendar instead of C4::Calendar (2.91 KB, patch)
2016-04-11 06:55 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 Kyle M Hall 2012-11-01 12:08:48 UTC
The Talking Tech outbound script adds the ReservesMaxPickUpDelay to the waitingdate of a reserve to calculate its last pickup day. 

However, this day may fall on a holiday. In this case, the last day to pick the item up should be pushed to the next open day.
Comment 1 Kyle M Hall 2012-11-01 15:38:46 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2012-11-01 15:41:27 UTC
I perltidy'ed the file which has obscured my changes. The only non-formatting change is at lines 296-298. It just checks to see if the pickup date is a holiday, and if it is loops day by day until if finds the next open day for that library.
Comment 3 Jared Camins-Esakov 2012-11-01 15:43:09 UTC
(In reply to comment #2)
> I perltidy'ed the file which has obscured my changes. The only
> non-formatting change is at lines 296-298. It just checks to see if the
> pickup date is a holiday, and if it is loops day by day until if finds the
> next open day for that library.

Could you please break this into two patches? Either order is fine.
Comment 4 Kyle M Hall 2012-11-02 12:31:15 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2012-11-02 12:31:23 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-11-02 12:33:31 UTC
> Could you please break this into two patches? Either order is fine.

No problem! Again, the main change is the addition of the loop starting at line 295. I also switched from using arrays for the date parts to named variables, in the interest of self-documenting code.
Comment 7 Owen Leonard 2013-03-19 00:00:02 UTC
How is this going to get tested?
Comment 8 Galen Charlton 2013-06-17 21:31:12 UTC
I'm adding dependencies on two other bugs concerned with adjusting the max pickup date for holidays.  The bugs are interlinked in a somewhat tangled way.  As it would be awkward if the iTiva export, the hold expiration cronjob, and the hold display were to have different notions of exactly when a waiting hold has sat on the shelf too long, any final patches for all three bugs will almost certainly be pushed as a unit.
Comment 9 I'm just a bot 2013-09-23 19:11:32 UTC
This bug depends on 8367 which is in status Patch doesn't apply
Comment 10 I'm just a bot 2013-09-23 19:50:23 UTC
This bug depends on 8367 which is in status Patch doesn't apply
Comment 11 I'm just a bot 2013-09-23 19:57:13 UTC
This bug depends on 8367 which is in status Patch doesn't apply
Comment 12 I'm just a bot 2013-10-24 19:26:26 UTC
This bug depends on 8367 which is in status Patch doesn't apply
Comment 13 Marcel de Rooy 2014-02-07 20:03:53 UTC
Setting status to BLOCKED since this patch depends on two patches that do not apply resp. failed qa.
Comment 14 j.kylmala 2015-08-31 10:44:40 UTC
(In reply to Kyle M Hall from comment #5)
> Created attachment 13213 [details] [review] [review]
> Bug 9004 - Talking Tech doesn't account for holidays when calculating a
> holds last pickup date - Add holidays check

Code looks good to me but I'm unable to test this.
Comment 15 Nick Clemens 2016-03-29 13:33:14 UTC
Test plan:
 0 - Set ReservesMaxPickUpDelay to some value greater than 0
 1 - Place a hold for a patron
 2 - Check in and confirm the hold to set status to waiting
 3 - On patron screen there should be a message "Waiting at X until d/a/te"
 4 - Run the itiva outbound cronjob script:
perl /path/to/koha/misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl -o ~/itiva.tmp -w 0 --type=RESERVE
 5 - Check the output file, the max waiting date seen above should be there
 6 - Add a holiday (or several) on the max waiting date
 7 - Run the script again
 8 - Date should eb unchanged in file
 9 - Apply patch
10 - Run the script again
11 - Date should now be moved past the holidays set in step 6
Comment 16 Nicole C. Engard 2016-04-07 18:27:25 UTC Comment hidden (obsolete)
Comment 17 Nicole C. Engard 2016-04-07 18:27:30 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-04-08 13:12:34 UTC
 FAIL   misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl
   FAIL   pod
                 in file misc/cronjobs/thirdparty/TalkingTech_itiva_outbound.pl
                 
                *** ERROR: 
                =item without previous =over

I cannot test this patch, but I think we can trust ByWater for the signoff.
It would have been better to use DateTime and Koha::Calendar to do the date calculation.
Comment 19 Nick Clemens 2016-04-08 13:22:26 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2016-04-08 13:24:05 UTC
No iTiva account is required for testing, this simply alters the data in the file generated by the script.

Note:the output to screen option for this script doesn't appear to function, results must be saved to a file to view.
Comment 21 Jonathan Druart 2016-04-08 14:52:28 UTC Comment hidden (obsolete)
Comment 22 Jonathan Druart 2016-04-08 14:55:28 UTC
Hi Nick,
Any chances to get a signoff on this one?
I have never used this script before so I may have missed something!
Comment 23 Nick Clemens 2016-04-08 15:00:14 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2016-04-11 06:55:10 UTC
Created attachment 50111 [details] [review]
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script.

Signed-off-by: Chris William <chrisw@pascolibraries.org>
Comment 25 Jonathan Druart 2016-04-11 06:55:24 UTC
Created attachment 50112 [details] [review]
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check

Signed-off-by: Chris William <chrisw@pascolibraries.org>
Comment 26 Jonathan Druart 2016-04-11 06:55:28 UTC
Created attachment 50113 [details] [review]
Bug 9004: (QA followup) Fix pod
Comment 27 Jonathan Druart 2016-04-11 06:55:49 UTC
Created attachment 50114 [details] [review]
Bug 9004: Use Koha::Calendar instead of C4::Calendar

This patch tries to make the code more readable using Koha::Calendar
instead of deprecated C4::Calendar and Date::Calc
Comment 28 Kyle M Hall 2016-04-29 12:05:36 UTC
Pushed to master for the May 2016 release!