The patron hold history page shows "No title" for old holds on records that have since been deleted. We should get the title from deletedbibios if possible.
Impossible, we lost the link. We need bug 25260.
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.