Bug 30648 - Title is lost in holds history when bibliographic record is deleted
Summary: Title is lost in holds history when bibliographic record is deleted
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks: 36345 38395
  Show dependency treegraph
 
Reported: 2022-04-28 14:59 UTC by Andrew Fuerste-Henry
Modified: 2025-02-12 13:57 UTC (History)
11 users (show)

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


Attachments
Bug 30648: Add deleted_biblionumber column for reserves (3.98 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: DO NOT PUSH - Schema update (2.82 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: Record deleted_biblionumber in holds (2.45 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: Unit tests (2.69 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: Add new field to REST API (2.85 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: Tidy (3.36 KB, patch)
2024-10-11 13:13 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30648: Add deleted_biblionumber column for reserves (4.13 KB, patch)
2024-10-25 10:54 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: DO NOT PUSH - Schema update (2.96 KB, patch)
2024-10-25 10:55 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: Record deleted_biblionumber in holds (2.59 KB, patch)
2024-10-25 10:55 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: Unit tests (2.84 KB, patch)
2024-10-25 10:55 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: Add new field to REST API (3.00 KB, patch)
2024-10-25 10:55 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: Tidy (3.50 KB, patch)
2024-10-25 10:55 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 30648: Add deleted_biblionumber column for reserves (4.20 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: DO NOT PUSH - Schema update (3.03 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: Record deleted_biblionumber in holds (2.66 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: Unit tests (2.91 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: Add new field to REST API (3.07 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: Tidy (3.57 KB, patch)
2024-10-28 10:31 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30648: deleted_biblio_id can be null (816 bytes, patch)
2024-10-29 10:34 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2022-04-28 14:59:50 UTC
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.
Comment 1 Jonathan Druart 2022-06-08 09:25:55 UTC
Impossible, we lost the link. We need bug 25260.
Comment 2 Katrin Fischer 2022-07-30 22:23:23 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?
Comment 3 Owen Leonard 2022-08-29 17:03:24 UTC
In the meantime I think we need an update to the holds history page in the OPAC to exclude deleted titles.
Comment 4 Nick Clemens (kidclamp) 2024-10-11 13:13:30 UTC
Created attachment 172687 [details] [review]
Bug 30648: Add deleted_biblionumber column for reserves
Comment 5 Nick Clemens (kidclamp) 2024-10-11 13:13:32 UTC
Created attachment 172688 [details] [review]
Bug 30648: DO NOT PUSH - Schema update
Comment 6 Nick Clemens (kidclamp) 2024-10-11 13:13:34 UTC
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!
Comment 7 Nick Clemens (kidclamp) 2024-10-11 13:13:36 UTC
Created attachment 172690 [details] [review]
Bug 30648: Unit tests
Comment 8 Nick Clemens (kidclamp) 2024-10-11 13:13:38 UTC
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
Comment 9 Nick Clemens (kidclamp) 2024-10-11 13:13:40 UTC
Created attachment 172692 [details] [review]
Bug 30648: Tidy
Comment 10 Jonathan Druart 2024-10-15 11:44:41 UTC
Do we abandon bug 25260 then?
Comment 11 Katrin Fischer 2024-10-15 12:44:30 UTC
(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.
Comment 12 Biblibre Sandboxes 2024-10-25 10:54:59 UTC
Created attachment 173340 [details] [review]
Bug 30648: Add deleted_biblionumber column for reserves

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 13 Biblibre Sandboxes 2024-10-25 10:55:02 UTC
Created attachment 173341 [details] [review]
Bug 30648: DO NOT PUSH - Schema update

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 14 Biblibre Sandboxes 2024-10-25 10:55:04 UTC
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>
Comment 15 Biblibre Sandboxes 2024-10-25 10:55:07 UTC
Created attachment 173343 [details] [review]
Bug 30648: Unit tests

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 16 Biblibre Sandboxes 2024-10-25 10:55:09 UTC
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>
Comment 17 Biblibre Sandboxes 2024-10-25 10:55:12 UTC
Created attachment 173345 [details] [review]
Bug 30648: Tidy

Signed-off-by: Anneli Österman <anneli.osterman@koha-suomi.fi>
Comment 18 Martin Renvoize (ashimema) 2024-10-28 10:31:19 UTC
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>
Comment 19 Martin Renvoize (ashimema) 2024-10-28 10:31:21 UTC
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>
Comment 20 Martin Renvoize (ashimema) 2024-10-28 10:31:24 UTC
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>
Comment 21 Martin Renvoize (ashimema) 2024-10-28 10:31:27 UTC
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>
Comment 22 Martin Renvoize (ashimema) 2024-10-28 10:31:30 UTC
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>
Comment 23 Martin Renvoize (ashimema) 2024-10-28 10:31:32 UTC
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>
Comment 24 Katrin Fischer 2024-10-28 18:48:49 UTC
Small stuff:

1) Database update

* Please use say_success
Comment 25 Katrin Fischer 2024-10-28 18:57:27 UTC
But overall: happy to see this fix :)

Do we have a bug for issues and old_issues already and circulation history?
Comment 26 Katrin Fischer 2024-10-29 07:54:01 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 27 Jonathan Druart 2024-10-29 10:11:30 UTC
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
Comment 28 Katrin Fischer 2024-10-29 10:15:52 UTC
(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!
Comment 29 Jonathan Druart 2024-10-29 10:34:28 UTC
Created attachment 173611 [details] [review]
Bug 30648: deleted_biblio_id can be null
Comment 30 Jonathan Druart 2024-10-29 10:36:46 UTC
(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.
Comment 31 Jonathan Druart 2024-10-29 10:41:36 UTC
Tests are missing for Koha/Biblio.pm:sub old_holds {
Comment 32 Katrin Fischer 2024-10-29 10:47:05 UTC
I'll grab the first follow-up here for the next push already.
Comment 33 Katrin Fischer 2024-10-30 06:56:20 UTC
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.
Comment 34 Katrin Fischer 2024-10-30 10:54:40 UTC
We got a fix for this as follow-up on bug 38273 now!
Comment 35 Jonathan Druart 2024-11-07 15:04:01 UTC
DB revs: say_success statements must be inside the if ! column_exists blocks.
Comment 36 Lucas Gass (lukeg) 2024-11-22 21:12:02 UTC
I see a commit in main that are not part of this bug report. What am I missing?


9c2ec5798b9f1062b15433554c33e9089460c2cf Bug 30648: (QA follow-up) Terminology: reserve => hold
Comment 37 Katrin Fischer 2024-11-22 21:20:12 UTC
(In reply to Lucas Gass (lukeg) from comment #36)
> I see a commit in main that are not part of this bug report. What am I
> missing?
> 
> 
> 9c2ec5798b9f1062b15433554c33e9089460c2cf Bug 30648: (QA follow-up)
> Terminology: reserve => hold

I didn't always attach follow-ups to the bug reports assuming RMaints would cherry-pick via git anyway.
Comment 38 Lucas Gass (lukeg) 2024-12-05 16:48:40 UTC
BAckported to 24.05.x for upcoming 24.05.06
Comment 39 Fridolin Somers 2024-12-13 15:39:24 UTC
(In reply to Katrin Fischer from comment #25)
> But overall: happy to see this fix :)
> 
> Do we have a bug for issues and old_issues already and circulation history?

I have the same question.
Whould be great to have consistency between holds history and issues history.
Although old_issue has itemnumber and not biblionumber.
Comment 40 Fridolin Somers 2024-12-13 15:39:55 UTC
I'm not sure about backporting to 23.11.x for stablity
I first wait for Bug 38395
Comment 41 Andrew Fuerste-Henry 2025-02-12 13:57:58 UTC
Noting for future readers that these patches got koha to retain the biblionumber of the deleted record but did not make koha use that retained number to display anything. See bug 38395 for that.