Bug 12204

Summary: in RESERVESLIP, any 'reserve' field is not being expanded correctly.
Product: Koha Reporter: Barton Chittenden <bartonski>
Component: NoticesAssignee: Magnus Enger <magnus>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: minor    
Priority: P5 - low CC: bgkriegel, jdemuth, jonathan.druart, josef.moravec, katrin.fischer, koha, kyle, magnus, marjorie.barry-vila, mtompset, patrick.robitaille
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 12204: Display the expiration date on RESERVESLIP
Bug 12204: Display the expiration date on RESERVESLIP
Bug 12204: Display the expiration date on RESERVESLIP

Description Barton Chittenden 2014-05-06 19:54:42 UTC
Any field containing 'reserves' is not expanded in the RESERVESLIP notice:

For example:

    <b>Hold until: </b> <<reserves.expirationdate>><br />

Renders as

    <b>Hold until: </b> <br />

And

    <p><b>Notes:</b>
    <pre><<reserves.reservenotes>></pre>

becomes

    <p><b>Notes:</b>
    <pre></pre>

I believe that this is a regression upon upgrade from 3.12.10 to 3.14.05.
Comment 1 Jonathan Druart 2015-04-28 14:13:09 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-04-28 14:14:12 UTC
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.
Comment 3 Mark Tompsett 2015-05-01 15:53:02 UTC
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.
Comment 4 Bernardo Gonzalez Kriegel 2015-05-06 19:57:10 UTC
(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?
Comment 5 Jonathan Druart 2015-09-01 10:27:12 UTC
(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.
Comment 6 Koha Team University Lyon 3 2015-12-15 16:31:37 UTC
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
Comment 7 Jonathan Druart 2015-12-16 09:10:36 UTC Comment hidden (obsolete)
Comment 8 Chris Cormack 2015-12-18 00:56:34 UTC
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>
Comment 9 Kyle M Hall 2015-12-18 18:21:55 UTC
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.
Comment 10 Katrin Fischer 2015-12-21 11:14:46 UTC
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.
Comment 11 Magnus Enger 2017-08-21 07:56:39 UTC
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.
Comment 12 Katrin Fischer 2017-08-27 10:30:28 UTC
Magnus, I think you are right.
Comment 13 Jonathan Druart 2017-09-01 13:51:49 UTC
(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.
Comment 14 Magnus Enger 2017-09-04 11:15:51 UTC
Looks like bug 19025 pulled the rug out from underneath this patch between my comment and that from Jonathan. :-)
Comment 15 Magnus Enger 2017-09-04 11:17:10 UTC
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.
Comment 16 Magnus Enger 2017-09-04 11:50:05 UTC
On further inspection it looks like this is completely fixed by bug 19025.