Summary: | Clearing item statuses with batch item modification tool does not work correctly | ||
---|---|---|---|
Product: | Koha | Reporter: | Emily Lamancusa (emlam) <emily.lamancusa> |
Component: | Cataloging | Assignee: | Emily Lamancusa (emlam) <emily.lamancusa> |
Status: | Pushed to oldstable --- | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | normal | ||
Priority: | P5 - low | CC: | flaterdavid, fridolin.somers, kebliss, lucas, m.de.rooy |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40259 | ||
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
Bug 39871: Don't pass undef for non-nullable fields from batch mod tool Bug 39871: Don't pass undef for non-nullable fields from batch mod tool Bug 39871: Don't pass undef for non-nullable fields from batch mod tool Bug 39871: (QA follow-up) Do not hardcode nullable check Bug 39871: (QA follow-up) Do not hardcode nullable check |
Description
Emily Lamancusa (emlam)
2025-05-09 18:21:33 UTC
Created attachment 183522 [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 Created attachment 183538 [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> 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> Looking here 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. 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> 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> (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. Wait. This follow-up does not handle not-nullable dates correctly (if we have them in items) 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> Nice work everyone! Pushed to main for 25.11 Nice work everyone! Pushed to 25.05.x 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 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!) |