Bug 33364 - Stop Holds to Pull report showing items that have been allocated to a recall
Summary: Stop Holds to Pull report showing items that have been allocated to a recall
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-29 21:18 UTC by Aleisha Amohia
Modified: 2024-01-09 05:42 UTC (History)
3 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch changes the check for pending recalls to include items in transit, as well as items awaiting pickup, to fill a recall request. It then ensures the Holds to Pull report excludes items that have pending recalls.
Version(s) released in:


Attachments
Bug 33364: Prevent Holds to Pull from including items with pending recalls (4.03 KB, patch)
2023-04-17 02:06 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33364: [22.05] Prevent Holds to Pull from including items with pending recalls (2.67 KB, patch)
2023-04-17 02:46 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33364: [22.05] Prevent Holds to Pull from including items with pending recalls (2.67 KB, patch)
2023-04-17 21:17 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 33364: [22.05] Prevent Holds to Pull from including items with pending recalls (2.71 KB, patch)
2023-09-17 22:56 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2023-03-29 21:18:22 UTC
The Holds to Pull report shows an item with a hold if the item is available on the shelf, ready to pull and allocate to the hold.

The Holds to Pull should NOT show an item with a hold if that item has already been allocated to a recall (i.e. the recalled item is awaiting pickup and or in transit for a patron).

To reproduce:

1. Enable UseRecalls and set the recalls-related circulation rules
2. Check out an item to Patron A
3. Place a hold on the item for Patron B
4. Log into the OPAC as Patron C and search for the item. Place a recall on the item.
5. Go back to the staff interface. Go to Circulation -> Holds to pull
6. Confirm the item shows in the Holds to pull table, with the hold for Patron B
7. Check in the item and confirm the recall as waiting
8. Go back to the Holds to pull page. You could manually regenerate the page but running the cronjob in the terminal misc/cronjobs/holds/build_holds_queue.pl
9. Notice that even though the item is allocated for a recall, the hold is showing. It should not be showing in this page.
Comment 1 Aleisha Amohia 2023-04-17 02:06:23 UTC
Created attachment 149741 [details] [review]
Bug 33364: Prevent Holds to Pull from including items with pending recalls

This patch changes the check for pending recalls to include items in transit, as well as items awaiting pickup, to fill a recall request.

It then ensures the Holds to Pull report excludes items that have pending recalls.

To test:

1. Enable UseRecalls and set the recalls-related circulation rules
2. Check out an item to Patron A
3. Place a hold on the item for Patron B
4. Log into the OPAC as Patron C and search for the item. Place a recall on the item.
5. Go back to the staff interface. Go to Circulation -> Holds to pull
6. Confirm the item shows in the Holds to pull table, with the hold for Patron B
7. Check in the item and confirm the recall as waiting
8. Go back to the Holds to pull page. You may need to manually regenerate the page by running the cronjob in the terminal misc/cronjobs/holds/build_holds_queue.pl
9. Notice that even though the item is allocated for a recall, the hold is showing. It should not be showing in this page.

10. Apply the patch and refresh the page

11. The item allocated to a recall should no longer be showing in the Holds to pull report

12. Go to the record detail page and go to the Recalls tab
13. Revert the recall from its waiting status
14. The record will now show on the Holds to pull page
15. Go to the Circulation -> Check in page
16. Go to the menu in the top-right of the page and click Set library
17. Change your library to a different one
18. Check in the item to confirm the recall as waiting and trigger a transfer
19. Go back to the Holds to pull page, regenerate if needed
20. The item should no longer show on the Holds to pull report

21. Confirm tests pass t/db_dependent/Koha/Item.t

Sponsored-by: Auckland University of Technology
Comment 2 Aleisha Amohia 2023-04-17 02:46:35 UTC Comment hidden (obsolete)
Comment 3 Aleisha Amohia 2023-04-17 21:17:59 UTC Comment hidden (obsolete)
Comment 4 Aleisha Amohia 2023-07-10 04:57:55 UTC
I'm not sure why this was set to Patch doesn't apply - the first patch applies cleanly on upstream and 22.11, and the second patch applies cleanly on 22.05.

This cannot be tested on a sandbox.

Please make sure you do not apply the second patch if testing on 22.11 and higher, and only apply the second patch if testing on 22.05.
Comment 5 Katrin Fischer 2023-07-16 11:16:58 UTC
Hi Sam, if you want to try again: 

git bz apply ... 
Apply? [(y)es, (n)o, (i)nteractive] i

Then comment out the second patch by removing the # so it won't be applied.

The other option that works here:
git bz apply ...
Apply? [(y)es, (n)o, (i)nteractive] i
git bz apply --skip

This will also keep the second patch from being applied.
Comment 6 David Nind 2023-09-17 22:56:11 UTC
Created attachment 155767 [details] [review]
Bug 33364: [22.05] Prevent Holds to Pull from including items with pending recalls

