Bug 21983 - Better handling of deleted biblios on ILL requests
Summary: Better handling of deleted biblios on ILL requests
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ILL (show other bugs)
Version: 23.05
Hardware: All All
: P5 - low blocker (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on: 22440
Blocks: 5334 34276
  Show dependency treegraph
 
Reported: 2018-12-11 08:12 UTC by Andrew Isherwood
Modified: 2024-04-05 15:48 UTC (History)
16 users (show)

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


Attachments
Bug 21983: Add foreign key constraint on biblio_id (3.10 KB, patch)
2018-12-14 16:11 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21983: Add foreign key constraint on biblio_id (3.13 KB, patch)
2018-12-14 16:13 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 21983: DB update (4.77 KB, patch)
2023-06-05 17:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21983: DBIC schema [DO NOT PUSH] (1.43 KB, patch)
2023-06-05 17:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21983: Add Koha::Biblio->ill_requests (3.01 KB, patch)
2023-06-05 17:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21983: Make DelBiblio update linked ILL requests (3.13 KB, patch)
2023-06-05 17:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 21983: DB update (4.83 KB, patch)
2023-06-05 17:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21983: DBIC schema [DO NOT PUSH] (1.49 KB, patch)
2023-06-05 17:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21983: Add Koha::Biblio->ill_requests (3.07 KB, patch)
2023-06-05 17:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21983: Make DelBiblio update linked ILL requests (3.19 KB, patch)
2023-06-05 17:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21983: DB update (4.88 KB, patch)
2023-06-06 16:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21983: DB update (4.88 KB, text/plain)
2023-06-06 16:59 UTC, Kyle M Hall
Details
Bug 21983: DBIC schema [DO NOT PUSH] (1.55 KB, patch)
2023-06-06 17:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21983: Add Koha::Biblio->ill_requests (3.12 KB, patch)
2023-06-06 17:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21983: Make DelBiblio update linked ILL requests (3.25 KB, patch)
2023-06-06 17:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 21983: (QA follow-up) Skip deleted_biblio_id from the API object (1.89 KB, patch)
2023-06-06 18:25 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2018-12-11 08:12:00 UTC
illrequests.biblio_id should be a foreign key to biblio.biblionumber, but it's not! When a biblio is deleted, illrequests.biblio_id should be null-ed
Comment 1 Andrew Isherwood 2018-12-14 16:11:03 UTC Comment hidden (obsolete)
Comment 2 Andrew Isherwood 2018-12-14 16:13:15 UTC
Created attachment 83227 [details] [review]
Bug 21983: Add foreign key constraint on biblio_id

This patch adds a foreign key constraint on biblio_id, this means that,
amongst other things, if a bib is deleted that an ILL request is a
attached to, the ILL request is no longer attached to that bib.

Test plan:
- Apply the patch
- Create an ILL request, a bib should automatically be created for the
item being requested
- Note that when viewing the request, a link to the bib is displayed in
the ILL request list view
- Delete the bib
- Refresh the ILL request list view
- TEST => Note that the link to the bib no longer exists
Comment 3 Michal Denar 2019-03-15 14:14:22 UTC
Please rebase on master.
Comment 4 Katrin Fischer 2019-03-16 09:16:53 UTC
I am not sure about this one, as we have deleted_biblio that is often useful for reporting. I imagine some libraries will want to remove ILL records from the catalog once the request is finished, but might still want to have the bibliographic information for reporting.
Comment 5 Magnus Enger 2019-03-16 10:57:09 UTC
(In reply to Katrin Fischer from comment #4)
> I am not sure about this one, as we have deleted_biblio that is often useful
> for reporting. I imagine some libraries will want to remove ILL records from
> the catalog once the request is finished, but might still want to have the
> bibliographic information for reporting.

I can confirm that this is indeed very true, yes.
Comment 6 Andrew Isherwood 2019-03-18 09:35:19 UTC
But it's just a foreign key to the (now deleted) bib ID, is that useful information that needs preserving? Sorry, genuine question, I'm not being facetious :)
Comment 7 Katrin Fischer 2019-11-24 21:46:00 UTC
(In reply to Andrew Isherwood from comment #6)
> But it's just a foreign key to the (now deleted) bib ID, is that useful
> information that needs preserving? Sorry, genuine question, I'm not being
> facetious :)

As long as the bilbio_id is there, you can join on the deleted* tables and get information about the records that were requested. This is interesting information to libraries. Example: if a record was requested multiple times, they might to purchase it. If records from a certain subject are ordered more than other subjects, they might want to shift the budget in this direction, etc.
Comment 8 Katrin Fischer 2019-11-24 21:47:07 UTC
I think Martin solved this somewhere else by having a separate column for the deleted ID as this will allow to have cleaner FK constraints, but I don't quite remember the bug report at the moment.
Comment 9 Katrin Fischer 2023-01-13 10:35:59 UTC
I still think we want to preserve the biblionumber, I am open to ideas on the best way of doing it.

It probably is more useful for backends that import a full MARC record from another ILL, as this will have more information than the request itself.

But for all backends the biblionumber als provides a way to find the item linked to it... and then you can look up in statistics, issues, old_issues etc. 

So even if you have a 'slim' record, it's something we'd like to keep.
Comment 10 Tomás Cohen Arazi 2023-06-05 12:34:55 UTC
Should we add a deleted_biblio_id attribute? or column? I'd say an attribute.

We have the same problem with borrowernumber, but worse, with a DELETE.
Comment 11 Katrin Fischer 2023-06-05 12:58:01 UTC
as biblio_id is in the illrequests I think it would make things easier to have the deleted one there as well.
Same for borrowernumber, it makes report writing much easier than a subquery to illrequestattributes.
Comment 12 Katrin Fischer 2023-06-05 12:59:05 UTC
(In reply to Katrin Fischer from comment #11)
> as biblio_id is in the illrequests I think it would make things easier to
> have the deleted one there as well.
> Same for borrowernumber, it makes report writing much easier than a subquery
> to illrequestattributes.

There is also:
Bug 32630 - Don't delete ILL requests when patron is deleted
Comment 13 Tomás Cohen Arazi 2023-06-05 15:14:14 UTC
I'm working on this. The plan:

- Add illrequests.deleted_biblio_id
- Make the db_rev move the biblio_id to deleted_biblio_id where needed
- Make Koha::Biblio->delete deal with that change as well.
Comment 14 Tomás Cohen Arazi 2023-06-05 17:01:45 UTC
Created attachment 152005 [details] [review]
Bug 21983: DB update

This patch:
- Adds a illrequests.deleted_biblio_id column
- Adjusts existing db_rev so people upgrading have the biblio_id value
  moved to the also created column, before attempting to add the FK
  constraint on the biblios table.
- Adds a new db_rev for those who unfortunately already updated, it
  should be idempotent.
Comment 15 Tomás Cohen Arazi 2023-06-05 17:01:48 UTC
Created attachment 152006 [details] [review]
Bug 21983: DBIC schema [DO NOT PUSH]
Comment 16 Tomás Cohen Arazi 2023-06-05 17:01:51 UTC
Created attachment 152007 [details] [review]
Bug 21983: Add Koha::Biblio->ill_requests

This patch adds a new method, used for retrieving the linked ill
requests for a biblio.

To test:
1. Apply this patch and run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!
Comment 17 Tomás Cohen Arazi 2023-06-05 17:01:54 UTC
Created attachment 152008 [details] [review]
Bug 21983: Make DelBiblio update linked ILL requests

This patch makes DelBiblio update the biblio linked ILL requests so the
value in biblio_id is moved to the deleted_biblio_id.

The change is covered by tests.

To test:
1. Apply this patchset
2. Run:
   $ ktd --shell
  k$ updatedatabase
  k$ qa --run-tests
=> SUCCESS: Tests pass! All green/good
3. Sign off :-D
Comment 18 Martin Renvoize 2023-06-05 17:40:21 UTC
Created attachment 152010 [details] [review]
Bug 21983: DB update

This patch:
- Adds a illrequests.deleted_biblio_id column
- Adjusts existing db_rev so people upgrading have the biblio_id value
  moved to the also created column, before attempting to add the FK
  constraint on the biblios table.
- Adds a new db_rev for those who unfortunately already updated, it
  should be idempotent.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2023-06-05 17:40:24 UTC
Created attachment 152011 [details] [review]
Bug 21983: DBIC schema [DO NOT PUSH]

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2023-06-05 17:40:27 UTC
Created attachment 152012 [details] [review]
Bug 21983: Add Koha::Biblio->ill_requests

This patch adds a new method, used for retrieving the linked ill
requests for a biblio.

To test:
1. Apply this patch and run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2023-06-05 17:40:30 UTC
Created attachment 152013 [details] [review]
Bug 21983: Make DelBiblio update linked ILL requests

This patch makes DelBiblio update the biblio linked ILL requests so the
value in biblio_id is moved to the deleted_biblio_id.

The change is covered by tests.

To test:
1. Apply this patchset
2. Run:
   $ ktd --shell
  k$ updatedatabase
  k$ qa --run-tests
=> SUCCESS: Tests pass! All green/good
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-06-05 17:42:38 UTC
Makes sense to me and keeps the data in a clean form whilst allowing us foreign keys.

We could perhaps make deleted_biblio_id a foreign key on the deletedbiblios table too.. 

Signing off and leaving Katrin to QA and comment on the above question.
Comment 23 Katrin Fischer 2023-06-06 11:40:19 UTC
I'll try to get to this ASAP, but might not make it today. If someone can take over, please do!
Comment 24 Kyle M Hall 2023-06-06 16:56:08 UTC
Created attachment 152056 [details] [review]
Bug 21983: DB update

This patch:
- Adds a illrequests.deleted_biblio_id column
- Adjusts existing db_rev so people upgrading have the biblio_id value
  moved to the also created column, before attempting to add the FK
  constraint on the biblios table.
- Adds a new db_rev for those who unfortunately already updated, it
  should be idempotent.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2023-06-06 16:59:57 UTC
Created attachment 152057 [details]
Bug 21983: DB update

This patch:
- Adds a illrequests.deleted_biblio_id column
- Adjusts existing db_rev so people upgrading have the biblio_id value
  moved to the also created column, before attempting to add the FK
  constraint on the biblios table.
- Adds a new db_rev for those who unfortunately already updated, it
  should be idempotent.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2023-06-06 17:00:04 UTC
Created attachment 152058 [details] [review]
Bug 21983: DBIC schema [DO NOT PUSH]

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Kyle M Hall 2023-06-06 17:00:07 UTC
Created attachment 152059 [details] [review]
Bug 21983: Add Koha::Biblio->ill_requests

This patch adds a new method, used for retrieving the linked ill
requests for a biblio.

To test:
1. Apply this patch and run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2023-06-06 17:00:09 UTC
Created attachment 152060 [details] [review]
Bug 21983: Make DelBiblio update linked ILL requests

This patch makes DelBiblio update the biblio linked ILL requests so the
value in biblio_id is moved to the deleted_biblio_id.

The change is covered by tests.

To test:
1. Apply this patchset
2. Run:
   $ ktd --shell
  k$ updatedatabase
  k$ qa --run-tests
=> SUCCESS: Tests pass! All green/good
3. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2023-06-06 18:25:42 UTC
Created attachment 152063 [details] [review]
Bug 21983: (QA follow-up) Skip deleted_biblio_id from the API object

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2023-06-06 18:31:28 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 31 Matt Blenkinsop 2023-06-12 11:17:02 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x
Comment 32 Alex Mwithi 2023-07-19 08:29:31 UTC
How do i solve this error

Update errors :

    Upgrade to 23.05.00.002 [08:24:59]: Bug 21983 - Deleted biblio handling on ILL
    ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741
Comment 33 Pedro Amorim 2023-07-19 08:32:41 UTC
(In reply to Alex Mwithi from comment #32)
> How do i solve this error
> 
> Update errors :
> 
>     Upgrade to 23.05.00.002 [08:24:59]: Bug 21983 - Deleted biblio handling
> on ILL
>     ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key
> name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741

Alex, which version are you upgrading from?
See bug 34276.
Comment 34 Alex Mwithi 2023-07-19 08:57:32 UTC
(In reply to Pedro Amorim from comment #33)
> (In reply to Alex Mwithi from comment #32)
> > How do i solve this error
> > 
> > Update errors :
> > 
> >     Upgrade to 23.05.00.002 [08:24:59]: Bug 21983 - Deleted biblio handling
> > on ILL
> >     ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key
> > name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741
> 
> Alex, which version are you upgrading from?
> See bug 34276.

i'm upgrading from Koha 22.11.07
Comment 35 Pedro Amorim 2023-07-19 09:34:38 UTC
This is very weird.
Did a new test:
1) git checkout v22.11.07
2) reset_all
3) git checkout v23.05.01
4) updatedatabase

Everything ran smoothly (including this bug 21983 DB update). I'm unable to reproduce this duplicate key error.
Comment 36 Jeremiah 2023-07-20 06:58:56 UTC
Hello,

I am experiencing the same issue as Alex
"Upgrade to 23.05.00.002 [23:52:55]: Bug 21983 - Deleted biblio handling on ILL
ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741"

Upgrading from 22.05 to 23.05
Comment 37 Jonathan Druart 2023-07-20 07:32:53 UTC
The history of illrequests_bibfk chaotic, I didn't find how this situation could happen but regarding the different reports we should add a separate check for the key (one for the FK, one for the key).
Comment 38 José Anjos 2023-07-31 13:53:43 UTC
I have the same problem
Comment 39 José Anjos 2023-07-31 15:33:59 UTC
I had to delete Relation illrequests_ibfk_1 to be able to delete Index illrequests_bibfk

Than, continue setup and the Relation and the Index was recreated.
Comment 40 tony-the-hawk 2023-08-30 13:21:50 UTC
I have same problem

Upgrade to 23.05.00.002 [14:53:05]: Bug 21983 - Deleted biblio handling on ILL
ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741

but I can't find illrequests_bibfk in any table of my database.
Comment 41 José Anjos 2023-08-30 14:16:21 UTC
(In reply to tony-the-hawk from comment #40)
> I have same problem
> 
> Upgrade to 23.05.00.002 [14:53:05]: Bug 21983 - Deleted biblio handling on
> ILL
> ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Duplicate key
> name 'illrequests_bibfk' at /usr/share/koha/lib/C4/Installer.pm line 741
> 
> but I can't find illrequests_bibfk in any table of my database.

Table: illrequests
Relations: illrequests_ibfk_1
Index: illrequests_bibfk
Comment 42 Fridolin Somers 2023-08-31 00:59:34 UTC
Look at Bug 34276