Bug 25184 - Items with a negative notforloan status should not be captured for holds
Summary: Items with a negative notforloan status should not be captured for holds
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25232
  Show dependency treegraph
 
Reported: 2020-04-17 17:39 UTC by Andrew Fuerste-Henry
Modified: 2020-05-22 14:05 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
**New system preference**: `TrapHoldsOnOrder` defaults to enabled.
Version(s) released in:
20.05.00, 19.11.06, 19.05.11


Attachments
Bug 25184: Items with a negative notforloan status should not be captured for holds (3.92 KB, patch)
2020-04-17 18:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25184: Items with a negative notforloan status should not be captured for holds (3.97 KB, patch)
2020-04-17 18:37 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25184: Items with a negative notforloan status should not be captured for holds (4.04 KB, patch)
2020-04-17 23:51 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 25184: Add syspref (6.76 KB, patch)
2020-04-21 17:21 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25184: Add syspref (6.74 KB, patch)
2020-04-21 17:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25184: Items with a negative notforloan status should not be captured for holds (4.10 KB, patch)
2020-05-06 18:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25184: Add syspref (6.79 KB, patch)
2020-05-06 18:38 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25184: (RM follow-up) Syntax error correction (1.17 KB, patch)
2020-05-11 10:11 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-04-17 17:39:51 UTC
Negative notforloan statuses should allow holds to be placed but not captured.

Due to coronavirus, we have libraries setting all returned materials to a negative notforloan value of Quarantine for several days. They're using UpdateNotForLoanStatusOnCheckin to set that status automatically. However, those items are still capturing for holds, even though those items cannot be checked out until the notforloan status is removed.

In cases like an On Order item where we do want the hold to fill at checkin, UpdateNotForLoanStatusOnCheckin should be used to clear that notforloan status so the hold can fill.
Comment 1 Kyle M Hall 2020-04-17 17:45:10 UTC
I believe this is definitely a bug. In master, if I set an item to a not for loan but holdable status ( < 0 ) I can place the hold, capture the hold and set it to waiting, but *not* check it out to the patron! This does not make sense. I should not be able to trap and item for checkout unless it can be checked out.
Comment 2 Kyle M Hall 2020-04-17 18:11:06 UTC
Created attachment 103177 [details] [review]
Bug 25184: Items with a negative notforloan status should not be captured for holds

Negative notforloan statuses should allow holds to be placed but not captured.

Due to coronavirus, we have libraries setting all returned materials to a negative notforloan value of Quarantine for several days.
They're using UpdateNotForLoanStatusOnCheckin to set that status automatically. However, those items are still capturing for holds,
even though those items cannot be checked out until the notforloan status is removed.

In cases like an On Order item where we do want the hold to fill at checkin,
UpdateNotForLoanStatusOnCheckin should be used to clear that notforloan status so the hold can fill.

In master, if I set an item to a not for loan but holdable status ( < 0 ) I can place the hold,
capture the hold and set it to waiting, but *not* check it out to the patron!
This does not make sense. I should not be able to trap an item for checkout unless it can be checked out.

Test Plan:
1) Set an item's notforloan value to -1
2) Place a hold on that item
3) Check in the item
4) Trap the item for that hold
5) Attempt to check the item out to the patron, you will be unable to
   because it is notforloan
6) Apply this patch
7) Restart all the things!
8) Repeat steps 1-3
9) The screen should no longer ask if the item should be trapped
   to fill the hold!
Comment 3 Andrew Fuerste-Henry 2020-04-17 18:27:23 UTC
Adding two steps to this test plan:

Test Plan:
1) Set an item's notforloan value to -1
2) Place a hold on that item
3) Check in the item
4) Trap the item for that hold
5) Attempt to check the item out to the patron, you will be unable to
   because it is notforloan
6) Apply this patch
7) Restart all the things!
8) Repeat steps 1-3
9) The screen should no longer ask if the item should be trapped
   to fill the hold!
10) Set UpdateNotForLoanStatusOnCheckin to -1: 0
11) Check item in again
12) NotForLoan status is removed and hold is captured
Comment 4 Andrew Fuerste-Henry 2020-04-17 18:37:14 UTC
Created attachment 103180 [details] [review]
Bug 25184: Items with a negative notforloan status should not be captured for holds

Negative notforloan statuses should allow holds to be placed but not captured.

Due to coronavirus, we have libraries setting all returned materials to a negative notforloan value of Quarantine for several days.
They're using UpdateNotForLoanStatusOnCheckin to set that status automatically. However, those items are still capturing for holds,
even though those items cannot be checked out until the notforloan status is removed.

In cases like an On Order item where we do want the hold to fill at checkin,
UpdateNotForLoanStatusOnCheckin should be used to clear that notforloan status so the hold can fill.

In master, if I set an item to a not for loan but holdable status ( < 0 ) I can place the hold,
capture the hold and set it to waiting, but *not* check it out to the patron!
This does not make sense. I should not be able to trap an item for checkout unless it can be checked out.

Test Plan:
1) Set an item's notforloan value to -1
2) Place a hold on that item
3) Check in the item
4) Trap the item for that hold
5) Attempt to check the item out to the patron, you will be unable to
   because it is notforloan
6) Apply this patch
7) Restart all the things!
8) Repeat steps 1-3
9) The screen should no longer ask if the item should be trapped
   to fill the hold!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 5 Andrew Fuerste-Henry 2020-04-17 23:51:50 UTC
Created attachment 103185 [details] [review]
Bug 25184: Items with a negative notforloan status should not be captured for holds

Negative notforloan statuses should allow holds to be placed but not captured.

