Bug 24612 - expirationdate blank if patron has more than one item from bib on hold
Summary: expirationdate blank if patron has more than one item from bib on hold
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-07 19:04 UTC by Christopher Brannon
Modified: 2020-06-12 19:07 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.07


Attachments
Bug 24612: Make hold-transfer-slip take reserve_id (9.04 KB, patch)
2020-02-12 09:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Make hold-transfer-slip take reserve_id (10.06 KB, patch)
2020-02-12 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Make hold-transfer-slip take reserve_id (10.05 KB, patch)
2020-04-13 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Add tests (3.92 KB, patch)
2020-04-13 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Use the reserve_id to identify a reserve when building a notice (924 bytes, patch)
2020-04-13 11:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Make hold-transfer-slip take reserve_id (11.25 KB, patch)
2020-05-13 04:14 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24612: Add tests (3.97 KB, patch)
2020-05-13 04:14 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24612: Use the reserve_id to identify a reserve when building a notice (983 bytes, patch)
2020-05-13 04:15 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 24612: Make hold-transfer-slip take reserve_id (11.34 KB, patch)
2020-05-21 14:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24612: Add tests (4.03 KB, patch)
2020-05-21 14:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24612: Use the reserve_id to identify a reserve when building a notice (1.02 KB, patch)
2020-05-21 14:17 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 24612: Fix existing ReserveSlip tests (2.75 KB, patch)
2020-05-22 09:20 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Fix existing ReserveSlip tests (4.28 KB, patch)
2020-05-22 09:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24612: Fix existing ReserveSlip tests (4.40 KB, patch)
2020-06-11 21:45 UTC, Aleisha Amohia
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2020-02-07 19:04:22 UTC
It is impossible to print the expiration date on a hold slip (HOLD_SLIP) if the patron has more than one hold on that bib.  The expirationdate comes back blank.
Comment 1 Christopher Brannon 2020-02-07 19:15:56 UTC
I have confirmed by looking at [% hold.reserve_id %] that HOLD_SLIP is looking at the wrong hold record.
Comment 2 Christopher Brannon 2020-02-07 19:29:37 UTC
If I assign <<reserves.expirationdate>> to another variable, for some reason, [% hold.expirationdate %] works.  Very wonky behavior.
Comment 3 Jonathan Druart 2020-02-12 09:10:57 UTC
Created attachment 98738 [details] [review]
Bug 24612: Make hold-transfer-slip take reserve_id

To make sure we are going to display the correct hold's info we need to
pass the reserve_id.
Comment 4 Jonathan Druart 2020-02-12 09:11:42 UTC
I think this patch will fix the issue you have. Can you confirm?
hold skip needs to be tested from return.pl (checkin) and circulation.pl (checkout)
Comment 5 Jonathan Druart 2020-02-12 09:22:22 UTC
Created attachment 98739 [details] [review]
Bug 24612: Make hold-transfer-slip take reserve_id

