Description
Andrew Isherwood
2018-12-11 08:12:00 UTC
Created attachment 83226 [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 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 Please rebase on master. 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. (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. 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 :) (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. 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. 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. 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. 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. (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 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. 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. Created attachment 152006 [details] [review] Bug 21983: DBIC schema [DO NOT PUSH] 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! 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 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> Created attachment 152011 [details] [review] Bug 21983: DBIC schema [DO NOT PUSH] Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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> 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. I'll try to get to this ASAP, but might not make it today. If someone can take over, please do! 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> 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> 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> 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> 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> 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> Pushed to master for 23.11. Nice work everyone, thanks! Nice work everyone! Pushed to oldstable for 22.11.x 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 (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. (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 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. 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 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). I have the same problem 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. 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. (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 Look at Bug 34276 |