Bug 12063 - Change date calculation for reserve expiration to skip all holidays
Summary: Change date calculation for reserve expiration to skip all holidays
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19626 19260
  Show dependency treegraph
 
Reported: 2014-04-10 09:54 UTC by paxed
Modified: 2017-11-15 12:57 UTC (History)
16 users (show)

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


Attachments
Change date calculation for reserve expiration to skip all holidays (5.61 KB, patch)
2014-04-10 09:54 UTC, paxed
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (21.19 KB, patch)
2016-06-20 10:16 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (21.18 KB, patch)
2016-06-30 08:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database (7.69 KB, patch)
2016-06-30 08:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (21.25 KB, patch)
2016-07-05 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database (7.75 KB, patch)
2016-07-05 11:11 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (20.76 KB, patch)
2017-01-18 14:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database (7.83 KB, patch)
2017-01-18 14:14 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one (1.49 KB, patch)
2017-01-18 14:15 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one (3.81 KB, patch)
2017-01-18 15:21 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one (3.97 KB, patch)
2017-03-21 15:26 UTC, Andreas Hedström Mace
Details | Diff | Splinter Review
Bug 12063 - Fix unit tests (2.82 KB, patch)
2017-03-22 12:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Fix unit tests (2.12 KB, patch)
2017-03-22 15:17 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Fix QA failures (10.46 KB, patch)
2017-03-30 09:57 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (20.78 KB, patch)
2017-04-10 13:40 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database (7.83 KB, patch)
2017-04-10 13:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one (3.97 KB, patch)
2017-04-10 13:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Fix unit tests (2.12 KB, patch)
2017-04-10 13:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Fix QA failures (11.44 KB, patch)
2017-04-10 13:41 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - make perl atomic update follow skeleton.perl (3.33 KB, patch)
2017-04-20 14:22 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063 - Remove checking of ExpireReservesMaxPickUpDelay in CancelExpiredReserves(). Koha::Hold::set_waiting calculate expiration date from today instead of hold's waiting date. (2.43 KB, patch)
2017-05-04 08:56 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 12063: Change date calculation for reserve expiration to skip all holiday (20.87 KB, patch)
2017-05-04 12:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database (7.93 KB, patch)
2017-05-04 12:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one (4.07 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - Fix unit tests (2.21 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - Fix QA failures (11.53 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - make perl atomic update follow skeleton.perl (3.42 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063 - Remove checking of ExpireReservesMaxPickUpDelay in CancelExpiredReserves(). Koha::Hold::set_waiting calculate expiration date from today instead of hold's waiting date. (2.52 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063: [QA Follow-up] Adjustments to db revision (3.46 KB, patch)
2017-05-04 12:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12063: [QA Follow-up] Small change of two test scripts (2.14 KB, patch)
2017-05-04 12:23 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 paxed 2014-04-10 09:54:24 UTC Comment hidden (obsolete)
Comment 1 Katrin Fischer 2014-04-15 08:37:33 UTC
Adding a see also to bug 8735 as both seem to be related.
Comment 2 paxed 2014-04-15 08:58:37 UTC
(In reply to Katrin Fischer from comment #1)
> Adding a see also to bug 8735 as both seem to be related.

Right - that bug only moves the date forward if the end date happens to be a closed date.
This one will not count holidays into the delay at all. So, different needs.
Comment 3 Koha Team Lyon 3 2016-02-09 13:55:53 UTC
Hi,
Does anybody still work on that ?
It will be great that the number of day in ReservesMaxPickUpDelay take into account only the open days of the library.
If a patron have 3 days to pick up his hold and that the libray is closed 2 days just after, he will have juste one day left... 

Thank you
Sonia
Comment 4 Alex Arnaud 2016-06-20 10:16:21 UTC Comment hidden (obsolete)
Comment 5 Alex Arnaud 2016-06-20 10:25:50 UTC
Sorry for erasing the previous patch but this BZ was not progressing for more than 2 years...

Here is an other approach: Holds expiration date is set one time (when reserves become waiting) so we no longer have to calculate it each time we need.

When setting the expiration date, all these system preferences are checked: ExpireReservesMaxPickUpDelay, ReservesMaxPickUpDelay and ExcludeHolidaysFromMaxPickUpDelay (new).

ReservesMaxPickUpDelay is used when checking for reserves to cancel.
Comment 6 Koha Team Lyon 3 2016-06-29 14:02:52 UTC
It works as described for the calculation of expirationdate
ExcludeHolidaysFromMaxPickUpDelay is activate and ReservesMaxPickUpDelay=5. Several closed days are put in the calendar and I have this :
29-06-2016 - open : waitingdate
30-06-2016 - closed : skip
01-07-2016 - closed : skip
02-07-2016 - open : day 1
03-07-2016 - closed : skip
04-07-2016 - closed : skip
05-07-2016 - open : day 2
06-07-2016 - open : day 3
07-07-2016 - closed
08-07-2016 - open : day 4
09-07-2016 - open : day 5 -> expirationdate

ExcludeHolidaysFromMaxPickUpDelay is activate and ReservesMaxPickUpDelay=5. Several closed days :

29-06-2016 - open : waitingdate
30-06-2016 - closed : day 1
01-07-2016 - closed : day 2
02-07-2016 - open : day 3
03-07-2016 - closed : day 4
04-07-2016 - closed : day 5 -> expirationdate

But, cancelation with misc/cronjobs/holds/cancel_expired_holds.pl doesn't work...
Comment 7 Alex Arnaud 2016-06-30 08:01:02 UTC Comment hidden (obsolete)
Comment 8 Alex Arnaud 2016-06-30 08:01:08 UTC Comment hidden (obsolete)
Comment 9 sandboxes@biblibre.com 2016-07-05 11:11:18 UTC
Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 10 sandboxes@biblibre.com 2016-07-05 11:11:48 UTC Comment hidden (obsolete)
Comment 11 sandboxes@biblibre.com 2016-07-05 11:11:51 UTC Comment hidden (obsolete)
Comment 12 Koha Team Lyon 3 2016-07-05 11:22:46 UTC
tested with several days closed on the library. It works as described for the calculation of expirationdate with
ExcludeHolidaysFromMaxPickUpDelay activated/desactivated and ReservesMaxPickUpDelay=5.

Cancelation with cancelation with misc/cronjobs/holds/cancel_expired_holds.pl (launched on the server by biblibre staff) works normaly. 
I tried to activate/desactivate the syspref ExpireReservesOnHolidays : if it was on "don't allow" and today was closed on the calendar, holds expired are not cancelled. If it was on "allow", holds are cancelled.
Fees are applied if something is set in ExpireReservesMaxPickUpDelayCharge syspref.

So works as expected :)
Comment 13 Katrin Fischer 2016-07-15 08:41:08 UTC
Hm, I have looked at the description and the code of this, but I have concerns about the use of the expirationdate column in the reserves table.

I think with this patch, it will be used for 2 different things:
1) The date set by the patron when placing the hold "I don't need this after..."
2) The date a waiting hold expires

It looks like when setting the hold waiting, this will overwrite the expiration date set by the patron. Is this what we want? My feeling is no. I think we shouldn't change the date the user set to avoid confusion (see also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12204#c10)
Comment 14 Marcel de Rooy 2016-07-19 11:50:27 UTC
Hm. I am not in favor of blindly overwriting expiredate for reserves that already have an expiry date, entered by the patron himself.

In this case we could probably make a compromise: if expire < today + delay then leave it as-is, else expire = today + delay.
Although the else should not be necessary, the cronjob is intelligent too :)

This change is not dramatic, but I would prefer an adjustment.
Comment 15 Katrin Fischer 2016-07-19 12:18:10 UTC
I wonder if it could be irritatng to the user to find the date entered changed. 
I am not sure if libraries print those in notices, but they show up in the OPAC.
Comment 16 Koha Team Lyon 3 2016-07-20 09:21:35 UTC
Hi Katrin, Hi Marcel,
For what I have understood, but I maybe wrong, patrons can put an expirationdate to say that they're not interstesting in this reservation if it's not available for them after this date.
Here, we change the date after the item was check-in, so available for the patron. In this case, the first information isn't useful anymore.
I haven't tested if there's a change of behaviour if the patron isn't the first in the holding list, but I don't think so.
For a functional point of view, I don't think that's an issue to overwrite expirationdate. 
But, It can be perhaps a problem if the library want to make statistics to know an average of the time between the expirationdate and the waitingdate... or some tricky things we sometimes want to know.
And, I can undertstand that it m
Comment 17 Koha Team Lyon 3 2016-07-20 09:28:54 UTC
Hi Katrin, Hi Marcel,
From what I have understood, but I may be wrong, patrons can put an expirationdate to say that they're not interested in this reservation if it's not available for them after this date.
Here, we change the date after the item was check-in, so available for the patron. In this case, the first information isn't useful anymore.
I haven't tested if there's a change of behaviour if the patron isn't the first in the holding list, but I don't think so.
From a functional point of view, I don't think that's an issue to overwrite expirationdate. 
But, It can be perhaps a problem if the library want to make statistics to know an average of the time between the expirationdate and the waitingdate... or some tricky things we sometimes wants to know.
And, I can understand that it may be not in the "coding line" and that's a dirty way of doing... you can say that better than me.

Sonia
Comment 18 Andreas Hedström Mace 2016-10-20 13:55:36 UTC
Without having actually tested this bug, only reading the discussion, I would say that I have to agree with Sonia. 

As this new date is set after the material is checked in, it doesn't appear (at least to me) that the expire date set by the patron would be valuable anymore. And the added value of excluding dates that the library is closed from the number of day in ReservesMaxPickUpDelay, seem greater than any negative impact.
Comment 19 Marcel de Rooy 2016-11-11 16:12:48 UTC
(In reply to Andreas Hedström Mace from comment #18)
> Without having actually tested this bug, only reading the discussion, I
> would say that I have to agree with Sonia. 
> 
> As this new date is set after the material is checked in, it doesn't appear
> (at least to me) that the expire date set by the patron would be valuable
> anymore. And the added value of excluding dates that the library is closed
> from the number of day in ReservesMaxPickUpDelay, seem greater than any
> negative impact.

Hi Andreas,
Thx for your feedback.
I read it again too now. This patch tries to achieve its goal by modifying the expirationdate, while it actually should change this select in CancelExpiredReserves?

my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS( waitingdate ) > ? AND found = 'W' AND priority = 0";

If the holiday stuff is taken into account here, I am having the impression that we achieve the same. Mabye Sonia or Alex can respond to that?
Comment 20 Viccy Kemp 2016-12-21 20:56:09 UTC
Flower Mound Public Library has noticed that reserves DO NOT TAKE INTO ACCOUNT closed holidays. What exactly is the point of marking up a calendar if the software does not know what to do with it. That is nuts. We have been live on Koha for four and a half months now and during both Thanksgiving and now Christmas the closed days are not being accounted for in the holds. Please, someone, figure out how to make this work.
Comment 21 Andreas Hedström Mace 2017-01-10 14:27:41 UTC
(In reply to Marcel de Rooy from comment #19)
> Hi Andreas,
> Thx for your feedback.
> I read it again too now. This patch tries to achieve its goal by modifying
> the expirationdate, while it actually should change this select in
> CancelExpiredReserves?
> 
> my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS(
> waitingdate ) > ? AND found = 'W' AND priority = 0";
> 
> If the holiday stuff is taken into account here, I am having the impression
> that we achieve the same. Mabye Sonia or Alex can respond to that?

Any word from Sonia or Alex regarding this (the suggestion from Marcel?)? We would very much like this to move forward, as is holiday is now creating problems with holds waiting on the shelves.
Comment 22 Alex Arnaud 2017-01-11 09:48:02 UTC
For me there is two case:

1) The expiration date set by the patron is higher than the calculated expiration date (that take into account pickup delay and holiday). In this case, i think it is quite fair to overwrite the patron expiration date since he is not supposed to keep the item waiting beyond the library delay. This is what this patch does.

2) The patron expiration date is before the calculated expiration date. As the patron don't need the item after its own date, we could overwrite expirationdate with the one calculated. We need a adjustment to have this behavior.
Comment 23 Marcel de Rooy 2017-01-11 13:39:30 UTC
(In reply to Alex Arnaud from comment #22)
> For me there is two case:
> 
> 1) The expiration date set by the patron is higher than the calculated
> expiration date (that take into account pickup delay and holiday). In this
> case, i think it is quite fair to overwrite the patron expiration date since
> he is not supposed to keep the item waiting beyond the library delay. This
> is what this patch does.
> 
> 2) The patron expiration date is before the calculated expiration date. As
> the patron don't need the item after its own date, we could overwrite
> expirationdate with the one calculated. We need a adjustment to have this
> behavior.

