Bug 39871 - Clearing item statuses with batch item modification tool does not work correctly
Summary: Clearing item statuses with batch item modification tool does not work correctly
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Emily Lamancusa (emlam)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2025-05-09 18:21 UTC by Emily Lamancusa (emlam)
Modified: 2025-08-06 20:24 UTC (History)
5 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes a bug with the Batch item modification tool. Previously, if library staff tried to clear the items' not-for-loan, withdrawn, lost, or damaged status using the Batch item modification tool, the fields would not be cleared correctly. Depending on the database settings, the job might fail completely and the items wouldn't be modified at all, or else the status would be cleared, but the status date (such as withdrawn_on or itemlost_on) would not be cleared. Now the tool can be used to clear those fields, just like any other non-mandatory field.
Version(s) released in:
25.11.00,25.05.02,24.11.08
Circulation function:


Attachments
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool (1.82 KB, patch)
2025-06-25 16:57 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool (1.87 KB, patch)
2025-06-26 14:38 UTC, David Flater
Details | Diff | Splinter Review
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool (1.93 KB, patch)
2025-06-26 15:19 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool (2.03 KB, patch)
2025-06-27 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39871: (QA follow-up) Do not hardcode nullable check (2.66 KB, patch)
2025-06-27 09:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 39871: (QA follow-up) Do not hardcode nullable check (3.00 KB, patch)
2025-06-27 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa (emlam) 2025-05-09 18:21:33 UTC
To replicate:
1. Set not-for-loan, withdrawn, and damaged statuses on some items
2. Try to edit only those items with the Batch item modification tool
3. Check the checkboxes to clear the withdrawn, not-for-loan, and damaged statuses
4. Click Save

If you view the background job report, it will say that the items could not be modified, and if you view the items you will see that the statuses were not cleared.

The background job logs show database warnings saying that those columns cannot be set to null.

More testing may be needed to see if this applies to other fields as well. I suspect it will apply to the lost field as well, but the lost status is currently hidden on the batch item modification screen on main for me - not sure if that is another bug or a configuration I wasn't aware of.
Comment 1 Emily Lamancusa (emlam) 2025-06-25 16:57:39 UTC Comment hidden (obsolete)
Comment 2 David Flater 2025-06-26 14:38:12 UTC Comment hidden (obsolete)
Comment 3 Owen Leonard 2025-06-26 15:19:28 UTC
Created attachment 183549 [details] [review]
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool

To test:
1. Set not-for-loan, withdrawn, and damaged statuses on some items
2. Try to edit only those items with the Batch item modification tool
3. Check the checkboxes to clear the withdrawn, not-for-loan, and
   damaged statuses
4. Click Save
5. View the background job report
   --> Note that it says the items could not be modified
6. Look at the items
   --> The statuses have not been cleared
7. Apply patch and restart_all
8. Repeat steps 2-5
   --> The background job report says the modifications were successful
9. View the items
   --> The statuses have been cleared

Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Marcel de Rooy 2025-06-27 08:17:32 UTC
Looking here
Comment 5 Marcel de Rooy 2025-06-27 09:23:26 UTC
Doing something wrong helps you find bugs :)

batchMod works with a nested try/catch construction:

First the worker calls Koha/BackgroundJob/BatchUpdateItem.pm doing
    try {
        my ($results) = Koha::Items->search( { itemnumber => \@record_ids } )->batch_update(
etc
    } catch {
        warn $_;
        die "Something terrible has happened!"
            if ( $_ =~ /Rollback failed/ );    # Rollback failed

And batch_update does this
        try {
            $schema->txn_do(
                sub {
[etc]
$item->set($new_values)->store( { skip_record_index => 1 } );
[etc]
        } catch {
            push @errors, {
                error => eval { $_->{error} } || "$_",
            };
            warn $_
Note here that the catch does not die or raise an exception! I found the error in worker-output.log:
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'enumchron' cannot be null at /usr/share/koha/Koha/Object.pm line 174
This error was raised by doing something wrong but that does not matter here.
The point is that the inner try block only warns in the catch. So the outer try block does not catch anything! The error is just silently ignored.
I just got back: No items modified.

Note that this error should have its own new report. Will open one.

What did I do wrong btw? I tested my follow-up with withdrawn and enumchron. First I tested if enumchron was blanked with NULL. That was ok. Then I just altered the table items with enumchron NOT NULL and hoped to see that it would be empty string. Forgetting that I am asking DBIx if it is nullable, so DBIx had not changed and said Go ahead. Etc etc.
Comment 6 Marcel de Rooy 2025-06-27 09:30:46 UTC
Created attachment 183591 [details] [review]
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool

To test:
1. Set not-for-loan, withdrawn, and damaged statuses on some items
2. Try to edit only those items with the Batch item modification tool
3. Check the checkboxes to clear the withdrawn, not-for-loan, and
   damaged statuses
4. Click Save
5. View the background job report
   --> Note that it says the items could not be modified
6. Look at the items
   --> The statuses have not been cleared
7. Apply patch and restart_all
8. Repeat steps 2-5
   --> The background job report says the modifications were successful
9. View the items
   --> The statuses have been cleared

Signed-off-by: David Flater <flaterdavid@gmail.com>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2025-06-27 09:30:48 UTC
Created attachment 183592 [details] [review]
Bug 39871: (QA follow-up) Do not hardcode nullable check

Lets check DBIx's columns_info.

Test plan:
Try to blank an integer column and a string column. Verify results.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2025-06-27 09:32:19 UTC
(In reply to Emily Lamancusa (emlam) from comment #0)
> The background job logs show database warnings saying that those columns
> cannot be set to null.

Actually an issue on itself as confirmed above.
Comment 9 Marcel de Rooy 2025-06-27 09:39:35 UTC
Wait. This follow-up does not handle not-nullable dates correctly (if we have them in items)
Comment 10 Marcel de Rooy 2025-06-27 09:48:42 UTC
Created attachment 183596 [details] [review]
Bug 39871: (QA follow-up) Do not hardcode nullable check

Lets check DBIx's columns_info.

Test plan:
Try to blank an integer column and a string column. Verify results.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Lucas Gass (lukeg) 2025-06-30 14:32:20 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 12 Paul Derscheid 2025-07-10 20:50:14 UTC
Nice work everyone!

Pushed to 25.05.x
Comment 13 Fridolin Somers 2025-07-28 08:14:52 UTC
Pushed to 24.11.x for 24.11.08

Whould be great to have a release note, it is a quite complexe change to explain
Comment 14 Emily Lamancusa (emlam) 2025-08-01 16:24:05 UTC
As it turns out, this bug manifests differently on production servers if they have SQL strict mode turned off. Without strict mode, the batch edit completes successfully, but the withdrawn_on, itemlost_on, etc date is not cleared.

It's probably not necessary to go back and change the release notes for 25.05.02 and 24.11.08 since no one else has reported that bug, but I'll change them for 25.11 at least.

Mostly I wanted to make a note on the off chance that another library notices their withdrawn_on/itemlost_on/etc dates aren't getting cleared, otherwise they have no way of knowing this is the same bug. (Hopefully this will save someone else from spending a bunch of time trying to track down the bug like I just did!)