Currently an item can be marked withdrawn if it is checked out to a patron or in transit from one branch to another. This causes confusion for staff checking an item back in or receiving the transfer. If an item is checked out or in transit it should not be able to be marked withdrawn.
Conversely, it would be nice to have the option to have items check-in automatically (be removed from a customer's account) when a staff member marks them as withdrawn, so that they don't linger forever in both statuses.
+! agreed - If an item is checked out or in transit, it should not be able to be marked withdrawn.
Also agree! This is a major cause for confusion with our staff as well. Preventing items that aren't checked in from being withdrawn would be a huge help.
Automatic return might not be very forgiving if you ended up picking a wrong item. Maybe easier to warn/deactivate and have the staff person return it if it's required.
I would like a message to pop up when you try to check in a withdrawn item that notifies staff that the item is withdrawn. Presently, it returns the message that the item was not checked out even on items that are both checked out and withdrawn
Created attachment 176241 [details] [review] Bug 23010: Add system preference PreventWithdrawingItemsStatus
(In reply to Felicity Brown from comment #1) > Conversely, it would be nice to have the option to have items check-in > automatically (be removed from a customer's account) when a staff member > marks them as withdrawn, so that they don't linger forever in both statuses. This is a cool idea and I encourage you to file it as a separate bug!
Created attachment 177299 [details] [review] Bug 23010: Add ability to block withdrawning of items with certain statuses
Created attachment 177402 [details] [review] Bug 23010: Handle error in updateitem.pl and template
Created attachment 177413 [details] [review] Bug 23010: Add unit tests
Created attachment 177434 [details] [review] Bug 23010: Add system preference PreventWithdrawingItemsStatus To test: 1. APPLY PATCH, updatedatabase, and restart_all 2. Search for the system preference, PreventWithDrawingItemsStatus 3. Select both in transit and checked out, then save 4. Find an item and put it in transit. 5. Now from the item detail page, go to manage items > edit item 6. Using the item that was put in transit, attempt to change the status to withdrawn. 7. You should see an error at the top of the page 'In transit item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 8. Using a different item, do a checkout 9. Now from the item detail page, go to manage items > edit item 10. Using the checked out item, attempt to change the status to withdrawn. 11. vYou should see an error at the top of the page 'Onloan item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 12. Now, using the in transit item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=x ) 13. Under Statuses, attempt to withdraw the item. 14. You should see an error near the top of the page "Cannot withdraw item in transit". 15. Now, using the checked out item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=144 ) 16. Under Statuses, attempt to withdraw the item. 17. You should see an error near the top of the page "Cannot withdraw checked out item.". 18. Using both the in-transit and checked out item, attempt to modify the status through batchMod. 19. Ensure that the items are not being updated to withdrawn. 20. Turn off PreventWithDrawingItemsStatus system preferece. 21. Going through each of the steps again, except this time your items should successfully withdraw. prove t/db_dependent/Koha/Item.t
Created attachment 177435 [details] [review] Bug 23010: Add ability to block withdrawning of items with certain statuses
Created attachment 177436 [details] [review] Bug 23010: Handle error in updateitem.pl and template
Created attachment 177437 [details] [review] Bug 23010: Add unit tests
Created attachment 177451 [details] [review] Bug 23010: Add system preference PreventWithdrawingItemsStatus To test: 1. APPLY PATCH, updatedatabase, and restart_all 2. Search for the system preference, PreventWithDrawingItemsStatus 3. Select both in transit and checked out, then save 4. Find an item and put it in transit. 5. Now from the item detail page, go to manage items > edit item 6. Using the item that was put in transit, attempt to change the status to withdrawn. 7. You should see an error at the top of the page 'In transit item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 8. Using a different item, do a checkout 9. Now from the item detail page, go to manage items > edit item 10. Using the checked out item, attempt to change the status to withdrawn. 11. vYou should see an error at the top of the page 'Onloan item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 12. Now, using the in transit item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=x ) 13. Under Statuses, attempt to withdraw the item. 14. You should see an error near the top of the page "Cannot withdraw item in transit". 15. Now, using the checked out item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=144 ) 16. Under Statuses, attempt to withdraw the item. 17. You should see an error near the top of the page "Cannot withdraw checked out item.". 18. Using both the in-transit and checked out item, attempt to modify the status through batchMod. 19. Ensure that the items are not being updated to withdrawn. 20. Turn off PreventWithDrawingItemsStatus system preferece. 21. Going through each of the steps again, except this time your items should successfully withdraw. prove t/db_dependent/Koha/Item.t Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177452 [details] [review] Bug 23010: Add ability to block withdrawning of items with certain statuses Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177453 [details] [review] Bug 23010: Handle error in updateitem.pl and template Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177454 [details] [review] Bug 23010: Add unit tests Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
SO, it works as described, no possibility to set withdraw when PreventWithDrawingItemsStatus is active (Items tab, edit item, batchMod). What about clearing withdraw status when PreventWithDrawingItemsStatus is active -- still possible (Items tab, edit item), not possible (batchMod)?
Created attachment 177911 [details] [review] Bug 23010: Add system preference PreventWithdrawingItemsStatus To test: 1. APPLY PATCH, updatedatabase, and restart_all 2. Search for the system preference, PreventWithDrawingItemsStatus 3. Select both in transit and checked out, then save 4. Find an item and put it in transit. 5. Now from the item detail page, go to manage items > edit item 6. Using the item that was put in transit, attempt to change the status to withdrawn. 7. You should see an error at the top of the page 'In transit item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 8. Using a different item, do a checkout 9. Now from the item detail page, go to manage items > edit item 10. Using the checked out item, attempt to change the status to withdrawn. 11. vYou should see an error at the top of the page 'Onloan item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 12. Now, using the in transit item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=x ) 13. Under Statuses, attempt to withdraw the item. 14. You should see an error near the top of the page "Cannot withdraw item in transit". 15. Now, using the checked out item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=144 ) 16. Under Statuses, attempt to withdraw the item. 17. You should see an error near the top of the page "Cannot withdraw checked out item.". 18. Using both the in-transit and checked out item, attempt to modify the status through batchMod. 19. Ensure that the items are not being updated to withdrawn. 20. Turn off PreventWithDrawingItemsStatus system preferece. 21. Going through each of the steps again, except this time your items should successfully withdraw. prove t/db_dependent/Koha/Item.t Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177912 [details] [review] Bug 23010: Add ability to block withdrawning of items with certain statuses Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177913 [details] [review] Bug 23010: Handle error in updateitem.pl and template Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 177914 [details] [review] Bug 23010: Add unit tests Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Rebased using the auto-rebase script, and some manual fixes.
Created attachment 178808 [details] [review] Bug 23010: Fix file permission of atomicupdate
Needs another rebase, unfortunately :( Looking at the code, this looks very promising! I do have one question, though: in Item.pm, the store method is throwing an exception in the middle if a withdrawn status is blocked by the syspref. Wouldn't that interrupt the processing of unrelated fields? (Or am I missing some Perl magic here?) Also, quick spelling fix: the syspref name should be PreventWithdrawing... (lowercase d), not PreventWithDrawing... (uppercase D), since withdrawing is one word.
Created attachment 181322 [details] [review] Bug 23010: Add system preference PreventWithdrawingItemsStatus To test: 1. APPLY PATCH, updatedatabase, and restart_all 2. Search for the system preference, PreventWithDrawingItemsStatus 3. Select both in transit and checked out, then save 4. Find an item and put it in transit. 5. Now from the item detail page, go to manage items > edit item 6. Using the item that was put in transit, attempt to change the status to withdrawn. 7. You should see an error at the top of the page 'In transit item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 8. Using a different item, do a checkout 9. Now from the item detail page, go to manage items > edit item 10. Using the checked out item, attempt to change the status to withdrawn. 11. vYou should see an error at the top of the page 'Onloan item cannot be withdrawn.' Make sure the item did not get marked as withdrawn. 12. Now, using the in transit item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=x ) 13. Under Statuses, attempt to withdraw the item. 14. You should see an error near the top of the page "Cannot withdraw item in transit". 15. Now, using the checked out item, go to the items tab. ( catalogue/moredetail.pl?biblionumber=144 ) 16. Under Statuses, attempt to withdraw the item. 17. You should see an error near the top of the page "Cannot withdraw checked out item.". 18. Using both the in-transit and checked out item, attempt to modify the status through batchMod. 19. Ensure that the items are not being updated to withdrawn. 20. Turn off PreventWithDrawingItemsStatus system preferece. 21. Going through each of the steps again, except this time your items should successfully withdraw. prove t/db_dependent/Koha/Item.t Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 181323 [details] [review] Bug 23010: Add ability to block withdrawning of items with certain statuses Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 181324 [details] [review] Bug 23010: Handle error in updateitem.pl and template Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 181325 [details] [review] Bug 23010: Add unit tests Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Created attachment 181326 [details] [review] Bug 23010: Fix file permission of atomicupdate
Created attachment 181327 [details] [review] Bug 23010: Update pref name to PreventWithdrawingItemsStatus
> I do have one question, though: in Item.pm, the store method is throwing an > exception in the middle if a withdrawn status is blocked by the syspref. > Wouldn't that interrupt the processing of unrelated fields? (Or am I missing > some Perl magic here?) Good question, Emily! I wrote this so if you try to update an item to a status that is not allowed via the system preference, we stop and exit. You can't get into this IF statement unless the syspref is on and you are editing the withdrawn status. If you edit other fields they won't be saved. It sounds like you would like this warn the user that they cannot updated the withdrawn status but continue to update other fields they have changes at that same time?
Created attachment 181400 [details] [review] Bug 23010: Report on count of items not modified via batchMod
(In reply to Lucas Gass (lukeg) from comment #33) > > I do have one question, though: in Item.pm, the store method is throwing an > > exception in the middle if a withdrawn status is blocked by the syspref. > > Wouldn't that interrupt the processing of unrelated fields? (Or am I missing > > some Perl magic here?) > > Good question, Emily! I wrote this so if you try to update an item to a > status that is not allowed via the system preference, we stop and exit. You > can't get into this IF statement unless the syspref is on and you are > editing the withdrawn status. If you edit other fields they won't be saved. > > It sounds like you would like this warn the user that they cannot updated > the withdrawn status but continue to update other fields they have changes > at that same time? Thanks for clarifying the intent here, Lucas! I was partly thinking about whether other fields should still be updated, and partly concerned about whether there would be side effects from the code that runs before we stop and exit. Both of these concerns mostly came from me missing/forgetting how Koha Objects and DBIC work, so please disregard my previous comment. :) I will try to QA this next week, but I'm not confident I'll be able to get to it before the soft feature freeze, so not setting myself as the QA contact just yet.
Thanks for working on this one, Lucas! I found a few things that need follow-up, but it's pretty close! If you're able to follow up before the deadline for this cycle, I'll do my best to review it right away and move it along. == Blockers == - Test coverage needed for the changes to BatchItemUpdate.pm and Items.pm in the last patch - The Automatic Item Modification by Age tool is able to set the withdrawn status, so it needs to handle the exception. Currently, with the patchset, the cron crashes if it tries to set the withdrawn status on an item where withdrawing is not allowed. Example steps to produce the error: 1. Set PreventWithdrawingItemsStatus to prevent withdrawing on both checkout and in-transit 2. Add the following Automatic Item Modification by Age rule: age: 365 age field: items.dateaccessioned condition: items.biblionumber = 126 substitution: items.withdrawn = 1 **Note, due to bug 39864, you currently cannot do this through the staff interface, and you need to insert the rule directly into the database: INSERT INTO systempreferences(variable, value) VALUES ('automatic_item_modification_by_age_configuration', '[{"agefield":"items.dateaccessioned","conditions":[{"value":126,"field":"items.biblionumber"}],"age":"400","substitutions":[{"field":"items.withdrawn","value":"1"}]}]'); 3. Search for biblionumber 126 (Intermediate Perl) 4. Check out one of its items and put another one of its items in-transit 5. misc/cronjobs/automatic_item_modification_by_age.pl -v --> Confirm the cron will try to withdraw the 4 items from that bib 6. misc/cronjobs/automatic_item_modification_by_age.pl -c --> Cron job throws an exception (When I tested the above, one of the items that should have been withdrawn did not get withdrawn because the cron crashed, but I'm not sure what determines the order in which the items are processed, so ymmv) == Needs feedback == From additem.pl (around line 650): + } catch { + if ( ref $_ && $_->can('error') ) { + push @errors, $_->error; + } + } Should we do something else with the error (or die/rethrow) if not $_->can('error')? I imagine it would be a pretty unusual edge case for that check to fail, but if it does, we shouldn't fail silently. ---- From cataloguing_additem.js: $(document).ready(function () { + $( + '#cataloguing_additem_newitem #f[action^="/cgi-bin/koha/cataloguing/additem.pl"]' + ); // Remove the onclick event defined in browser.js, // otherwise the deletion confirmation will not work correctly $('a[href*="biblionumber="]').off("click"); It's not clear to me what this change does or if it's intentional. If it is an intentional change, can you clarify?
Created attachment 182124 [details] [review] Bug 23010: Handle error in C4/Items.pm using try/catch
Created attachment 182129 [details] [review] Bug 23010: Add tests for AutomaticItemModificationByAge
> - The Automatic Item Modification by Age tool is able to set the withdrawn > status, so it needs to handle the exception. Currently, with the patchset, > the cron crashes if it tries to set the withdrawn status on an item where > withdrawing is not allowed. > Example steps to produce the error: > 1. Set PreventWithdrawingItemsStatus to prevent withdrawing on both checkout > and in-transit > 2. Add the following Automatic Item Modification by Age rule: > age: 365 > age field: items.dateaccessioned > condition: items.biblionumber = 126 > substitution: items.withdrawn = 1 > **Note, due to bug 39864, you currently cannot do this through the staff > interface, and you need to insert the rule directly into the database: > INSERT INTO systempreferences(variable, value) VALUES > ('automatic_item_modification_by_age_configuration', > '[{"agefield":"items.dateaccessioned","conditions":[{"value":126,"field": > "items.biblionumber"}],"age":"400","substitutions":[{"field":"items. > withdrawn","value":"1"}]}]'); > > 3. Search for biblionumber 126 (Intermediate Perl) > 4. Check out one of its items and put another one of its items in-transit > 5. misc/cronjobs/automatic_item_modification_by_age.pl -v > --> Confirm the cron will try to withdraw the 4 items from that bib > 6. misc/cronjobs/automatic_item_modification_by_age.pl -c > --> Cron job throws an exception Great catch here, I've corrected it with a follow up patch.
Created attachment 182130 [details] [review] Bug 23010: Remove JS added by mistake
> From cataloguing_additem.js: > > $(document).ready(function () { > + $( > + '#cataloguing_additem_newitem > #f[action^="/cgi-bin/koha/cataloguing/additem.pl"]' > + ); > // Remove the onclick event defined in browser.js, > // otherwise the deletion confirmation will not work correctly > $('a[href*="biblionumber="]').off("click"); > > It's not clear to me what this change does or if it's intentional. If it is > an intentional change, can you clarify? Definitely a mistake, I have removed it.
> == Needs feedback == > > From additem.pl (around line 650): > > + } catch { > + if ( ref $_ && $_->can('error') ) { > + push @errors, $_->error; > + } > + } > > Should we do something else with the error (or die/rethrow) if not > $_->can('error')? I imagine it would be a pretty unusual edge case for that > check to fail, but if it does, we shouldn't fail silently. OK, looking back here I am unclear why I did this. I don't think we need to do the `if ( ref $_ && $_->can('error') ) {` We should just try to store and if not catch the error
Created attachment 182131 [details] [review] Bug 23010: Clean up try/catch in additem.pl
> - Test coverage needed for the changes to BatchItemUpdate.pm and Items.pm in > the last patch I'm really not sure how to write a test for the 1 line change in BatchItemUpdate.pm...