|
Description
Andrew Fuerste-Henry
2020-04-17 17:39:51 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. 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! 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 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> 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> 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! 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! 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> 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> Works well, passes QA scripts and has unit tests.. Passing QA I got scared for a second here until I realized there is a pref :D Nice work everyone! Pushed to master for 20.05 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
Created attachment 104666 [details] [review] Bug 25184: (RM follow-up) Syntax error correction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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
backported to 19.05.x for 19.05.11 |