In the work on bug 23463 the subroutine _set_defaults_for_add was removed in commit 02134d7604a6fc08b88046fde87b6c909de04353. This subroutine was called when a new item was added (in subroutines AddItem and AddItemBatchFromMarc). However, commit 02134d7604a6fc08b88046fde87b6c909de04353 adds this to the store method of Koha::Item : + unless ( $self->dateaccessioned ) { + $self->dateaccessioned($today); + } + I'm guessing this migth have been a mistake, and it should have been added to the new method? The problem here is that libraries that have old items with dateaccessioned NULL will gradually get their database filled with garbage dateaccessioned as the date gets updated whenever the item is issued and therefore will be unable to rely on the accuracy of dateaccessioned.
... and it will show up in new acquisition lists etc. Not good. :( Koha always enforced the dateaccessioned, so it can only be empty as a result of data manipulation or migrations.
Did you see: 29815 Koha Serials alexbuckley@catalyst.net.nz Push --- Pre-populate 'Date acquired' field when adding/editing items
Hm, as Koha always forced dateaccessioned itself, I wonder if this is not actually a data problem :( The field cannot be null, unless you explicitly forced it to be. When migrating data using the import tools, the date is also set. The only way to end up with empty is by migrating with SQL, I think.
If we would remove the second $self->dateaccessioned($today) that is after the modify branch in store, we still have the effect that opening an item that has a NULL date in dateaccessioned will be populated with today due to 29815 (the value builder will put today in that field now). Does it make more sense to choose some arbitrary date for all unknown acquired dates and run a SQL update to replace the NULLs? And even the 0000-00-00 values that I still discovered in my table too?
Indeed, missing dateaccessioned fields stems from data migration. And sometimes there is not meaningful date in the source data, so it makes sense to allow it to be empty to indicate that the date is unknown. As it stands we have to use a garbage date such as 1901-01-01 to indicate that the date is unknown.
Adding see also to bug 27768. Strange values like 0000-00-00, 0001-01-01 etc. Date picker does not like them either.
(In reply to Marcel de Rooy from comment #4) > Does it make more sense to choose some arbitrary date for all unknown > acquired dates and run a SQL update to replace the NULLs? And even the > 0000-00-00 values that I still discovered in my table too? +1
In any case, having $self->dateaccessioned($today) twice in store() is a bug.
Updating severity... maybe it will draw a patch?
Created attachment 136504 [details] [review] Bug 29958: Do not set dateaccessioned on updates This patch reomves the second occurence of setting daeaccessioned today, outside of the 'add/update' handling in Koha::Item->store.
There wasn't actually a unit test for this.. either way. I'm not sure what the repercussions are of having a null deaccessioned in the database?
Created attachment 136555 [details] [review] Bug 29958: Do not set dateaccessioned on updates This patch reomves the second occurence of setting daeaccessioned today, outside of the 'add/update' handling in Koha::Item->store. Signed-off-by: David Nind <david@davidnind.com>
Testing notes: - Before patch applied: edit an item with an accessioned date, remove the accessioned date, update another field (for example: copy number), save ==> accessioned date changed to today (couldn't find a record/item without a date) - After patch applied: . edit an item with an accessioned date, remove the accessioned date, update another field (for example: copy number), save ==> accessioned date is empty . edit the same item, leaved accessioned date blank, edit another field (such as copy number), save ==> accessioned date remains empty
Created attachment 137052 [details] [review] Bug 29958: Do not set dateaccessioned on updates This patch reomves the second occurence of setting daeaccessioned today, outside of the 'add/update' handling in Koha::Item->store. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Martin: Any chance of adding a dbrev that is able to clear the date for these records too? For instance, we use the date to do acquisition reporting on year/month; and these data are useless now since we have a lot of old records without the date and checkins/checkouts now have added the date.
I don't think we can separate the accidental updates from the intended ones - so not sure how a database update could be written?
(In reply to Katrin Fischer from comment #16) > I don't think we can separate the accidental updates from the intended ones > - so not sure how a database update could be written? I was thinking that we had a trace of eg a checkin/checkout and an item store but unfortunately the latter is missing: $issue->item->onloan(undef)->store({ log_action => 0, skip_record_index => $params->{skip_record_index} }); (from MarkIssueReturned) If we would have had both, we could make an educated guess. But if we only have an checkin or checkout without an item modification trace, I am afraid that it is not possible. Any more thoughts? Heading to the old database dumps ;)
Created attachment 137144 [details] [review] Bug 29958: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 137145 [details] [review] Bug 29958: Do not set dateaccessioned on updates This patch reomves the second occurence of setting daeaccessioned today, outside of the 'add/update' handling in Koha::Item->store. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for 22.05.04
pushed to 21.11.x for 21.11.11
Backported: Pushed to 21.05.x branch for 21.05.18 Nothing to document, marking resolved.