Bug 30556 - OPAC shows "Place hold" button on available items even if on shelf holds aren't allowed
Summary: OPAC shows "Place hold" button on available items even if on shelf holds aren...
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Hammat wele
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-04-15 20:10 UTC by Caroline Cyr La Rose
Modified: 2022-12-05 17:55 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the OPAC details page for a record so that the "Place hold" button is not shown when on shelf holds are not allowed (when the "On shelf holds allowed" circulation and fines rule is set to either "If any unavailable" or "If all unavailable").
Version(s) released in:


Attachments
Bug 30556: Place hold button doesn't show when not allowed (2.28 KB, text/plain)
2022-04-20 19:21 UTC, The Minh Luong
Details
Bug 30556: Place hold button doesn't show when not allowed (7.44 KB, patch)
2022-04-21 20:26 UTC, The Minh Luong
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.36 KB, patch)
2022-04-27 16:55 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (923 bytes, patch)
2022-04-29 19:18 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (780 bytes, patch)
2022-04-29 20:03 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.40 KB, patch)
2022-04-30 21:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (971 bytes, patch)
2022-04-30 21:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (828 bytes, patch)
2022-04-30 21:52 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.56 KB, patch)
2022-05-04 19:11 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2022-05-05 00:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (877 bytes, patch)
2022-05-05 00:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.61 KB, patch)
2022-05-05 00:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.61 KB, patch)
2022-05-26 18:40 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (1.62 KB, patch)
2022-07-04 13:51 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2022-08-17 13:11 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (877 bytes, patch)
2022-08-17 13:11 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.55 KB, patch)
2022-08-17 13:11 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (1.62 KB, patch)
2022-08-17 13:11 UTC, Shi Yao Wang
Details | Diff | Splinter Review
Bug 30556: Changing the logged-in user's branch by pick-up location (3.38 KB, patch)
2022-12-05 17:39 UTC, Hammat wele
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Caroline Cyr La Rose 2022-04-15 20:10:16 UTC
When circulation rules don't allow on shelf holds, the "Place hold" button still appears on the OPAC next to available items.

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record

The Place hold button should not appear, but currently, it does.

