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: RESOLVED INVALID
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: 2023-10-17 14:51 UTC (History)
12 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
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2023-02-24 17:33 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (877 bytes, patch)
2023-02-24 17:33 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.59 KB, patch)
2023-02-24 17:33 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (1.62 KB, patch)
2023-02-24 17:33 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: Changing the logged-in user's branch by pick-up location (3.37 KB, patch)
2023-02-24 17:33 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30846: 'If any unavailable' doesn't consider negative notforlan values as unavailable (10.13 KB, patch)
2023-02-27 13:52 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30846: (follow-up) Unit tests (1.16 KB, patch)
2023-02-27 13:52 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2023-04-25 20:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (877 bytes, patch)
2023-04-25 20:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.59 KB, patch)
2023-04-25 20:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (1.67 KB, patch)
2023-04-25 20:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Changing the logged-in user's branch by pick-up location (3.41 KB, patch)
2023-04-25 20:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2023-09-25 19:11 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (836 bytes, patch)
2023-09-25 19:11 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.58 KB, patch)
2023-09-25 19:11 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (2.00 KB, patch)
2023-09-25 19:11 UTC, Hinemoea Viault
Details | Diff | Splinter Review
Bug 30556: Changing the logged-in user's branch by pick-up location (3.41 KB, text/plain)
2023-09-25 19:12 UTC, Hinemoea Viault
Details
Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved (2.76 KB, patch)
2023-09-30 00:25 UTC, Hammat wele
Details | Diff | Splinter Review
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber (1020 bytes, patch)
2023-10-13 13:36 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 30556: Fix IsAvailableForItemLevelRequest to return early (836 bytes, patch)
2023-10-13 13:36 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 30556: Place hold button doesn't show when not allowed (5.60 KB, patch)
2023-10-13 13:36 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 30556: (follow-up) Unit tests (2.00 KB, patch)
2023-10-13 13:36 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved (2.76 KB, patch)
2023-10-13 13:36 UTC, Emily-Rose Francoeur
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
Comment 31 Hammat wele 2023-02-24 17:33:14 UTC
Created attachment 147378 [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 32 Hammat wele 2023-02-24 17:33:19 UTC
Created attachment 147379 [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 33 Hammat wele 2023-02-24 17:33:23 UTC
Created attachment 147380 [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 34 Hammat wele 2023-02-24 17:33:28 UTC
Created attachment 147381 [details] [review]
Bug 30556: (follow-up) Unit tests
Comment 35 Hammat wele 2023-02-24 17:33:31 UTC
Created attachment 147382 [details] [review]
Bug 30556: Changing the logged-in user's branch by pick-up location
Comment 36 Hammat wele 2023-02-24 17:35:35 UTC
Rebased
Comment 37 Hammat wele 2023-02-27 13:52:00 UTC Comment hidden (obsolete)
Comment 38 Hammat wele 2023-02-27 13:52:04 UTC Comment hidden (obsolete)
Comment 39 David Nind 2023-04-25 20:40:32 UTC
Created attachment 150218 [details] [review]
Bug 30556: Fix CanBookBeReserved to return early if no borrowernumber

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2023-04-25 20:40:35 UTC
Created attachment 150219 [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 41 David Nind 2023-04-25 20:40:38 UTC
Created attachment 150220 [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 42 David Nind 2023-04-25 20:40:41 UTC
Created attachment 150221 [details] [review]
Bug 30556: (follow-up) Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2023-04-25 20:40:44 UTC
Created attachment 150222 [details] [review]
Bug 30556: Changing the logged-in user's branch by pick-up location

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 Jonathan Druart 2023-05-11 09:00:24 UTC
1.
+    return { status => '' } unless $borrowernumber;

+    return 0 unless $patron;

No, you can raise an exception if you want.

2. There is no explanation on the logic changes. Can you detail?

3. Code is duplicated in controller scripts, that feels wrong. We certainly want it in a module, covered by tests.
Comment 45 Katrin Fischer 2023-06-17 21:03:46 UTC
See last comment from Joubu.
Comment 46 Hinemoea Viault 2023-09-25 19:11:46 UTC
Created attachment 156188 [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 47 Hinemoea Viault 2023-09-25 19:11:53 UTC
Created attachment 156189 [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 48 Hinemoea Viault 2023-09-25 19:11:55 UTC
Created attachment 156190 [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 49 Hinemoea Viault 2023-09-25 19:11:58 UTC
Created attachment 156191 [details] [review]
Bug 30556: (follow-up) Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 Hinemoea Viault 2023-09-25 19:12:01 UTC
Created attachment 156192 [details]
Bug 30556: Changing the logged-in user's branch by pick-up location

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 Hammat wele 2023-09-30 00:25:48 UTC
Created attachment 156429 [details] [review]
Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved
Comment 52 David Nind 2023-10-01 18:13:32 UTC
I'm not getting the expected results for the 'On shelf holds allowed', both before and after the patch is applied.

Maybe this is something to do with Bug 34886 - Regression in when hold button appears?

Set up:
- Changed the circulation and fines rules for all patron categories and item types (basically, just used one rule)
- Used Mary Burton as the patron

Before the patch is applied, I am getting this behavour:

- On shelf holds allowed: Yes
  . Not logged in to OPAC: no 'Place hold' option
  . Logged in to OPAC: can place a hold

- On shelf holds allowed: If any unavailable
  . Not logged in to OPAC: no 'Place hold' option
  . Logged in to OPAC: no 'Place hold' option

- On shelf holds allowed: If all unavailable
  . Not logged in to OPAC: no 'Place hold' option
  . Logged in to OPAC: no 'Place hold' option


After the patch is applied, I am getting this behavour:

- On shelf holds allowed: Yes
  . Not logged in to OPAC: no 'Place hold' option
  . Logged in to OPAC: can place a hold

- On shelf holds allowed: If any unavailable
  . Not logged in to OPAC: can place a hold
  . Logged in to OPAC: no 'Place hold' option

- On shelf holds allowed: If all unavailable
  . Not logged in to OPAC: can place a hold
  . Logged in to OPAC: no 'Place hold' option
Comment 53 Emily-Rose Francoeur 2023-10-13 12:54:24 UTC
Hi David,
I tested the patch with the setup you mentioned, and I didn't obtain the same result as you after applying the patch. Here are the results I obtained:

- On-shelf holds allowed: Yes
	. Not logged in to OPAC: Can place a hold (we don’t have the same result here)
	. Logged in to OPAC: Can place a hold
- On-shelf holds allowed: If any unavailable
	. Not logged in to OPAC: Can place a hold
	. Logged in to OPAC: No 'Place hold' option
- On-shelf holds allowed: If all unavailable
	. Not logged in to OPAC: Can place a hold
	. Logged in to OPAC: No 'Place hold' option

I conducted the tests twice and obtained the same results:

- First time: I used a notice with only one item. This item had the "Book" itype, wasn't checked out, and had an available status.
- Second time: I used a notice with two items. These items had the "Book" itype, were not checked out, and had an available status.

Can you describe the notice and the item(s) (itype, status, checked out or not, availability) that you used, and I will attempt to reproduce the issue on my end?
Comment 54 Emily-Rose Francoeur 2023-10-13 13:36:37 UTC
Created attachment 157087 [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 55 Emily-Rose Francoeur 2023-10-13 13:36:40 UTC
Created attachment 157088 [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 56 Emily-Rose Francoeur 2023-10-13 13:36:44 UTC
Created attachment 157089 [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 57 Emily-Rose Francoeur 2023-10-13 13:36:48 UTC
Created attachment 157090 [details] [review]
Bug 30556: (follow-up) Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 Emily-Rose Francoeur 2023-10-13 13:36:51 UTC
Created attachment 157091 [details] [review]
Bug 30556: Fix the call on IsAvailableForItemLevelRequest and CanBookBeReserved
Comment 59 Emily-Rose Francoeur 2023-10-13 13:39:47 UTC
The patch has been rebased
Comment 60 Blou 2023-10-17 14:51:18 UTC
The problem doesn't seem to happen anymore, now that Bug 30846 is on the stack.  We can close this bug.