Due to coronavirus, we have libraries setting all returned materials to a negative notforloan value of Quarantine for several days.
They're using UpdateNotForLoanStatusOnCheckin to set that status automatically. However, those items are still capturing for holds,
even though those items cannot be checked out until the notforloan status is removed.

In cases like an On Order item where we do want the hold to fill at checkin,
UpdateNotForLoanStatusOnCheckin should be used to clear that notforloan status so the hold can fill.

In master, if I set an item to a not for loan but holdable status ( < 0 ) I can place the hold,
capture the hold and set it to waiting, but *not* check it out to the patron!
This does not make sense. I should not be able to trap an item for checkout unless it can be checked out.

Test Plan:
1) Set an item's notforloan value to -1
2) Place a hold on that item
3) Check in the item
4) Trap the item for that hold
5) Attempt to check the item out to the patron, you will be unable to
   because it is notforloan
6) Apply this patch
7) Restart all the things!
8) Repeat steps 1-3
9) The screen should no longer ask if the item should be trapped
   to fill the hold!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Catherine Ingram <Catherine.Ingram@cedarparktexas.gov>
Comment 6 Kyle M Hall 2020-04-21 17:21:14 UTC
Created attachment 103396 [details] [review]
Bug 25184: Add syspref

It's entirely possible that some libraries are relying on the current
before for part of their workflow. Do to this possibility, it seems like
a good idea to control this behavior via a system preference.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Set TrapHoldsOnOrder to "don't trap"
4) Set an item's notforloan value to -1
5) Place a hold on that item
6) Check in the item
7) Note the item is not trapped for hold
9) Set TrapHoldsOnOrder to "trap"
10) Check in the item
11) Koha should now ask if you'd like to trap the item for the hold!
Comment 7 Kyle M Hall 2020-04-21 17:23:51 UTC
Created attachment 103397 [details] [review]
Bug 25184: Add syspref

It's entirely possible that some libraries are relying on the current
before for part of their workflow. Do to this possibility, it seems like
a good idea to control this behavior via a system preference.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Set TrapHoldsOnOrder to "don't trap"
4) Set an item's notforloan value to -1
5) Place a hold on that item
6) Check in the item
7) Note the item is not trapped for hold
9) Set TrapHoldsOnOrder to "trap"
10) Check in the item
11) Koha should now ask if you'd like to trap the item for the hold!
Comment 8 Martin Renvoize 2020-05-06 18:38:23 UTC
Created attachment 104449 [details] [review]
Bug 25184: Items with a negative notforloan status should not be captured for holds

Negative notforloan statuses should allow holds to be placed but not captured.

Due to coronavirus, we have libraries setting all returned materials to a negative notforloan value of Quarantine for several days.
They're using UpdateNotForLoanStatusOnCheckin to set that status automatically. However, those items are still capturing for holds,
even though those items cannot be checked out until the notforloan status is removed.

In cases like an On Order item where we do want the hold to fill at checkin,
UpdateNotForLoanStatusOnCheckin should be used to clear that notforloan status so the hold can fill.

In master, if I set an item to a not for loan but holdable status ( < 0 ) I can place the hold,
capture the hold and set it to waiting, but *not* check it out to the patron!
This does not make sense. I should not be able to trap an item for checkout unless it can be checked out.

Test Plan:
1) Set an item's notforloan value to -1
2) Place a hold on that item
3) Check in the item
4) Trap the item for that hold
5) Attempt to check the item out to the patron, you will be unable to
   because it is notforloan
6) Apply this patch
7) Restart all the things!
8) Repeat steps 1-3
9) The screen should no longer ask if the item should be trapped
   to fill the hold!

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Catherine Ingram <Catherine.Ingram@cedarparktexas.gov>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-05-06 18:38:26 UTC
Created attachment 104450 [details] [review]
Bug 25184: Add syspref

It's entirely possible that some libraries are relying on the current
before for part of their workflow. Do to this possibility, it seems like
a good idea to control this behavior via a system preference.

Test Plan:
1) Apply this patch set
2) Run updatedatabase.pl
3) Set TrapHoldsOnOrder to "don't trap"
4) Set an item's notforloan value to -1
5) Place a hold on that item
6) Check in the item
7) Note the item is not trapped for hold
9) Set TrapHoldsOnOrder to "trap"
10) Check in the item
11) Koha should now ask if you'd like to trap the item for the hold!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-05-06 18:38:53 UTC
Works well, passes QA scripts and has unit tests.. Passing QA
Comment 11 Katrin Fischer 2020-05-06 21:25:57 UTC
I got scared for a second here until I realized there is a pref :D
Comment 12 Martin Renvoize 2020-05-11 08:23:45 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Marcel de Rooy 2020-05-11 09:44:41 UTC
You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 't c)' at line 2 [for Statement "         INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES         ('TrapHoldsOnOrder','1',NULL,'If enabled, Koha will trap holds for on order items ( notforloan < 0 )','YesNo't c)     "] at installer/data/mysql/updatedatabase.pl line 22052
Comment 14 Martin Renvoize 2020-05-11 10:11:32 UTC
Created attachment 104666 [details] [review]
Bug 25184: (RM follow-up) Syntax error correction

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Joy Nelson 2020-05-11 21:58:29 UTC
qa error on updatedatabase statement.

FAIL   installer/data/mysql/updatedatabase.pl
   OK     critic
   FAIL   forbidden patterns
                forbidden pattern: Use INSERT IGNORE INTO on inserting a new syspref (see bug 9071) (line 20517)


Corrected in backport to 19.11.x for 19.11.06
Comment 16 Lucas Gass 2020-05-15 22:05:59 UTC
backported to 19.05.x for 19.05.11