I think this may be related to Bug 3142? I'm not sure...
Comment 1 The Minh Luong 2022-04-20 19:21:22 UTC
Created attachment 133497 [details]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds holds allowed is on "If all unavailable", the Place hold button is not showed.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
Comment 2 The Minh Luong 2022-04-21 20:26:27 UTC
Created attachment 133608 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds holds allowed is on "If all unavailable", the Place hold button is not showed.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
Comment 3 Shi Yao Wang 2022-04-27 16:55:13 UTC
Created attachment 134172 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
Comment 4 Shi Yao Wang 2022-04-29 19:18:25 UTC
Created attachment 134420 [details] [review]
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber
Comment 5 Shi Yao Wang 2022-04-29 20:03:16 UTC
Created attachment 134421 [details] [review]
Bug 30556: Fix IsAvailableForItemLevelRequest to return early
Comment 6 Shi Yao Wang 2022-04-29 20:38:11 UTC
The last 2 patches are not directly related to this bug but they should have been there in the first place to not cause any errors when borrowernumber or patron are undefined.
Comment 7 David Nind 2022-04-30 21:51:52 UTC
Created attachment 134436 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-04-30 21:51:57 UTC
Created attachment 134437 [details] [review]
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2022-04-30 21:52:03 UTC
Created attachment 134438 [details] [review]
Bug 30556: Fix IsAvailableForItemLevelRequest to return early

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-04-30 22:01:13 UTC
I noted when testing that the "Place hold" button is also not displayed when the "If any unavailable" option is selected. I assume that this is OK - I've never really got my head around this setting!
Comment 11 Shi Yao Wang 2022-05-04 19:11:18 UTC
Created attachment 134617 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
8. Logout of the opac.
9. The button is here again since it should always show when not logged
in.

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2022-05-05 00:22:13 UTC
Created attachment 134619 [details] [review]
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 David Nind 2022-05-05 00:22:19 UTC
Created attachment 134620 [details] [review]
Bug 30556: Fix IsAvailableForItemLevelRequest to return early

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 14 David Nind 2022-05-05 00:22:24 UTC
Created attachment 134621 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
8. Logout of the opac.
9. The button is here again since it should always show when not logged
in.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 15 Katrin Fischer 2022-05-06 09:43:41 UTC
Please don't forget to assign yourself to the bug :)
Comment 16 Shi Yao Wang 2022-05-06 12:57:14 UTC
We can probably optimize this bug fix if https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 goes through.
Comment 17 Shi Yao Wang 2022-05-06 12:58:23 UTC
This bug fix could be optimized if https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20985 goes through.
Comment 18 Shi Yao Wang 2022-05-06 13:00:08 UTC
Sorry, sent the same message twice...
Comment 19 Shi Yao Wang 2022-05-26 18:40:48 UTC
Created attachment 135395 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
8. Logout of the opac.
9. The button is here again since it should always show when not logged
in.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 Kyle M Hall 2022-07-01 11:02:29 UTC
(In reply to David Nind from comment #10)
> I noted when testing that the "Place hold" button is also not displayed when
> the "If any unavailable" option is selected. I assume that this is OK - I've
> never really got my head around this setting!

It's awkward language, it boils down to "If I could walk into the library and each of the items on this record are something I could check out right now, don't allow holds." So if I have 10 items on a record, and all down are able to be checked out, don't allow holds. But, if even one is checked out, allow holds.
Comment 21 Kyle M Hall 2022-07-01 11:54:33 UTC
Quick and dirty perf testing, one page load per:

500 Items
Pre-patch
ISBD: 2.5
MARC: 6.21
Details: 4.51

Post-patch
ISBD: 2.84
MARC: 6.15
Details: 5.86

3000 Items
Pre-patch
ISBD: 6.28
MARC: 25.32
Details: 23.48

Post-patch
ISBD: 6.07
MARC: 25.55
Details: 23.51
Comment 22 Kyle M Hall 2022-07-01 12:19:43 UTC
3000 Items
Post-patch, rule = "If any unavailable"
ISBD: 27.01
MARC: 44.76
Details: 43.51
Comment 23 Kyle M Hall 2022-07-01 12:23:05 UTC
It appears that this patch definitely has a noticeable effect on page loads at scale. 

The most immediate blocker at this point is a lack of unit tests for the changes to the Perl modules. Please provide unit tests for the changes to CanBookBeReserved and IsAvailableForItemLevelRequest.
Comment 24 Shi Yao Wang 2022-07-04 13:51:44 UTC
Created attachment 137065 [details] [review]
Bug 30556: (follow-up) Unit tests
Comment 25 Shi Yao Wang 2022-08-17 13:11:36 UTC
Created attachment 139289 [details] [review]
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Shi Yao Wang 2022-08-17 13:11:40 UTC
Created attachment 139290 [details] [review]
Bug 30556: Fix IsAvailableForItemLevelRequest to return early

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 Shi Yao Wang 2022-08-17 13:11:45 UTC
Created attachment 139291 [details] [review]
Bug 30556: Place hold button doesn't show when not allowed

When On shelf holds allowed is on "If all unavailable", the Place hold button is not shown.

To test:

1. Create a circulation rule with on shelf holds set to "If all unavailable"
2. Create or modify a record to match the items itype to the circulation rule
3. Use a patron matching the circ rule category to log into the opac
4. Look for the record
5. Notice that the Place hold button is there, even thought it's not allowed
6. Apply the patch.
7. The button is not there.
8. Logout of the opac.
9. The button is here again since it should always show when not logged
in.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 28 Shi Yao Wang 2022-08-17 13:11:49 UTC
Created attachment 139292 [details] [review]
Bug 30556: (follow-up) Unit tests
Comment 29 Katrin Fischer 2022-10-02 22:37:47 UTC
In your code I see:

+my $currentbranch = C4::Context->userenv ? C4::Context->userenv->{branch} : undef;
+    $is_available = IsAvailableForItemLevelRequest($item, $patron, $currentbranch)
+      unless $is_available;

But when I look at the POD for IsAvailableForItemLevelRequest the third parameter is supposed to be the pick-up location, not the logged-in user's branch:

https://git.koha-community.org/Koha-community/Koha/src/commit/9408a443c24a5424828f62b48fa1dda04fdf3512/C4/Reserves.pm#L1323

my $is_available = IsAvailableForItemLevelRequest( $item_record, $borrower_record, $pickup_branchcode );

I believe this isn't correct. A system might allow you to place a hold for pickup at the item's home library, which might not be the patron's home library.
Comment 30 Hammat wele 2022-12-05 17:39:18 UTC
Created attachment 144429 [details] [review]
Bug 30556: Changing the logged-in user's branch by pick-up location