Bug 23010 - If an item is checked out or in transit it should not be able to be marked withdrawn
Summary: If an item is checked out or in transit it should not be able to be marked wi...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Lucas Gass (lukeg)
QA Contact: Emily Lamancusa (emlam)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-05-29 14:25 UTC by Jessie Zairo
Modified: 2025-05-08 22:45 UTC (History)
14 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 23010: Add system preference PreventWithdrawingItemsStatus (4.51 KB, patch)
2025-01-08 15:12 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add ability to block withdrawning of items with certain statuses (4.14 KB, patch)
2025-01-28 23:05 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Handle error in updateitem.pl and template (3.71 KB, patch)
2025-01-31 19:02 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add unit tests (2.65 KB, patch)
2025-01-31 21:20 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add system preference PreventWithdrawingItemsStatus (6.15 KB, patch)
2025-01-31 22:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add ability to block withdrawning of items with certain statuses (4.14 KB, patch)
2025-01-31 22:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Handle error in updateitem.pl and template (3.71 KB, patch)
2025-01-31 22:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add unit tests (2.65 KB, patch)
2025-01-31 22:24 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add system preference PreventWithdrawingItemsStatus (6.19 KB, patch)
2025-02-02 17:29 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 23010: Add ability to block withdrawning of items with certain statuses (4.19 KB, patch)
2025-02-02 17:29 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 23010: Handle error in updateitem.pl and template (3.76 KB, patch)
2025-02-02 17:29 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 23010: Add unit tests (2.70 KB, patch)
2025-02-02 17:29 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 23010: Add system preference PreventWithdrawingItemsStatus (6.98 KB, patch)
2025-02-12 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23010: Add ability to block withdrawning of items with certain statuses (4.01 KB, patch)
2025-02-12 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23010: Handle error in updateitem.pl and template (3.64 KB, patch)
2025-02-12 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23010: Add unit tests (2.73 KB, patch)
2025-02-12 14:02 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23010: Fix file permission of atomicupdate (675 bytes, patch)
2025-02-27 20:12 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add system preference PreventWithdrawingItemsStatus (6.47 KB, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add ability to block withdrawning of items with certain statuses (3.99 KB, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Handle error in updateitem.pl and template (3.60 KB, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add unit tests (2.75 KB, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Fix file permission of atomicupdate (675 bytes, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Update pref name to PreventWithdrawingItemsStatus (3.65 KB, patch)
2025-04-22 22:27 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Report on count of items not modified via batchMod (3.07 KB, patch)
2025-04-23 22:43 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Handle error in C4/Items.pm using try/catch (1.00 KB, patch)
2025-05-08 21:30 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Add tests for AutomaticItemModificationByAge (4.37 KB, patch)
2025-05-08 22:25 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Remove JS added by mistake (1015 bytes, patch)
2025-05-08 22:29 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 23010: Clean up try/catch in additem.pl (744 bytes, patch)
2025-05-08 22:37 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jessie Zairo 2019-05-29 14:25:57 UTC
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.
Comment 1 Felicity Brown 2022-09-13 17:54:25 UTC
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.
Comment 2 Rebecca Coert 2022-09-27 16:25:24 UTC
+! agreed - If an item is checked out or in transit, it should not be able to be marked withdrawn.
Comment 3 Katie Bliss 2023-01-30 16:32:00 UTC
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.
Comment 4 Katrin Fischer 2023-10-28 15:08:13 UTC
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.
Comment 5 lthorrat@cuyahogalibrary.org 2023-12-29 16:39:07 UTC
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
Comment 6 Lucas Gass (lukeg) 2025-01-08 15:12:58 UTC
Created attachment 176241 [details] [review]
Bug 23010: Add system preference PreventWithdrawingItemsStatus
Comment 7 Andrew Fuerste-Henry 2025-01-15 15:31:12 UTC
(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!
Comment 8 Lucas Gass (lukeg) 2025-01-28 23:05:10 UTC
Created attachment 177299 [details] [review]
Bug 23010: Add ability to block withdrawning of items with certain statuses
Comment 9 Lucas Gass (lukeg) 2025-01-31 19:02:59 UTC
Created attachment 177402 [details] [review]
Bug 23010: Handle error in updateitem.pl and template
Comment 10 Lucas Gass (lukeg) 2025-01-31 21:20:14 UTC
Created attachment 177413 [details] [review]
Bug 23010: Add unit tests
Comment 11 Lucas Gass (lukeg) 2025-01-31 22:24:51 UTC
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
Comment 12 Lucas Gass (lukeg) 2025-01-31 22:24:54 UTC
Created attachment 177435 [details] [review]
Bug 23010: Add ability to block withdrawning of items with certain statuses
Comment 13 Lucas Gass (lukeg) 2025-01-31 22:24:57 UTC
Created attachment 177436 [details] [review]
Bug 23010: Handle error in updateitem.pl and template
Comment 14 Lucas Gass (lukeg) 2025-01-31 22:24:59 UTC
Created attachment 177437 [details] [review]
Bug 23010: Add unit tests
Comment 15 Roman Dolny 2025-02-02 17:29:02 UTC
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>
Comment 16 Roman Dolny 2025-02-02 17:29:05 UTC
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>
Comment 17 Roman Dolny 2025-02-02 17:29:08 UTC
Created attachment 177453 [details] [review]
Bug 23010: Handle error in updateitem.pl and template

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 18 Roman Dolny 2025-02-02 17:29:11 UTC
Created attachment 177454 [details] [review]
Bug 23010: Add unit tests

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 19 Roman Dolny 2025-02-02 17:40:44 UTC
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)?
Comment 20 Jonathan Druart 2025-02-12 14:02:15 UTC
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>
Comment 21 Jonathan Druart 2025-02-12 14:02:18 UTC
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>
Comment 22 Jonathan Druart 2025-02-12 14:02:22 UTC
Created attachment 177913 [details] [review]
Bug 23010: Handle error in updateitem.pl and template

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 23 Jonathan Druart 2025-02-12 14:02:25 UTC
Created attachment 177914 [details] [review]
Bug 23010: Add unit tests

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 24 Jonathan Druart 2025-02-12 14:02:36 UTC
Rebased using the auto-rebase script, and some manual fixes.
Comment 25 Lucas Gass (lukeg) 2025-02-27 20:12:07 UTC
Created attachment 178808 [details] [review]
Bug 23010: Fix file permission of atomicupdate
Comment 26 Emily Lamancusa (emlam) 2025-04-04 20:12:11 UTC
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.
Comment 27 Lucas Gass (lukeg) 2025-04-22 22:27:12 UTC
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>
Comment 28 Lucas Gass (lukeg) 2025-04-22 22:27:16 UTC
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>
Comment 29 Lucas Gass (lukeg) 2025-04-22 22:27:19 UTC
Created attachment 181324 [details] [review]
Bug 23010: Handle error in updateitem.pl and template

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 30 Lucas Gass (lukeg) 2025-04-22 22:27:22 UTC
Created attachment 181325 [details] [review]
Bug 23010: Add unit tests

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 31 Lucas Gass (lukeg) 2025-04-22 22:27:26 UTC
Created attachment 181326 [details] [review]
Bug 23010: Fix file permission of atomicupdate
Comment 32 Lucas Gass (lukeg) 2025-04-22 22:27:29 UTC
Created attachment 181327 [details] [review]
Bug 23010: Update pref name to PreventWithdrawingItemsStatus
Comment 33 Lucas Gass (lukeg) 2025-04-22 22:37:44 UTC
> 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?
Comment 34 Lucas Gass (lukeg) 2025-04-23 22:43:55 UTC
Created attachment 181400 [details] [review]
Bug 23010: Report on count of items not modified via batchMod
Comment 35 Emily Lamancusa (emlam) 2025-04-25 20:18:18 UTC
(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.
Comment 36 Emily Lamancusa (emlam) 2025-05-08 20:11:55 UTC
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?
Comment 37 Lucas Gass (lukeg) 2025-05-08 21:30:15 UTC
Created attachment 182124 [details] [review]
Bug 23010: Handle error in C4/Items.pm using try/catch
Comment 38 Lucas Gass (lukeg) 2025-05-08 22:25:57 UTC
Created attachment 182129 [details] [review]
Bug 23010: Add tests for AutomaticItemModificationByAge
Comment 39 Lucas Gass (lukeg) 2025-05-08 22:27:07 UTC
> - 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.
Comment 40 Lucas Gass (lukeg) 2025-05-08 22:29:14 UTC
Created attachment 182130 [details] [review]
Bug 23010: Remove JS added by mistake
Comment 41 Lucas Gass (lukeg) 2025-05-08 22:29:49 UTC
> 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.
Comment 42 Lucas Gass (lukeg) 2025-05-08 22:34:49 UTC
> == 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
Comment 43 Lucas Gass (lukeg) 2025-05-08 22:37:28 UTC
Created attachment 182131 [details] [review]
Bug 23010: Clean up try/catch in additem.pl
Comment 44 Lucas Gass (lukeg) 2025-05-08 22:45:26 UTC
> - 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...