Bug 29958

Summary: Missing dateaccessioned is set to today when storing an item
Product: Koha Reporter: Andreas Jonsson <andreas.jonsson>
Component: CatalogingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: alexbuckley, arthur.suzuki, david, dcook, fridolin.somers, jonathan.druart, Laura.escamilla, lucas, m.de.rooy, martin.renvoize, ola.andersson, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27768
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29815
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29963
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32451
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes editing items without an accession date - the accessioned date will remain empty, instead of being updated to today's date.
Version(s) released in:
22.11.00, 22.05.04, 21.11.11, 21.05.18
Bug Depends on:    
Bug Blocks: 32456    
Attachments: Bug 29958: Do not set dateaccessioned on updates
Bug 29958: Do not set dateaccessioned on updates
Bug 29958: Do not set dateaccessioned on updates
Bug 29958: Regression tests
Bug 29958: Do not set dateaccessioned on updates

Description Andreas Jonsson 2022-01-27 12:39:17 UTC
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.
Comment 1 Katrin Fischer 2022-01-27 13:43:18 UTC
... 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.
Comment 2 Marcel de Rooy 2022-04-28 09:02:31 UTC
Did you see:

29815	Koha	Serials	alexbuckley@catalyst.net.nz	Push	---	Pre-populate 'Date acquired' field when adding/editing items
Comment 3 Katrin Fischer 2022-04-28 09:06:53 UTC
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.
Comment 4 Marcel de Rooy 2022-04-28 09:16:49 UTC
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?
Comment 5 Andreas Jonsson 2022-04-28 09:18:28 UTC
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.
Comment 6 Marcel de Rooy 2022-04-28 09:21:26 UTC
Adding see also to bug 27768. Strange values like 0000-00-00, 0001-01-01 etc.
Date picker does not like them either.
Comment 7 David Cook 2022-04-28 23:44:40 UTC
(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
Comment 8 Fridolin Somers 2022-06-08 06:36:09 UTC
In any case, having $self->dateaccessioned($today) twice in store() is a bug.
Comment 9 Katrin Fischer 2022-06-09 07:10:15 UTC
Updating severity... maybe it will draw a patch?
Comment 10 Martin Renvoize 2022-06-24 14:42:17 UTC
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.
Comment 11 Martin Renvoize 2022-06-24 14:44:23 UTC
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?
Comment 12 David Nind 2022-06-25 16:33:32 UTC
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>
Comment 13 David Nind 2022-06-25 16:49:33 UTC
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
Comment 14 Marcel de Rooy 2022-07-04 08:16:30 UTC
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>
Comment 15 Marcel de Rooy 2022-07-04 08:20:05 UTC
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.
Comment 16 Katrin Fischer 2022-07-04 10:48:29 UTC
I don't think we can separate the accidental updates from the intended ones - so not sure how a database update could be written?
Comment 17 Marcel de Rooy 2022-07-04 11:57:55 UTC
(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 ;)
Comment 18 Tomás Cohen Arazi 2022-07-05 12:15:58 UTC
Created attachment 137144 [details] [review]
Bug 29958: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Tomás Cohen Arazi 2022-07-05 12:16:03 UTC
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>
Comment 20 Tomás Cohen Arazi 2022-07-05 12:23:29 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 21 Lucas Gass 2022-08-09 22:08:58 UTC
Backported to 22.05.x for 22.05.04
Comment 22 Arthur Suzuki 2022-08-12 07:31:59 UTC
pushed to 21.11.x for 21.11.11
Comment 23 Victor Grousset/tuxayo 2022-08-17 21:46:50 UTC
Backported: Pushed to 21.05.x branch for 21.05.18
Nothing to document, marking resolved.