Bug 36101 - Deleting an item removes the itemnumber from old_reserves when it should not
Summary: Deleting an item removes the itemnumber from old_reserves when it should not
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lari Strand
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-02-15 10:49 UTC by Lari Strand
Modified: 2024-03-19 20:03 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 36101: Drop constraint to items.itemnumber from old_reserves-table (2.25 KB, patch)
2024-03-12 09:55 UTC, Lari Strand
Details | Diff | Splinter Review
Bug 36101: Drop constraint to items.itemnumber from old_reserves-table (2.30 KB, patch)
2024-03-13 21:25 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36101: (follow-up) Fix DBUpdate file permission and perltidy (915 bytes, patch)
2024-03-13 21:25 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Strand 2024-02-15 10:49:47 UTC
Unless there's a good reason for this data loss, the itemnumber should be preserved in old_reserves after deleting an item.

To reproduce:
1.1 Create an item level hold or a hold on a biblio with just one item for it
1.2 Cancel the hold
1.3 Check old_reserves, the itemnumber for the reserve is present
1.4 Delete the item linked to the reserve
1.5 Check old_reserves again, the itemnumber is now NULL 

or:

2.1 Create a hold for a biblio with multiple items
2.2 checkin one of the items linked to the biblio the reserve was made for and confirm the hold
2.3 Cancel the hold
2.3 Check old_reserves, the itemnumber for the reserve is present
2.4 Delete the item linked to the reserve
2.5 Check old_reserves again, the itemnumber is now NULL

The constraint that does the deletion from old_reserves-table when an item is deleted:

CONSTRAINT `old_reserves_ibfk_3` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE SET NULL

This should be removed. After removal the itemnumber does not get deleted from the old_Reserves table when the item is deleted.
Comment 1 Lari Strand 2024-03-12 09:55:35 UTC
Created attachment 163087 [details] [review]
Bug 36101: Drop constraint to items.itemnumber from old_reserves-table

This patch removes the constraint from old_reserves table to the items table so a deleted item's itemnumber will be preserved in the old_reserves table.

To test:

