Bug 9004

Summary: Talking Tech doesn't account for holidays when calculating a holds last pickup date
Product: Koha Reporter: Kyle M Hall <kyle>
Component: NoticesAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: gitbot, gmcharlt, jdemuth, jonathan.druart, joonas.kylmala, katrin.fischer, m.de.rooy, nick
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8367
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 8735    
Bug Blocks:    
Attachments: Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date.
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script.
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script.
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check
Bug 9004: (QA followup) Fix pod
Bug 9004: Use Koha::Calendar instead of C4::Calendar
Bug 9004: Use Koha::Calendar instead of C4::Calendar
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Perltidy script.
Bug 9004 - Talking Tech doesn't account for holidays when calculating a holds last pickup date - Add holidays check
Bug 9004: (QA followup) Fix pod
Bug 9004: Use Koha::Calendar instead of C4::Calendar

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 Joonas Kylmälä 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 (kidclamp) 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 (kidclamp) 2016-04-08 13:22:26 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens (kidclamp) 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 (kidclamp) 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!