|
Description
Andrew Fuerste-Henry
2022-04-28 14:59:50 UTC
I have confirmed it. In old_reserves the biblionumber is set to NULL when the bibliographic record is deleted. I know that merging the tables is the ultimate solution, but is there another solution we could think of? Maybe we could not cascade on delete? In the meantime I think we need an update to the holds history page in the OPAC to exclude deleted titles. Created attachment 172687 [details] [review] Bug 30648: Add deleted_biblionumber column for reserves Created attachment 172688 [details] [review] Bug 30648: DO NOT PUSH - Schema update Created attachment 172689 [details] [review] Bug 30648: Record deleted_biblionumber in holds This patch ensures the deleted biblionumber is recorded in current and previous holds before the record is deleted To test: 1 - Place and fill a few holds on a biblio, completing checkout to patron 2 - Place a few holds on the biblio and do not fulfill them 3 - Delete the items from the biblio via the DB (otherwise you cannot delete last item when there are holds) DELETE FROM items WHERE biblionumber={biblionumber} 4 - Delete the biblio 5 - Check the DB and confirm the deleted_biblionumber column has been filled SELECT reserve_id,deleted_biblionumber,biblionumber FROM old_reserves WHERE deleted_biblionumber = {biblionumber}; 6 - Sign off! Created attachment 172690 [details] [review] Bug 30648: Unit tests Created attachment 172691 [details] [review] Bug 30648: Add new field to REST API Maybe we just undef it since ti will always be null except for old_hold obecjts and we don't have that route yet Created attachment 172692 [details] [review] Bug 30648: Tidy Do we abandon bug 25260 then? (In reply to Jonathan Druart from comment #10) > Do we abandon bug 25260 then? I don't think we need to: keeping the deleted biblionumber is not the same problem as merging old_reserves and reserves. It would still be needed with the merge. Created attachment 173340 [details] [review] Bug 30648: Add deleted_biblionumber column for reserves Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173341 [details] [review] Bug 30648: DO NOT PUSH - Schema update Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173342 [details] [review] Bug 30648: Record deleted_biblionumber in holds This patch ensures the deleted biblionumber is recorded in current and previous holds before the record is deleted To test: 1 - Place and fill a few holds on a biblio, completing checkout to patron 2 - Place a few holds on the biblio and do not fulfill them 3 - Delete the items from the biblio via the DB (otherwise you cannot delete last item when there are holds) DELETE FROM items WHERE biblionumber={biblionumber} 4 - Delete the biblio 5 - Check the DB and confirm the deleted_biblionumber column has been filled SELECT reserve_id,deleted_biblionumber,biblionumber FROM old_reserves WHERE deleted_biblionumber = {biblionumber}; 6 - Sign off! Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173343 [details] [review] Bug 30648: Unit tests Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173344 [details] [review] Bug 30648: Add new field to REST API Maybe we just undef it since ti will always be null except for old_hold obecjts and we don't have that route yet Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173345 [details] [review] Bug 30648: Tidy Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Created attachment 173491 [details] [review] Bug 30648: Add deleted_biblionumber column for reserves Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173492 [details] [review] Bug 30648: DO NOT PUSH - Schema update Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173493 [details] [review] Bug 30648: Record deleted_biblionumber in holds This patch ensures the deleted biblionumber is recorded in current and previous holds before the record is deleted To test: 1 - Place and fill a few holds on a biblio, completing checkout to patron 2 - Place a few holds on the biblio and do not fulfill them 3 - Delete the items from the biblio via the DB (otherwise you cannot delete last item when there are holds) DELETE FROM items WHERE biblionumber={biblionumber} 4 - Delete the biblio 5 - Check the DB and confirm the deleted_biblionumber column has been filled SELECT reserve_id,deleted_biblionumber,biblionumber FROM old_reserves WHERE deleted_biblionumber = {biblionumber}; 6 - Sign off! Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173494 [details] [review] Bug 30648: Unit tests Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173495 [details] [review] Bug 30648: Add new field to REST API Maybe we just undef it since ti will always be null except for old_hold obecjts and we don't have that route yet Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 173496 [details] [review] Bug 30648: Tidy Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Small stuff: 1) Database update * Please use say_success But overall: happy to see this fix :) Do we have a bug for issues and old_issues already and circulation history? Pushed for 24.11! Well done everyone, thank you! API tests broken by this:
POST /api/v1/holds [{"message":"Expected integer - got null.","path":"\/body\/deleted_biblio_id"}]
t/db_dependent/api/v1/biblios.t
t/db_dependent/api/v1/patrons_holds.t
t/db_dependent/api/v1/holds.t
Koha_Main/3078
(In reply to Jonathan Druart from comment #27) > API tests broken by this: > > POST /api/v1/holds [{"message":"Expected integer - got > null.","path":"\/body\/deleted_biblio_id"}] > > t/db_dependent/api/v1/biblios.t > t/db_dependent/api/v1/patrons_holds.t > t/db_dependent/api/v1/holds.t > > > Koha_Main/3078 Joubu is always a step ahead of me, I was still typing this out. Please fix urgently! Created attachment 173611 [details] [review] Bug 30648: deleted_biblio_id can be null (In reply to Jonathan Druart from comment #29) > Created attachment 173611 [details] [review] [review] > Bug 30648: deleted_biblio_id can be null t/db_dependent/api/v1/biblios.t still failing after this patch. Tests are missing for Koha/Biblio.pm:sub old_holds {
I'll grab the first follow-up here for the next push already. Suspecting this is related here: > prove t/db_dependent/api/v1/holds.t > t/db_dependent/api/v1/holds.t .. 1/14 No reserves HOLD_CANCELLATION > letter transported by email at /kohadevbox/koha/C4/Letters.pm line 610. > t/db_dependent/api/v1/holds.t .. 12/14 # Looks like you planned 37 tests > but ran 3. > t/db_dependent/api/v1/holds.t .. 13/14 > # Failed test 'PUT /holds/{hold_id}/pickup_location tests' > # at t/db_dependent/api/v1/holds.t line 1470. > Can't locate object method "branchcode" via package "euRljh" (perhaps > you forgot to load "euRljh"?) at t/db_dependent/api/v1/holds.t line 1377. > # Looks like your test exited with 11 just after 13. > t/db_dependent/api/v1/holds.t .. Dubious, test returned 11 (wstat > 2816, > 0xb00) > Failed 2/14 subtests There is a comment on bug 30661 that might help fix: (In reply to Johanna Räisä from comment #30) > I didn't get that HOLD_CANCELLATION error after I pulled main repo. The > subtest "PUT /holds/{hold_id}/pickup_location tests" fails in holds.t > > There are some interesting branchcode fetching in "PUT > /holds/{hold_id}/pickup_location tests" at least: > $hold->discard_changes->branchcode->branchcode > If those second branchcode values are removed the tests pass in that subtest. We got a fix for this as follow-up on bug 38273 now! DB revs: say_success statements must be inside the if ! column_exists blocks. |