Bug 19626 - Database update for bug 12063 incorrectly calculates expirationdate for holds
Summary: Database update for bug 12063 incorrectly calculates expirationdate for holds
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 12063
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-15 12:56 UTC by Nick Clemens
Modified: 2017-11-26 16:26 UTC (History)
2 users (show)

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


Attachments
Bug 19626 - Calculate expiration date for existing holds based on waitingdate (1.39 KB, patch)
2017-11-15 13:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19626 - Calculate expiration date for existing holds based on waitingdate (1.72 KB, patch)
2017-11-16 18:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19626 - Calculate expiration date for existing holds based on waitingdate (1.81 KB, patch)
2017-11-17 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19626: (QA follow-up) Small optimization (1.64 KB, patch)
2017-11-17 08:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-11-15 12:56:58 UTC
Logic in short seems to be:
1 - Does hold already have an expirationdate?
    a- Keep it if before calculated expirationdate
2 - If not 1 then calculate expiration as curdate + delay (+ holidays if pref)

The problem is, before 12063 waiting holds didn't have an expirationdate, so a hold that has been waiting for 2 months will now get a waiting date after the upgrade.

I think line 14374 should be:
my $expirationdate = dt_from_string($hold->waitingdate);
Comment 1 Nick Clemens 2017-11-15 13:14:09 UTC
Created attachment 69152 [details] [review]
Bug 19626 - Calculate expiration date for existing holds based on waitingdate

To test:
1 - Have/create a 16.11 instance with some waiting holds
2 - Those holds should not have an expirationdate
3 - Make sure some of the holds are waiting longer than
ReservesMaxPickupDelay
BACKUP THE DB
4 - Upgrade to 17.05 (or later)
5 - Check the expirationdate for the holds
6 - The date wil be in the future (curdate + delay)
7 - Restore DB
8 - Apply patch
9 - Run the upgrade again
10 - expirationdate should now be based on waitingdate
Comment 2 Marcel de Rooy 2017-11-15 16:04:31 UTC
You dont catch:

        if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) {
            $expirationdate = $calendar->days_forward( dt_from_string(), $max_pickup_delay );
        }
Comment 3 Nick Clemens 2017-11-16 18:02:13 UTC
Created attachment 69183 [details] [review]
Bug 19626 - Calculate expiration date for existing holds based on waitingdate

To test:
1 - Have/create a 16.11 instance with some waiting holds
2 - Those holds should not have an expirationdate
3 - Make sure some of the holds are waiting longer than
ReservesMaxPickupDelay
BACKUP THE DB
4 - Upgrade to 17.05 (or later)
5 - Check the expirationdate for the holds
6 - The date wil be in the future (curdate + delay)
7 - Restore DB
8 - Apply patch
9 - Run the upgrade again
10 - expirationdate should now be based on waitingdate
Comment 4 Marcel de Rooy 2017-11-17 08:29:25 UTC
Created attachment 69188 [details] [review]
Bug 19626 - Calculate expiration date for existing holds based on waitingdate

To test:
1 - Have/create a 16.11 instance with some waiting holds
2 - Those holds should not have an expirationdate
3 - Make sure some of the holds are waiting longer than
ReservesMaxPickupDelay
BACKUP THE DB
4 - Upgrade to 17.05 (or later)
5 - Check the expirationdate for the holds
6 - The date wil be in the future (curdate + delay)
7 - Restore DB
8 - Apply patch
9 - Run the upgrade again
10 - expirationdate should now be based on waitingdate

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2017-11-17 08:29:29 UTC
Created attachment 69189 [details] [review]
Bug 19626: (QA follow-up) Small optimization

We are only using $calendar when the pref is set.
Date calculation can be moved in if-else structure.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-11-17 08:30:22 UTC
Not sure if this really is a major btw.
Combining SO and QA in one step.
Comment 7 Jonathan Druart 2017-11-26 16:26:45 UTC
Pushed to master for 17.11, thanks to everybody involved!