This patch changes the check for pending recalls to include items in transit, as well as items awaiting pickup, to fill a recall request.

It then ensures the Holds to Pull report excludes items that have pending recalls.

To test:

1. Enable UseRecalls and set the recalls-related circulation rules
2. Check out an item to Patron A
3. Place a hold on the item for Patron B
4. Log into the OPAC as Patron C and search for the item. Place a recall on the item.
5. Go back to the staff interface. Go to Circulation -> Holds to pull
6. Confirm the item shows in the Holds to pull table, with the hold for Patron B
7. Check in the item and confirm the recall as waiting
8. Go back to the Holds to pull page. You may need to manually regenerate the page by running the cronjob in the terminal misc/cronjobs/holds/build_holds_queue.pl
9. Notice that even though the item is allocated for a recall, the hold is showing. It should not be showing in this page.

10. Apply the patch and refresh the page

11. The item allocated to a recall should no longer be showing in the Holds to pull report

12. Go to the record detail page and go to the Recalls tab
13. Revert the recall from its waiting status
14. The record will now show on the Holds to pull page
15. Go to the Circulation -> Check in page
16. Go to the menu in the top-right of the page and click Set library
17. Change your library to a different one
18. Check in the item to confirm the recall as waiting and trigger a transfer
19. Go back to the Holds to pull page, regenerate if needed
20. The item should no longer show on the Holds to pull report

21. Confirm tests pass t/db_dependent/Koha/Item.t

Sponsored-by: Auckland University of Technology

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2023-09-17 23:00:49 UTC
The 22.05 patch works as per the test plan, and I've signed it off.

I haven't signed off the patch for master, as it doesn't seem to work for me:
- Step 11 - No change noted after applying the patch, restart_all, refreshing browser, etc. It is still showing on the holds to pull list for Patron B (step 2).

Tested using koha-testing-docker (KTD). (For 22.05 - checked out 22.05 branch, for KTD changed .env so that KOHA_IMAGE=22.05, then ktd pull. Accessed KTD using ktd --root --shell)
Comment 8 Katrin Fischer 2023-09-18 19:54:48 UTC


Hi Aleisha, can you please check David's comment on the patch for master?

In reply to David Nind from comment #7)
> The 22.05 patch works as per the test plan, and I've signed it off.
> 
> I haven't signed off the patch for master, as it doesn't seem to work for me:
> - Step 11 - No change noted after applying the patch, restart_all,
> refreshing browser, etc. It is still showing on the holds to pull list for
> Patron B (step 2).
> 
> Tested using koha-testing-docker (KTD). (For 22.05 - checked out 22.05
> branch, for KTD changed .env so that KOHA_IMAGE=22.05, then ktd pull.
> Accessed KTD using ktd --root --shell)
Comment 9 Aleisha Amohia 2023-10-09 08:14:16 UTC
The patch applies and works for me.

You may find it easier to test using a record with only 1 item.
Comment 10 David Nind 2023-10-09 21:01:12 UTC
(In reply to Aleisha Amohia from comment #9)
> The patch applies and works for me.
> 
> You may find it easier to test using a record with only 1 item.

I had another go at testing, but I'm not sure I'm getting it right. Will leave for someone else to test (or I'll try it fresh tomorrow!).

1. If I use a record with one item, for step 6 there is nothing in the holds to pull queue (even after running the cronjob).

2. If I use a record with two or more items (for simplicity, items are all in the same library), it appears on the holds to pull list both before and after the patch is applied - this makes sense to me, as there is more than one item available - both for a hold and the recall. (steps 9 and 11)

3. My recall rules:
   - Recalls allowed (total): 5
   - Recalls per record (count): 5
   - On shelf recalls allowed: If any unavailable
   - Recall due date interval (day): 5
   - Recall overdue fine amount: 5.00
   - Recall pickup period (day): 5

4. Patrons used: I changed three patrons so that they are all in the same library (Centerville).

5. The tests fail after the patch for master was applied:

prove t/db_dependent/Koha/Item.t
t/db_dependent/Koha/Item.t .. 22/30     # Looks like you planned 23 tests but ran 24.
t/db_dependent/Koha/Item.t .. 25/30 
#   Failed test 'Recalls tests'
#   at t/db_dependent/Koha/Item.t line 2122.
t/db_dependent/Koha/Item.t .. 28/30 # Looks like you failed 1 test of 30.
t/db_dependent/Koha/Item.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/30 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Item.t (Wstat: 256 Tests: 30 Failed: 1)
  Failed test:  25
  Non-zero exit status: 1
Files=1, Tests=30, 13 wallclock secs ( 0.04 usr  0.02 sys +  8.30 cusr  3.56 csys = 11.92 CPU)
Result: FAIL
Comment 11 David Nind 2024-01-09 05:42:04 UTC
The patch for master still applies, however the tests are still failing.

I haven't fully re-tested the 22.05 and master patches yet - see comment #10