To make sure we are going to display the correct hold's info we need to
pass the reserve_id.
Comment 6 Andrew Fuerste-Henry 2020-02-14 18:11:04 UTC
Christopher, I'm not able to replicate this in Master. I tried to confirm the behavior before applying the patch and haven't gotten the expiration date to come up null. Can you please confirm the problem still exists and/or provide steps to replicate it?
Comment 7 Lisette Scheer 2020-02-18 19:45:51 UTC
(In reply to Andrew Fuerste-Henry from comment #6)
> Christopher, I'm not able to replicate this in Master. I tried to confirm
> the behavior before applying the patch and haven't gotten the expiration
> date to come up null. Can you please confirm the problem still exists and/or
> provide steps to replicate it?

I was able to replicate is this way:

1. In notices and slips, add [% hold.reserve_id %] to your hold slip.
2. Place 2 bib level holds for the same patron on a bib. 
3. Check in one of the items and click print slip. 
4. Observe that the slip doesn't have an expiration date at all. 
5. Cancel the waiting hold. 
6. Check in the other item and click print slip. 
7. Observe the expiration date on the slip.
Comment 8 Sally 2020-03-24 17:02:41 UTC
I've tested and I can't get the patch to solve the problem.

This is what I did:  

TEST PLAN:

1.  Add some content to HOLD_SLIP notice, e.g.

<h2>[% branch.branchname %]</h2>
<div>[% biblio.author %]<br>[% biblio.title %]<br>[% item.barcode %]
<ul><li> Reserve ID:  [% hold.reserve_id %]</li>
<li>Expiration date: [% hold.expirationdate %]</li></ul>

2.  Add 2 holds for 1 patron to a single record
3.  Check the reserve IDs in the reserves table - on a clean sandbox, they will be 1 and 2 
4.  Check in one of the items from the record and print the slip
5.  Note that the reserve ID on the slip is 2 and the expiration date is blank
6.  Repeated check ins do not change this
7.  Check in a second item from the record
8.  Note that the reserve ID for this hold is also 2, but this time the expiration date is filled in
9.  Check in the first item again - the reserve ID stays as 2, but this time the expiration date is filled in

I repeated this several times, and the odd numbered holds (1, 3, 5, 7 etc) never appear - but the even ones appear for all items (2, 4, 6, 8).  

I set the bug as Failed QA because it doesn't seem to solve the problem - is that the correct status?  Or should I have chosen Patch Doesn't Apply?
Comment 9 Jonathan Druart 2020-04-13 11:51:37 UTC
Created attachment 102818 [details] [review]
Bug 24612: Make hold-transfer-slip take reserve_id

To make sure we are going to display the correct hold's info we need to
pass the reserve_id.

See comment 7 and 8 for test plans
Comment 10 Jonathan Druart 2020-04-13 11:51:40 UTC
Created attachment 102819 [details] [review]
Bug 24612: Add tests

ReserveSlip call need to be adjusted in test.
We also add a new reserve (same biblio, same patron) to highlight the problem.
Comment 11 Jonathan Druart 2020-04-13 11:51:44 UTC
Created attachment 102820 [details] [review]
Bug 24612: Use the reserve_id to identify a reserve when building a notice

Now that we have the reserve_id PK on the reserves table we should use
it (instead of the couple borrowernumber, biblionumber)
Comment 12 Jonathan Druart 2020-04-13 11:52:54 UTC
(In reply to Sally from comment #8)
> I've tested and I can't get the patch to solve the problem.

I think it's now fixed with the last patchset!

> I set the bug as Failed QA because it doesn't seem to solve the problem - is
> that the correct status?  Or should I have chosen Patch Doesn't Apply?

"Failed QA" is the correct status when the patches do no work as you expected.
Comment 13 Victor Grousset/tuxayo 2020-05-13 04:14:55 UTC
Created attachment 104803 [details] [review]
Bug 24612: Make hold-transfer-slip take reserve_id

To make sure we are going to display the correct hold's info we need to
pass the reserve_id.

== Test plan ==
1. Add some content to HOLD_SLIP notice, e.g.

  <h2>[% branch.branchname %]</h2>
  <div>[% biblio.author %]<br>[% biblio.title %]<br>[% item.barcode %]
  <ul><li> Reserve ID:  [% hold.reserve_id %]</li>
  <li>Expiration date: [% hold.expirationdate %]</li></ul>

2. Add 2 holds for 1 patron to a single record
3. Check the reserve IDs in the reserves table - on a clean sandbox, they will be 1 and 2
4. Check in one of the items from the record and print the slip
5. Note that the reserve ID on the slip is 2 and the expiration date is blank
6. Repeated check ins do not change this
7. Check in a second item from the record
8. Note that the reserve ID for this hold is also 2, but this time the expiration date is filled in
9. Check in the first item again - the reserve ID stays as 2, but this time the expiration date is filled in
10. Apply patch
11. cancel the holds to come back to a clean state
    (and maybe ensure items aren't in transit)
12. redo the test and see the following differences
13. 1st checkin:
    1. expiration date ok
    2. the reserve ID is the one of the first hold
14. 2nd checkin:
    1. expiration date ok
    2. the reserve ID is the one of the second hold

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 14 Victor Grousset/tuxayo 2020-05-13 04:14:59 UTC
Created attachment 104804 [details] [review]
Bug 24612: Add tests

ReserveSlip call need to be adjusted in test.
We also add a new reserve (same biblio, same patron) to highlight the problem.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 15 Victor Grousset/tuxayo 2020-05-13 04:15:02 UTC
Created attachment 104805 [details] [review]
Bug 24612: Use the reserve_id to identify a reserve when building a notice

Now that we have the reserve_id PK on the reserves table we should use
it (instead of the couple borrowernumber, biblionumber)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 16 Victor Grousset/tuxayo 2020-05-13 04:15:18 UTC
It works! (an so the automated tests) thanks Jonathan. :D

Thanks Lisette and Sally for the test plan.
Comment 17 Katrin Fischer 2020-05-21 14:17:43 UTC
Created attachment 105202 [details] [review]
Bug 24612: Make hold-transfer-slip take reserve_id

To make sure we are going to display the correct hold's info we need to
pass the reserve_id.

== Test plan ==
1. Add some content to HOLD_SLIP notice, e.g.

  <h2>[% branch.branchname %]</h2>
  <div>[% biblio.author %]<br>[% biblio.title %]<br>[% item.barcode %]
  <ul><li> Reserve ID:  [% hold.reserve_id %]</li>
  <li>Expiration date: [% hold.expirationdate %]</li></ul>

2. Add 2 holds for 1 patron to a single record
3. Check the reserve IDs in the reserves table - on a clean sandbox, they will be 1 and 2
4. Check in one of the items from the record and print the slip
5. Note that the reserve ID on the slip is 2 and the expiration date is blank
6. Repeated check ins do not change this
7. Check in a second item from the record
8. Note that the reserve ID for this hold is also 2, but this time the expiration date is filled in
9. Check in the first item again - the reserve ID stays as 2, but this time the expiration date is filled in
10. Apply patch
11. cancel the holds to come back to a clean state
    (and maybe ensure items aren't in transit)
12. redo the test and see the following differences
13. 1st checkin:
    1. expiration date ok
    2. the reserve ID is the one of the first hold
14. 2nd checkin:
    1. expiration date ok
    2. the reserve ID is the one of the second hold

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2020-05-21 14:17:47 UTC
Created attachment 105203 [details] [review]
Bug 24612: Add tests

ReserveSlip call need to be adjusted in test.
We also add a new reserve (same biblio, same patron) to highlight the problem.

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2020-05-21 14:17:52 UTC
Created attachment 105204 [details] [review]
Bug 24612: Use the reserve_id to identify a reserve when building a notice

Now that we have the reserve_id PK on the reserves table we should use
it (instead of the couple borrowernumber, biblionumber)

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Martin Renvoize 2020-05-22 08:33:50 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 21 Jonathan Druart 2020-05-22 09:20:41 UTC
Created attachment 105253 [details] [review]
Bug 24612: Fix existing ReserveSlip tests
Comment 22 Jonathan Druart 2020-05-22 09:27:40 UTC
Created attachment 105254 [details] [review]
Bug 24612: Fix existing ReserveSlip tests
Comment 23 Aleisha Amohia 2020-06-11 21:45:59 UTC
Created attachment 105775 [details] [review]
Bug 24612: Fix existing ReserveSlip tests
Comment 24 Aleisha Amohia 2020-06-11 22:37:20 UTC
backported to 19.11.x for 19.11.07
Comment 25 Victor Grousset/tuxayo 2020-06-12 19:07:26 UTC
Patch a bit too large to warrant a backport to oldoldstable (19.05.x) considering it's severity is only of level "normal"
And there is a non-trivial conflict when cherry-picking.