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 |