Adding a see also to bug 8735 as both seem to be related.

(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.

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

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.

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...

Patch tested with a sandbox, by sonia BOUIS <sonia.bouis@univ-lyon3.fr>

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 :)

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)

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.

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.

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

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

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.

(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?

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.

(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.

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.

(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 ?

(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.

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?

(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.

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 ?

(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

(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 :)

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]

Brilliant! I hope this is doable, and we are also able to test and (possibly) sign off!

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>

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>

I would like to QA this patch set. Setting QA contact

Created attachment 59165 [details] [review]
Bug 12063 - Keep patron's requested expiration date if it is prior to the calculated one

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.

(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.

(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

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")

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.

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>

Ah great. I will be looking again at this one very soon.

(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 :)

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.

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.

(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.

Created attachment 61461 [details] [review]
Bug 12063 - Fix unit tests

It should be ok now (Sorry Marcel and ths for the review)

QA: Looking here now

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.

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.

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.

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.

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.

Yup, good catch Marcel. Will fix that asap

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.

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)

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>

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>

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>

Created attachment 62004 [details] [review]
Bug 12063 - Fix unit tests

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.

Could you please have another look at comment60 for the two atomic update files?

Created attachment 62474 [details] [review]
Bug 12063 - make perl atomic update follow skeleton.perl

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.

(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 ?

(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

(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.

(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.

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".

(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.

(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.

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.

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>

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>

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>

Created attachment 63094 [details] [review]
Bug 12063 - Fix unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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>

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>

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>

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>

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>

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.

Pushed to master for 17.05, thanks Alex, Marcel!

This won't get ported back to 16.11.x as it is an enhancement.

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.

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.

*** Bug 12353 has been marked as a duplicate of this bug. ***

*** Bug 14307 has been marked as a duplicate of this bug. ***