As indicated before, I would not overwrite the expiration date.
What about comment19 ?
Comment 24 Alex Arnaud 2017-01-11 14:31:18 UTC
(In reply to Marcel de Rooy from comment #23)
> (In reply to Alex Arnaud from comment #22)
> > For me there is two case:
> > 
> > 1) The expiration date set by the patron is higher than the calculated
> > expiration date (that take into account pickup delay and holiday). In this
> > case, i think it is quite fair to overwrite the patron expiration date since
> > he is not supposed to keep the item waiting beyond the library delay. This
> > is what this patch does.
> > 
> > 2) The patron expiration date is before the calculated expiration date. As
> > the patron don't need the item after its own date, we could overwrite
> > expirationdate with the one calculated. We need a adjustment to have this
> > behavior.
> 
> As indicated before, I would not overwrite the expiration date.
> What about comment19 ?

Not sure to understand. (In reply to Marcel de Rooy from comment #19)
> (In reply to Andreas Hedström Mace from comment #18)
> > Without having actually tested this bug, only reading the discussion, I
> > would say that I have to agree with Sonia. 
> > 
> > As this new date is set after the material is checked in, it doesn't appear
> > (at least to me) that the expire date set by the patron would be valuable
> > anymore. And the added value of excluding dates that the library is closed
> > from the number of day in ReservesMaxPickUpDelay, seem greater than any
> > negative impact.
> 
> Hi Andreas,
> Thx for your feedback.
> I read it again too now. This patch tries to achieve its goal by modifying
> the expirationdate, while it actually should change this select in
> CancelExpiredReserves?
> 
> my $query = "SELECT * FROM reserves WHERE TO_DAYS( NOW() ) - TO_DAYS(
> waitingdate ) > ? AND found = 'W' AND priority = 0";
> 
> If the holiday stuff is taken into account here, I am having the impression
> that we achieve the same. Mabye Sonia or Alex can respond to that?

The code here is just simplified since the expirationdate (with max pickup delay + holiday) has already been set at the check in time. So we just need to fetch reserves whose expirationdate is over.
Comment 25 Viccy Kemp 2017-01-11 15:35:52 UTC
I do not understand what Alex and Marcel are working on here. All I know is it appears that reserves do not take into account the dates when the library is closed. Today, the holds are showing they expire on January 16, but the library is closed that day and this is defined in the calendar. 
WHY IS THIS NOT TAKEN INTO ACCOUNT ON THE EXPIRY DATE?
Comment 26 Alex Arnaud 2017-01-11 16:02:30 UTC
(In reply to Viccy Kemp from comment #25)
> I do not understand what Alex and Marcel are working on here. All I know is
> it appears that reserves do not take into account the dates when the library
> is closed. Today, the holds are showing they expire on January 16, but the
> library is closed that day and this is defined in the calendar. 
> WHY IS THIS NOT TAKEN INTO ACCOUNT ON THE EXPIRY DATE?

Hello Viccy,

The purpose of this patch is that the expiration date of reserves being calculated one time when the item is checked in and that this date take into account the pickup delay set by the library *AND* the potential closed days during the period until the delay.

Let's admit:

  - a pickup delay set to 10 (days),
  - an item checked in at 01/01/2017,
  - so hold expires at 11/01/2017,
  - but library is closed from 04/01/2017 to 06/01/2017 (3 days),
  - if ExcludeHolidaysFromMaxPickUpDelay system preference is enabled, the expiration date will be extended with these 3 days. So until 14/01/2017.

But, the case of holds expiring on a closed day is already handled in koha by disabling ExpireReservesOnHolidays if i'm not wrong.
Comment 27 Koha Team Lyon 3 2017-01-17 10:28:06 UTC
Hello,
I'm afraid that we are mired in a never ending discussion.
For me, as I have already sayd, there's no reason to keep expirationday after the hold is available and waiting to be picked up, as this date is meaning for users "I don't need this hold if it isn't available for me after this date".

But, I can see that it bothers Marcel so I propose that we try to find new solutions to help this feature to be pushed in Koha (because it's really important for many libraries).

So, I don't know if it's valuable but it seems to me that it could at least be simple, what do you think about creating a new field where we can stock a "pickupexpirationdate" or something like that ?

Marcel, Alex ? Is it possible ?
Comment 28 Alex Arnaud 2017-01-17 11:07:39 UTC
(In reply to Koha Team Lyon 3 from comment #27)
> Hello,
> I'm afraid that we are mired in a never ending discussion.
> For me, as I have already sayd, there's no reason to keep expirationday
> after the hold is available and waiting to be picked up, as this date is
> meaning for users "I don't need this hold if it isn't available for me after
> this date".
> 
> But, I can see that it bothers Marcel so I propose that we try to find new
> solutions to help this feature to be pushed in Koha (because it's really
> important for many libraries).
> 
> So, I don't know if it's valuable but it seems to me that it could at least
> be simple, what do you think about creating a new field where we can stock a
> "pickupexpirationdate" or something like that ?
> 
> Marcel, Alex ? Is it possible ?

Hello Sonia,

I'm not comfortable with having two expirationdate fields. And i think we can spare ourselves from adding a new field by doing what Marcel suggests previously:
  override the expiration date only if the one requested by the patron is before the calculated one.

My two cents
Comment 29 Marcel de Rooy 2017-01-17 11:11:00 UTC
(In reply to Alex Arnaud from comment #28)
> 
> Hello Sonia,
> 
> I'm not comfortable with having two expirationdate fields. And i think we
> can spare ourselves from adding a new field by doing what Marcel suggests
> previously:
>   override the expiration date only if the one requested by the patron is
> before the calculated one.
> 
> My two cents

Yeah, sounds better :)
Comment 30 Koha Team Lyon 3 2017-01-17 14:00:24 UTC
Hurrah! We have an agreement ;-)
Is it difficult to add this verification to the patch ?
We can re-test it and it can pursue his way toward the "Pushed to master" status...

[in aparte, I'm not totally sure that it's really understandable for a user..
a borrower who said that he doesn't need his hold after friday 20th January (for exemple), and this item is available the thursday 19th january with a pickup delay of 3 days (rules of the library. In our Library, this what we send in hold notice). The item will return in stacks the 20th January instead of monday the 23th. 
We would see after this patch how our users react and we could open a new ticket if necessary]
Comment 31 Andreas Hedström Mace 2017-01-17 15:47:05 UTC
Brilliant! I hope this is doable, and we are also able to test and (possibly) sign off!
Comment 32 Alex Arnaud 2017-01-18 14:14:47 UTC
Created attachment 59161 [details] [review]
Bug 12063: Change date calculation for reserve expiration to skip all holiday

This patch makes koha automatically set expiration date when reserves become
waitting. Also it adds a new syspref "ExcludeHolidaysFromMaxPickUpDelay" that allows to
take holidays into account while calculating expiration date.

Test plan:

  - Install this patch and run updatedatabase.pl script,
  - allow ExpireReservesMaxPickUpDelay in system preferences,
  - set ReservesMaxPickUpDelay to 5.

  - Place an hold on a checked out item and check in this item:
    The hold's expiration date should be today + 5.

  - Allow ExcludeHolidaysFromMaxPickUpDelay in system preferences,
  - add holiday during this pickup delay period,
  - Create a new hold and make it comes waitting:
    The hold's expiration date should be today + 5 + number of closed
    day(s).

Also:
  - Check that ExpireReservesOnHolidays syspref works again
    without ExcludeHolidaysFromMaxPickUpDelay.
  - Check that cancel fees apply again if wanted.

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 33 Alex Arnaud 2017-01-18 14:14:56 UTC
Created attachment 59162 [details] [review]
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 34 Alex Arnaud 2017-01-18 14:15:04 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2017-01-18 14:37:22 UTC
I would like to QA this patch set. Setting QA contact
Comment 36 Alex Arnaud 2017-01-18 15:21:10 UTC
Created attachment 59165 [details] [review]
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one
Comment 37 Andreas Hedström Mace 2017-02-13 13:51:49 UTC
Tested and everything works as intended, except that it is not possible to cancel holds with the cronjob in misc/cronjobs/holds/cancel_expired_holds.pl, and thus not possible to set a cancelation fee.

However, with the ExpireReservesOnHolidays syspref set to allow, and without ExcludeHolidaysFromMaxPickUpDelay (set to don’t allow), the hold is cancelled and sets the fee. 

I don't think this is the desired behaviour (one might want to exclude holidays but still apply a fee for those who doesn’t pick up a reserved book), so I'm setting this to patch doesn't apply.

If someone disagrees, feel free to change the status.
Comment 38 Marcel de Rooy 2017-02-13 13:53:39 UTC
(In reply to Andreas Hedström Mace from comment #37)
> Tested and everything works as intended, except that it is not possible to
> cancel holds with the cronjob in
> misc/cronjobs/holds/cancel_expired_holds.pl, and thus not possible to set a
> cancelation fee.
> 
> However, with the ExpireReservesOnHolidays syspref set to allow, and without
> ExcludeHolidaysFromMaxPickUpDelay (set to don’t allow), the hold is
> cancelled and sets the fee. 
> 
> I don't think this is the desired behaviour (one might want to exclude
> holidays but still apply a fee for those who doesn’t pick up a reserved
> book), so I'm setting this to patch doesn't apply.
> 
> If someone disagrees, feel free to change the status.

Sounds like a Failed QA.
Patch doesnt apply is meant for patches needing a rebase.
Comment 39 Alex Arnaud 2017-03-20 11:03:01 UTC
(In reply to Andreas Hedström Mace from comment #37)
> Tested and everything works as intended, except that it is not possible to
> cancel holds with the cronjob in
> misc/cronjobs/holds/cancel_expired_holds.pl, and thus not possible to set a
> cancelation fee.
> 
> However, with the ExpireReservesOnHolidays syspref set to allow, and without
> ExcludeHolidaysFromMaxPickUpDelay (set to don’t allow), the hold is
> cancelled and sets the fee. 
> 
> I don't think this is the desired behaviour (one might want to exclude
> holidays but still apply a fee for those who doesn’t pick up a reserved
> book), so I'm setting this to patch doesn't apply.
> 
> If someone disagrees, feel free to change the status.

Hello Andreas,

I didn't managed to reproduce this issue.
I have an expired hold. ExpireReservesOnHolidays is set to "don't allow". ExcludeHolidaysFromMaxPickUpDelay is set to "allow", i'm not on a holiday period and ExpireReservesMaxPickUpDelayCharge is set to 11.

Launch misc/cronjobs/holds/cancel_expired_holds.pl:
  1) hold is canceled,
  2) related borrower is charged
Comment 40 Koha Team Lyon 3 2017-03-21 14:19:24 UTC
hello Andreas,
I have tested one more time but I can't reproduce your issue.

you said : "one might want to exclude holidays but still apply a fee for those who doesn’t pick up a reserved book"

I try with those syspref : 
ExcludeHolidaysFromMaxPickUpDelay = allow
ExpireReservesMaxPickUpDelay= allow
ExpireReservesMaxPickUpDelayCharge=1
ReservesMaxPickUpDelay=1

That seems to be the setting translation of your sentence.
After launching misc/cronjobs/holds/cancel_expired_holds.pl, holds expired are canceled and a fee of 1 € is charged to borrowers.

Perhaps, I haven't really understood what is the problem..


(I assume that the script misc/cronjobs/holds/cancel_expired_holds.pl will have no effect if ExpireReservesMaxPickUpDelay is set to "don't allow")
Comment 41 Andreas Hedström Mace 2017-03-21 15:21:27 UTC
Thanks for testing Alex and Sonia!

And yes that is the same settings I was using. Since it works for both of you, I'm going to assume that either it is an issue with my devbox or I made a mistake while testing. Going to sign off.
Comment 42 Andreas Hedström Mace 2017-03-21 15:26:13 UTC
Created attachment 61401 [details] [review]
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one

Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Comment 43 Marcel de Rooy 2017-03-21 15:29:12 UTC
Ah great. I will be looking again at this one very soon.
Comment 44 Marcel de Rooy 2017-03-21 15:46:35 UTC
(In reply to Andreas Hedström Mace from comment #41)
> And yes that is the same settings I was using. Since it works for both of
> you, I'm going to assume that either it is an issue with my devbox or I made
> a mistake while testing. Going to sign off.
Andreas:
This is not the best introduction for a signoff btw :)
Comment 45 Marcel de Rooy 2017-03-21 16:01:09 UTC
Prelimary results (QA can be an iterative process):

perl t/db_dependent/Holds/CancelReserves.t
1..5
ok 1 - use C4::Reserves;
ok 2 - Reserve 1 should not be canceled.
ok 3 - Reserve 2 should be canceled.
Can't locate object method "get_instance" via package "Koha::Cache" at t/db_dependent/Holds/CancelReserves.t line 81.
# Looks like your test exited with 255 just after 3.

perl t/db_dependent/Holds/WaitingReserves.t
1..11
ok 1 - use C4::Reserves;
ok 2 - Waiting date should be set to today
ok 3 - Expiration date should be set to today + 6
ok 4 - Reserve status is now "waiting"
ok 5 - Priority should be 0
ok 6 - Item number should be set correctly
ok 7 - 2nd reserve - Reserve status is now "To transfer"
ok 8 - 2nd reserve - Priority should be 0
ok 9 - 2nd reserve - Item number should be set correctly
not ok 10 - Expiration date should be set to today + 7
#   Failed test 'Expiration date should be set to today + 7'
#   at t/db_dependent/Holds/WaitingReserves.t line 218.
#          got: '2017-03-27'
#     expected: '2017-03-29'
ok 11 - Requested expiration date should be kept
# Looks like you failed 1 test of 11.
Comment 46 Alex Arnaud 2017-03-22 12:56:21 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2017-03-22 13:34:48 UTC
Still something wrong?

1..11
ok 1 - use C4::Reserves;
Violation of unique constraint in Letter at /usr/share/koha/masterclone/t/lib/TestBuilder.pm line 202.
ok 2 - Waiting date should be set to today
ok 3 - Expiration date should be set to today + 6
ok 4 - Reserve status is now "waiting"
ok 5 - Priority should be 0
ok 6 - Item number should be set correctly
not ok 7 - 2nd reserve - Reserve status is now "To transfer"
#   Failed test '2nd reserve - Reserve status is now "To transfer"'
#   at t/db_dependent/Holds/WaitingReserves.t line 175.
#          got: 'W'
#     expected: 'T'
ok 8 - 2nd reserve - Priority should be 0
ok 9 - 2nd reserve - Item number should be set correctly
ok 10 - Expiration date should be set to today + 7
ok 11 - Requested expiration date should be kept
# Looks like you failed 1 test of 11.
Comment 48 Andreas Hedström Mace 2017-03-22 15:08:22 UTC
(In reply to Marcel de Rooy from comment #44)
> (In reply to Andreas Hedström Mace from comment #41)
> > And yes that is the same settings I was using. Since it works for both of
> > you, I'm going to assume that either it is an issue with my devbox or I made
> > a mistake while testing. Going to sign off.
> Andreas:
> This is not the best introduction for a signoff btw :)

Haha, yes, sorry about that. Just wanted to mentioned that I looked at this (cancelation fee) again and got it to work properly. It was an issue with date/time on debian, where it was not correct set.
Comment 49 Alex Arnaud 2017-03-22 15:17:43 UTC
Created attachment 61461 [details] [review]
Bug 12063 - Fix unit tests
Comment 50 Alex Arnaud 2017-03-22 15:18:59 UTC
It should be ok now (Sorry Marcel and ths for the review)
Comment 51 Marcel de Rooy 2017-03-23 13:32:46 UTC
QA: Looking here now
Comment 52 Marcel de Rooy 2017-03-23 14:45:22 UTC
Well, holds patches are not simple at least. Submitting a few comments collected so far.
Do not give up. We are improving on the way.
Comment 53 Marcel de Rooy 2017-03-23 14:45:44 UTC
Please look at this code in C4/Letters.pm: 
    if ( $table eq 'reserves' && $values->{'waitingdate'} ) {
        my @waitingdate = split /-/, $values->{'waitingdate'};

        $values->{'expirationdate'} = '';
        if ( C4::Context->preference('ReservesMaxPickUpDelay') ) {
            my $dt = dt_from_string();
            $dt->add( days => C4::Context->preference('ReservesMaxPickUpDelay') );
            $values->{'expirationdate'} = output_pref( { dt => $dt, dateonly => 1 } );
        }
This code should be adjusted too in line with the changes in $hold->set_waiting. Actually, if there is a waiting date, we must assume now that it is no longer needed to calculate an expiration date. In the above code the holiday was not respected either.
Comment 54 Marcel de Rooy 2017-03-23 14:45:58 UTC
Also please look here in circ/waitingreserves.pl
    ( $waiting_year, $waiting_month, $waiting_day ) =
      Add_Delta_Days( $waiting_year, $waiting_month, $waiting_day,
        $max_pickup_delay);
    my $calcDate = Date_to_Days( $waiting_year, $waiting_month, $waiting_day );
This date is compared with today and holds may be cancelled. We should use the new expirationdate here too. 
An interesting aspect is this one too in the same script: 
$max_pickup_delay-- if C4::Context->preference('ExpireReservesMaxPickUpDelay');
This is kind of strange btw. You should either use the delay or not use it.
Comment 55 Marcel de Rooy 2017-03-23 14:46:11 UTC
About the atomic update (db revision):
You add the pref, but we should also take into account all waiting reserves now and look at/calculate their expiration date.
Comment 56 Marcel de Rooy 2017-03-23 14:46:24 UTC
New sub days_foward:
Please add a unit test for this routine. In Calendar.t? Or Holidays.t?
Also note that the while loop is very dangerous if you would pass a negative number (infinite loop). Please check the value of the days.
Comment 57 Alex Arnaud 2017-03-23 16:08:50 UTC
Yup, good catch Marcel. Will fix that asap
Comment 58 Alex Arnaud 2017-03-30 09:57:36 UTC Comment hidden (obsolete)
Comment 59 Alex Arnaud 2017-03-30 10:05:06 UTC
Patch above fix Marcel's QA comments. Also, i added some minor changes:

in the sub Koha::Hold::set_waiting:
 => Remove the test "if ( C4::Context->preference("ExpireReservesMaxPickUpDelay") )".
I think we want to calculate the expiration date even if ExpireReservesMaxPickUpDelay is set to dont't allow

in C4::Reserves::CancelExpiredReserves:
return if ExpireReservesMaxPickUpDelay is set to don't allow. Do nothing

turn this BZ back to Signed off status. Tell me if the changes make i should pass it to Needs Signoff again.
Comment 60 Marcel de Rooy 2017-04-07 06:15:36 UTC
perl t/db_dependent/Holds/CancelReserves.t
1..5
ok 1 - use C4::Reserves;
ok 2 - Reserve 1 should not be canceled.
not ok 3 - Reserve 2 should be canceled.
#   Failed test 'Reserve 2 should be canceled.'
#   at t/db_dependent/Holds/CancelReserves.t line 66.
#          got: 'Koha::Hold=HASH(0x905fce8)'
#     expected: undef
ok 4 - Reserve 3 should not be canceled.
not ok 5 - Reserve 3 should be canceled.
#   Failed test 'Reserve 3 should be canceled.'
#   at t/db_dependent/Holds/CancelReserves.t line 101.
#          got: 'Koha::Hold=HASH(0x8f502b8)'
#     expected: undef
# Looks like you failed 2 tests of 5.

bug_12063-define_expirationdate_for_waitting_reserves.perl
Please adjust conform skeleton file in this same directory (DBVersion test, SetVersion and print)
Comment 61 Alex Arnaud 2017-04-10 13:40:59 UTC
Created attachment 62001 [details] [review]
Bug 12063: Change date calculation for reserve expiration to skip all holiday

This patch makes koha automatically set expiration date when reserves become
waitting. Also it adds a new syspref "ExcludeHolidaysFromMaxPickUpDelay" that allows to
take holidays into account while calculating expiration date.

Test plan:

  - Install this patch and run updatedatabase.pl script,
  - allow ExpireReservesMaxPickUpDelay in system preferences,
  - set ReservesMaxPickUpDelay to 5.

  - Place an hold on a checked out item and check in this item:
    The hold's expiration date should be today + 5.

  - Allow ExcludeHolidaysFromMaxPickUpDelay in system preferences,
  - add holiday during this pickup delay period,
  - Create a new hold and make it comes waitting:
    The hold's expiration date should be today + 5 + number of closed
    day(s).

Also:
  - Check that ExpireReservesOnHolidays syspref works again
    without ExcludeHolidaysFromMaxPickUpDelay.
  - Check that cancel fees apply again if wanted.

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 62 Alex Arnaud 2017-04-10 13:41:10 UTC
Created attachment 62002 [details] [review]
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 63 Alex Arnaud 2017-04-10 13:41:21 UTC
Created attachment 62003 [details] [review]
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one

Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>
Comment 64 Alex Arnaud 2017-04-10 13:41:31 UTC
Created attachment 62004 [details] [review]
Bug 12063 - Fix unit tests
Comment 65 Alex Arnaud 2017-04-10 13:41:41 UTC
Created attachment 62005 [details] [review]
Bug 12063 - Fix QA failures

- Remove expiration date calculation in C4::Letter since it's done
    when setting the reserve waiting,
  - remove expiration date calculation in circ/waitingreserves.pl. Use
    the one in DB,
  - add a new atomic update that calculate expiration date for
    waiting reserves,
  - add tests for days_foward function and fix the infinite loop.
Comment 66 Marcel de Rooy 2017-04-20 12:53:52 UTC
Could you please have another look at comment60 for the two atomic update files?
Comment 67 Alex Arnaud 2017-04-20 14:22:50 UTC
Created attachment 62474 [details] [review]
Bug 12063 - make perl atomic update follow skeleton.perl
Comment 68 Marcel de Rooy 2017-05-03 13:07:20 UTC
Still working on this one now.
Preliminary results:

 sub CancelExpiredReserves {
+    return unless C4::Context->preference("ExpireReservesMaxPickUpDelay");
This may have a unwanted side-effect. If we do not use the pickup delay, but still want to cancel expired reserves. It is no longer possible. (Note that patrons may have entered expiration dates too.)
Not marking this as a blocker, since this is probably exceptional. Do you have an easy fix or can you open a new report for it?

sub set_waiting {
[...]
+    if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) {
+        $expirationdate = $calendar->days_forward( dt_from_string($self->waitingdate), $max_pickup_delay );
+    }
This may cause something weird. If you do a second checkin while the waitingdate is say yesterday, the waitingdate is set to today but the expiration date will be based upon the old waitingdate. Again probably exceptional, but still something to fix. I would opt here for basing the expiry on today, since we reset the waitingdate too.
Comment 69 Alex Arnaud 2017-05-03 13:25:06 UTC
(In reply to Marcel de Rooy from comment #68)
> Still working on this one now.
> Preliminary results:
> 
>  sub CancelExpiredReserves {
> +    return unless C4::Context->preference("ExpireReservesMaxPickUpDelay");
> This may have a unwanted side-effect. If we do not use the pickup delay, but
> still want to cancel expired reserves. 

You mean: If ExpireReservesMaxPickUpDelay is set to don't allow ? If yes, this is because we don't want to cancel expired reserves right ? Or i don't understand the syspref.

> 
> sub set_waiting {
> [...]
> +    if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) {
> +        $expirationdate = $calendar->days_forward(
> dt_from_string($self->waitingdate), $max_pickup_delay );
> +    }
> This may cause something weird. If you do a second 

Is it possible that the code of set_waiting (after tranferToDo) should be called 2 times for the same reserve ?
Comment 70 Alex Arnaud 2017-05-03 13:28:28 UTC
(In reply to Alex Arnaud from comment #69)
> (In reply to Marcel de Rooy from comment #68)
 
> > sub set_waiting {
> > [...]
> > +    if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) {
> > +        $expirationdate = $calendar->days_forward(
> > dt_from_string($self->waitingdate), $max_pickup_delay );
> > +    }
> > This may cause something weird. If you do a second 
> 
> Is it possible that the code of set_waiting (after tranferToDo) should be
> called 2 times for the same reserve ?

Anyway it doesn't hurt to pass today to days_forward. So i'll fix it
Comment 71 Marcel de Rooy 2017-05-03 13:45:48 UTC
(In reply to Alex Arnaud from comment #69)
> (In reply to Marcel de Rooy from comment #68)
> >  sub CancelExpiredReserves {
> > +    return unless C4::Context->preference("ExpireReservesMaxPickUpDelay");
> > This may have a unwanted side-effect. If we do not use the pickup delay, but
> > still want to cancel expired reserves. 
> 
> You mean: If ExpireReservesMaxPickUpDelay is set to don't allow ? If yes,
> this is because we don't want to cancel expired reserves right ? Or i don't
> understand the syspref.
The situation I described is independent of this pref. You could use the cron job to remove expired reserves based on expiration dates entered by patrons.

> > sub set_waiting {
> > [...]
> > +    if ( C4::Context->preference("ExcludeHolidaysFromMaxPickUpDelay") ) {
> > +        $expirationdate = $calendar->days_forward(
> > dt_from_string($self->waitingdate), $max_pickup_delay );
> > +    }
> > This may cause something weird. If you do a second 
> 
> Is it possible that the code of set_waiting (after tranferToDo) should be
> called 2 times for the same reserve ?
As mentioned, this is exceptional. But it can be easily simulated. Check this book in today and perhaps by accident tomorrow (it was still on the desk or something like that). Two separate checkins, two calls.
Comment 72 Alex Arnaud 2017-05-03 14:34:39 UTC
(In reply to Marcel de Rooy from comment #71)
> The situation I described is independent of this pref. You could use the
> cron job to remove expired reserves based on expiration dates entered by
> patrons.
> 

Difficult to decide the issue. The behavior of sub CancelExpiredReserves was before:
  - Cancel holds with an expiration date (asked by the patron afaik),
  - Cancel other exipred holds only if ExpireReservesMaxPickUpDelay is set to allow

So now we have all waiting reerves with an expiration date, we can't make the difference between those with expiration ask by the patron and those with expiration calculated.
Comment 73 Alex Arnaud 2017-05-03 14:38:05 UTC
And note that CancelExpiredReserves is only caled by the script misc/cronjobs/holds/cancel_expired_holds.pl. Nowhere else if i'm not wrong. So if we remove the check of ExpireReservesMaxPickUpDelay from this sub, we get a kind of "force cancel reserves".
Comment 74 Marcel de Rooy 2017-05-04 07:13:18 UTC
(In reply to Alex Arnaud from comment #72)
> (In reply to Marcel de Rooy from comment #71)
> > The situation I described is independent of this pref. You could use the
> > cron job to remove expired reserves based on expiration dates entered by
> > patrons.
> > 
> 
> Difficult to decide the issue. The behavior of sub CancelExpiredReserves was
> before:
>   - Cancel holds with an expiration date (asked by the patron afaik),
>   - Cancel other exipred holds only if ExpireReservesMaxPickUpDelay is set
> to allow
> 
> So now we have all waiting reerves with an expiration date, we can't make
> the difference between those with expiration ask by the patron and those
> with expiration calculated.

True, but not in all cases. If you disabled ExpireReservesMaxPickUpDelay, all expired reserves must be 'patron-originated'.
But the question is: do we still need to make a distinction? The reserve has expired for some reason, we can cancel it. We can just remove the line imo.
Comment 75 Marcel de Rooy 2017-05-04 07:15:45 UTC
(In reply to Alex Arnaud from comment #73)
> And note that CancelExpiredReserves is only caled by the script
> misc/cronjobs/holds/cancel_expired_holds.pl. Nowhere else if i'm not wrong.
> So if we remove the check of ExpireReservesMaxPickUpDelay from this sub, we
> get a kind of "force cancel reserves".

I do not understand. CancelExpiredReserves should just do what it says. Note that this patch would change behavior otherwise.
Comment 76 Alex Arnaud 2017-05-04 08:56:49 UTC
Created attachment 63057 [details] [review]
Bug 12063 - Remove checking of ExpireReservesMaxPickUpDelay in CancelExpiredReserves(). Koha::Hold::set_waiting calculate expiration date from today instead of hold's waiting date.
Comment 77 Marcel de Rooy 2017-05-04 12:22:52 UTC
Created attachment 63091 [details] [review]
Bug 12063: Change date calculation for reserve expiration to skip all holiday

This patch makes koha automatically set expiration date when reserves become
waitting. Also it adds a new syspref "ExcludeHolidaysFromMaxPickUpDelay" that allows to
take holidays into account while calculating expiration date.

Test plan:

  - Install this patch and run updatedatabase.pl script,
  - allow ExpireReservesMaxPickUpDelay in system preferences,
  - set ReservesMaxPickUpDelay to 5.

  - Place an hold on a checked out item and check in this item:
    The hold's expiration date should be today + 5.

  - Allow ExcludeHolidaysFromMaxPickUpDelay in system preferences,
  - add holiday during this pickup delay period,
  - Create a new hold and make it comes waitting:
    The hold's expiration date should be today + 5 + number of closed
    day(s).

Also:
  - Check that ExpireReservesOnHolidays syspref works again
    without ExcludeHolidaysFromMaxPickUpDelay.
  - Check that cancel fees apply again if wanted.

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 78 Marcel de Rooy 2017-05-04 12:22:58 UTC
Created attachment 63092 [details] [review]
Bug 12063 - Remove Koha::Hold::waiting_expires_on since dateexpiration is set on database

Signed-off-by: sonia BOUIS <sonia.bouis@univ-lyon3.fr>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 79 Marcel de Rooy 2017-05-04 12:23:04 UTC
Created attachment 63093 [details] [review]
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one

Signed-off-by: Andreas Hedström Mace <andreas.hedstrom.mace@sub.su.se>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 80 Marcel de Rooy 2017-05-04 12:23:09 UTC
Created attachment 63094 [details] [review]
Bug 12063 - Fix unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 81 Marcel de Rooy 2017-05-04 12:23:15 UTC
Created attachment 63095 [details] [review]
Bug 12063 - Fix QA failures

- Remove expiration date calculation in C4::Letter since it's done
    when setting the reserve waiting,
  - remove expiration date calculation in circ/waitingreserves.pl. Use
    the one in DB,
  - add a new atomic update that calculate expiration date for
    waiting reserves,
  - add tests for days_foward function and fix the infinite loop.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 82 Marcel de Rooy 2017-05-04 12:23:21 UTC
Created attachment 63096 [details] [review]
Bug 12063 - make perl atomic update follow skeleton.perl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 83 Marcel de Rooy 2017-05-04 12:23:26 UTC
Created attachment 63097 [details] [review]
Bug 12063 - Remove checking of ExpireReservesMaxPickUpDelay in CancelExpiredReserves(). Koha::Hold::set_waiting calculate expiration date from today instead of hold's waiting date.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 84 Marcel de Rooy 2017-05-04 12:23:32 UTC
Created attachment 63098 [details] [review]
Bug 12063: [QA Follow-up] Adjustments to db revision

Replace use by require.
Add require Koha::Calendar (seriously needed to prevent crashing).
Base expirationdate on today not on waitingdate (just as in Reserves).
Remove if ( waitingdate ) construction. Not needed anymore.
Remove $cancel_on_holidays. Not used. Note that the if construction around
the new pref is not strictly needed at upgrade time (pref still zero).
Fill the dbrev description.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 85 Marcel de Rooy 2017-05-04 12:23:38 UTC
Created attachment 63099 [details] [review]
Bug 12063: [QA Follow-up] Small change of two test scripts

Removing dbh from one script, changing rollback in the other.
Schema is leading now.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 86 Marcel de Rooy 2017-05-04 12:30:31 UTC
Final QA Comment:
After a longer process, we reached Passed QA now. Added two small follow-ups. One serious problem in the db rev fixed.

Still wanting to mention two points without triggering discussion again:
[1] The construction around expirationdate still feels little bit unappropriate to me personally, but after some discussion we found a compromize.
[2] The change in Letters.pm is still arguable. Previously, expirationdate was calculated here and could be used on a notice. Now, we do no longer need to calculate it theoretically. But take e.g. the hold slip; the notice is generated before the expiration date is set and therefore still empty.
I recommend to have a look at this problem on a new report. I do not consider it as a blocker.

Thanks, Alex.
Comment 87 Kyle M Hall 2017-05-09 13:17:23 UTC
Pushed to master for 17.05, thanks Alex, Marcel!
Comment 88 Katrin Fischer 2017-05-14 09:31:27 UTC
This won't get ported back to 16.11.x as it is an enhancement.
Comment 89 Katrin Fischer 2017-05-14 09:32:12 UTC
I think it would be good to have the "Text to go in the release notes" filled for this one, describing the change to the libraries updating.
Comment 90 Katrin Fischer 2017-05-14 09:40:10 UTC
Also, what changes for libraries not using the cronjob to automatically expire holds? 

We don't use this because libraries often want to leave the book on a few days longer during holidays etc. and because the slip for the next patron is not printed automatically and there is no clue for the library that they have to print a new one.