1. Apply patch
2. run updatedatabase.pl
3. Check constraint old_reserves_ibfk_3 has been dropped
Comment 2 Lucas Gass 2024-03-13 21:25:39 UTC
Created attachment 163119 [details] [review]
Bug 36101: Drop constraint to items.itemnumber from old_reserves-table

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 3 Lucas Gass 2024-03-13 21:25:42 UTC
Created attachment 163120 [details] [review]
Bug 36101: (follow-up) Fix DBUpdate file permission and perltidy

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Comment 4 Lucas Gass 2024-03-13 21:27:27 UTC
Looks good, I added a follow to fix the file permission for installer/data/mysql/atomicupdate/bug_36101.pl and perltidy.
Comment 5 Lari Strand 2024-03-14 07:43:25 UTC
Thank you Lucas.
Comment 6 Marcel de Rooy 2024-03-15 09:25:27 UTC
Filling assignee. Please do it yourself next time.
Comment 7 Marcel de Rooy 2024-03-15 09:28:54 UTC
(In reply to Lari Strand from comment #0)
> Unless there's a good reason for this data loss, the itemnumber should be
> preserved in old_reserves after deleting an item.

Please explain why there is a good reason to do so. Keeping an itemnumber in this table that does no longer exist is bad practice.
Comment 8 Jonathan Druart 2024-03-15 09:40:55 UTC
Also we have

CONSTRAINT `old_issues_ibfk_2` FOREIGN KEY (`itemnumber`) REFERENCES `items` (`itemnumber`) ON DELETE SET NULL ON UPDATE SET NULL,
Comment 9 Lari Strand 2024-03-15 11:43:43 UTC
In our case, we need to find out the biblio details for the deleted item so we can show these details for patrons in their lists of past loans/old issues. Without the itemnumber in the old_issues table finding out what the patron has previously loaned is impossible.

We are not using opac as a patron interface, we have our own implementation, that uses this plugin to talk to the web interface:
https://github.com/KohaSuomi/koha-plugin-rest-di

If the biblio is also deleted, we can find the details from deletedbiblio-table.

Maybe finding out biblio details based on the old_issues itemnumber might be interesting for statistical/reporting purposes as well.

"Keeping an itemnumber in this table that does no longer exist is bad practice."

We still have deleted-tables so I don't consider the data as non-existing.

If we touch these old_-tables like this and destroy this archived data that can be linked still with deleted-tables, shouldn't we do the same to action_logs? I consider action_logs as an "archive" table just like old_issues or old_reserves (not sure if we do this already, though. Remove itemnumbers/biblionumbers from action_logs that are linked to deleted items/biblios?).
Comment 10 Lari Strand 2024-03-15 11:49:49 UTC
Sorry, I was supposed to comment this to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32729, but the same logic applies here I assume.
Comment 11 Lari Strand 2024-03-15 11:58:08 UTC
This ticket is just to make handling of these two old_ -tables' itemnumber data uniform.
Comment 12 Lari Strand 2024-03-15 12:33:39 UTC
We should also consider the impact this change makes when we join deleted-tables with "active" tables in the future, when nothing is really deleted and transferred to somewhere else, but tagged as "deleted" in an always "active" table. Will deleting the data still make sense in the future?
Comment 13 Marcel de Rooy 2024-03-18 09:14:30 UTC
(In reply to Lari Strand from comment #9)
> "Keeping an itemnumber in this table that does no longer exist is bad
> practice."
> 
> We still have deleted-tables so I don't consider the data as non-existing.

Dont you think that we should adhere to proven standards as to SQL, normalization, etc ? :)

> 
> If we touch these old_-tables like this and destroy this archived data that
> can be linked still with deleted-tables, shouldn't we do the same to
> action_logs? I consider action_logs as an "archive" table just like
> old_issues or old_reserves (not sure if we do this already, though. Remove
> itemnumbers/biblionumbers from action_logs that are linked to deleted
> items/biblios?).

Good point. There is room for further improvement indeed. But no reason to switch good to less.
Comment 14 Andrew Fuerste-Henry 2024-03-18 13:36:25 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Lari Strand from comment #0)
> > Unless there's a good reason for this data loss, the itemnumber should be
> > preserved in old_reserves after deleting an item.
> 
> Please explain why there is a good reason to do so. Keeping an itemnumber in
> this table that does no longer exist is bad practice.

The itemnumber in the reserves table may refer to an item in the items table or may refer to an item in the deleteditems table. The [table]/deleted[table] structure for items and bibs supports the fundamental idea that a library may need to know about titles/items that were once in their collection but are not any longer. Even after an item has been removed from the collection, it's still useful to know which holds and checkouts involved that item.
Comment 15 paxed 2024-03-19 07:57:06 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Lari Strand from comment #0)
> > Unless there's a good reason for this data loss, the itemnumber should be
> > preserved in old_reserves after deleting an item.
> 
> Please explain why there is a good reason to do so. Keeping an itemnumber in
> this table that does no longer exist is bad practice.

The items may not exist physically in the library, but the data certainly does in the deleteditems/deletdbiblio/whatever tables.
Comment 16 Katrin Fischer 2024-03-19 08:46:30 UTC
Hi, I agree with not removing the information about the linked item even if it was deleted, for the reasons given. But maybe we can find some middle ground here with the implementation. Have you seen the discussion on bug 36345? It suggests the fix we have recently implemented for the biblionumber in aqorders.
Comment 17 Lari Strand 2024-03-19 09:57:52 UTC
> Dont you think that we should adhere to proven standards as to SQL,
> normalization, etc ? :)

I think we should adhere to the needs of the users who use the library system. We only gain more functionality in the system by not touching the data (use cases reported in both tickets concerning old_-tables versus following "good practice" for no gains or benefits whatsoever (are there really standards for this to follow? What is the good practice concerning archived data?). Anyways, feels more like bad practice in my mind to touch these tables when deleting stuff.
Comment 18 Lucas Gass 2024-03-19 20:03:48 UTC
> Dont you think that we should adhere to proven standards as to SQL,
> normalization, etc ? :)

We should keep the data that librarians want to keep. If the problem is a FK constraint then that FK  constraint should go, not the data librarians need.