Bug 12204

Hello Barton,
The expirationdate field was not retrieved from the DB before the slip generation, my patch fixes that.
But the other fields should be displayed, I have just tested with reservenotes and it displayed correctly.

Is it possible to add a reserve/hold with a particular expiration date, but then before that expiration date, change a system preference to indicate that it should already be expired? If so, this patch is insufficient. If not, then it should be fine.

(In reply to Jonathan Druart from comment #1)
> Test plan:
> 1/ Define RESERVESLIP with, at least,
>     Hold until: <<reserves.expirationdate>>

On current master RESERVESLIP belongs to 'circulation' module,
on edit there is no access to 'reserves' values.

If you change first to 'Holds' module, and save, 
then you can access 'reserves' values.

Could be this the cause of the problem?

(In reply to Bernardo Gonzalez Kriegel from comment #4)
> (In reply to Jonathan Druart from comment #1)
> > Test plan:
> > 1/ Define RESERVESLIP with, at least,
> >     Hold until: <<reserves.expirationdate>>
> 
> On current master RESERVESLIP belongs to 'circulation' module,
> on edit there is no access to 'reserves' values.
> 
> If you change first to 'Holds' module, and save, 
> then you can access 'reserves' values.
> 
> Could be this the cause of the problem?

No, the value is not retrieved from the DB.

tried on a sandbox :
<h1>Something went wrong !</h1>Applying: Bug 12204: Display the expiration date on RESERVESLIP Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Reserves.pm
Auto-merging C4/Letters.pm
CONFLICT (content): Merge conflict in C4/Letters.pm Failed to merge in the changes.
Patch failed at 0001 Bug 12204: Display the expiration date on RESERVESLIP

Created attachment 45800 [details] [review]
Bug 12204: Display the expiration date on RESERVESLIP

If an expiration date is set on placing an item on hold, the date is not
displayed on the reserve slip.
It's only generated/displayed if the prefs ExpireReservesMaxPickUpDelay
and ReservesMaxPickUpDelay are set.

Test plan:
1/ Define RESERVESLIP with, at least,
    Hold until: <<reserves.expirationdate>>
2/ Place an item on hold and define an expiration date
3/ Check the item in and print the slip
4/ Note that the expiration date is correctly displayed

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

This patch causes a regression. The expiration date for a waiting hold should be calculated using waitingdate + ReservesMaxPickUpDelay. With this patch if a waiting hold had a fulfillment expiration date, that date will be displayed and no the waiting expiration date.

Furthermore, I have been unable to reproduce the issue on master.

Looking a bit deeper into this, I think there is a confusion about the data in the reserves table:

reserves.expirationdate is NOT the date when a hold expires for pick-up. It's the date the user set for not needing the hold any longer when placing it. It should probably be used to automatically cancel holds.
I think this one should always display as it is in the database and the old feature was built on a wrong assumption about the existing column.

What we would need additionally is a 'pickup-expirationdate'. We don't have a column for this in the reserves table currently. So we can either introduce a new column or calculate it to be put in the notices on the fly. 

And only this date should then take the ReservesMaxPickUpDelay into account and be calculated like Kyle says: waitingdate + ReservesMaxPickUpDelay. It shoudl also probably take the calendar into account in some way, maybe by using methods like the useDaysMode preference.

There have been a lot of long discussions, but as far as I can tell, these bugs:

Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14937 minor, P5 - low, ---, kyle.m.hall, CLOSED FIXED, Expiration date for holds based onReservesMaxPickUpDelay should not require ExpireReservesMaxPickUpDelay

Bug http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12063 enhancement, P5 - low, ---, alex.arnaud, RESOLVED FIXED, Change date calculation for reserve expiration to skip all holidays

have changed things enough that it now makes sense to revive the current bug/patch. I have made a similar change on my live server and customers are happy with it. 

I am setting the status to "Signed off". Please set it to "Needs signoff" you think that is necessary.

Magnus, I think you are right.

(In reply to Magnus Enger from comment #11)
> have changed things enough that it now makes sense to revive the current
> bug/patch. I have made a similar change on my live server and customers are
> happy with it. 

What is the patch? This one does no longer apply.

Looks like bug 19025 pulled the rug out from underneath this patch between my comment and that from Jonathan. :-)

Comment on attachment 45800 [details] [review]
Bug 12204: Display the expiration date on RESERVESLIP

Bug 19025 makes this patch obsolete. But the problem described in the bug report remains, as far as I can tell.

On further inspection it looks like this is completely fixed by bug 19025.