Bug 33262 - When an ordered record is deleted, we lose all information on what was ordered
Summary: When an ordered record is deleted, we lose all information on what was ordered
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low blocker (vote)
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10758 10869
  Show dependency treegraph
 
Reported: 2023-03-17 10:56 UTC by Katrin Fischer
Modified: 2024-03-18 14:09 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.06,22.05.13


Attachments
Bug 33262: Add deleted_biblionumber column to aqorders table (2.52 KB, patch)
2023-05-04 11:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Add deleted_biblionumber column to aqorders table (3.96 KB, patch)
2023-05-04 13:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: [DO NOT PUSH] Schema update (1.27 KB, patch)
2023-05-04 13:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Store the biblionumber of a deleted record in the order line (2.28 KB, patch)
2023-05-04 13:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Unit tests (1.66 KB, patch)
2023-05-04 13:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Add deleted_biblionumber column to aqorders table (2.50 KB, patch)
2023-05-04 13:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: [DO NOT PUSH) Schema update (1.70 KB, patch)
2023-05-04 13:28 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Store the biblionumber of a deleted record in the order line (2.22 KB, patch)
2023-05-04 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Unit tests (1.67 KB, patch)
2023-05-04 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Add deleted_biblionumber column to aqorders table (2.50 KB, patch)
2023-05-04 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: [DO NOT PUSH) Schema update (1.70 KB, patch)
2023-05-04 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Store the biblionumber of a deleted record in the order line (2.22 KB, patch)
2023-05-04 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Unit tests (1.67 KB, patch)
2023-05-04 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Add new field to REST API route for orders (1.64 KB, patch)
2023-05-04 15:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33262: Add deleted_biblionumber column to aqorders table (2.54 KB, patch)
2023-05-05 00:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33262: [DO NOT PUSH) Schema update (1.75 KB, patch)
2023-05-05 00:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33262: Store the biblionumber of a deleted record in the order line (2.27 KB, patch)
2023-05-05 00:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33262: Unit tests (1.71 KB, patch)
2023-05-05 00:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33262: Add new field to REST API route for orders (1.68 KB, patch)
2023-05-05 00:46 UTC, David Nind
Details | Diff | Splinter Review
Bug 33262: Add deleted_biblionumber column to aqorders table (2.60 KB, patch)
2023-05-05 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33262: [DO NOT PUSH) Schema update (1.81 KB, patch)
2023-05-05 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33262: Store the biblionumber of a deleted record in the order line (2.32 KB, patch)
2023-05-05 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33262: Unit tests (1.77 KB, patch)
2023-05-05 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33262: Add new field to REST API route for orders (1.74 KB, patch)
2023-05-05 12:04 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2023-03-17 10:56:25 UTC
I am moving this out from bug 10758 that was filed for the display issue. But that is actually only the symptom, while we are actually facing a data loss issue.

Libraries need to be able to account for the money they spent even after the items and records have been removed from their collection. 

At the moment we lose the connection between order and bibliographic record when the record is deleted: 

--
-- Table structure for table `aqorders`
--

DROP TABLE IF EXISTS `aqorders`;
/*!40101 SET @saved_cs_client     = @@character_set_client */;
/*!40101 SET character_set_client = utf8 */;
CREATE TABLE `aqorders` (
  `ordernumber` int(11) NOT NULL AUTO_INCREMENT COMMENT 'primary key and unique identifier assigned by Koha to each line',
  `biblionumber` int(11) DEFAULT NULL COMMENT 'links the order to the biblio being ordered (biblio.biblionumber)',
[...]
  CONSTRAINT `aqorders_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE SET NULL ON UPDATE CASCADE,


We do have the information about the deleted record and the items, but we need to preserve the link between order and deleted record in some way.
Comment 1 Jonathan Druart 2023-03-22 14:05:39 UTC
What are you suggesting?

The only way to do it correctly is bug 20271...
Comment 2 Katrin Fischer 2023-03-22 15:23:55 UTC
(In reply to Jonathan Druart from comment #1)
> What are you suggesting?
> 
> The only way to do it correctly is bug 20271...

We have waited for this for years now. It's a big data issue, I think it's time to look for alternative solutions. 

We had some ideas, but I'd really like to get some people together to discuss this next week:

* Remove the FK for now
* Add a separate column to store the deleted biblionumber to aqorders
* Add a separate column for title information to aqorders. Maybe using a TT template for configuring/how what to store.
Comment 3 Jonathan Druart 2023-05-04 06:44:13 UTC
We should lower the priority here, it's not a blocker and it's a long standing bug.
Comment 4 Katrin Fischer 2023-05-04 08:42:11 UTC
blocker is defined as data loss. I hope to provide a patch before freezes.
Comment 5 Katrin Fischer 2023-05-04 11:16:11 UTC
Created attachment 150647 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.

Test plan in last patch.
Comment 6 Katrin Fischer 2023-05-04 13:12:16 UTC
Created attachment 150657 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.
Comment 7 Katrin Fischer 2023-05-04 13:12:19 UTC
Created attachment 150658 [details] [review]
Bug 33262: [DO NOT PUSH] Schema update
Comment 8 Katrin Fischer 2023-05-04 13:12:21 UTC
Created attachment 150659 [details] [review]
Bug 33262: Store the biblionumber of a deleted record in the order line

When a bibliographic record is deleted and linked to an order
in the acquisition module, the biblionumber in the order line is
currently deleted as well. This makes it impossible to tell what
was ordered in the first place. This is a big issue for libraries
that need to be able to track how money was spent over long periods
of time.

With this patch the biblionumber of the deleted record will be moved
to a new column deleted_biblionumber in the order line, keeping the FK
on biblionumber.

To test:
* Apply patch, run dataase update
* Create a order/basket in the acquisitions module
* Order several things, you can use existing records from the sample data
* Make sure you order one of the records at least twice
* Take a look at the aqorders table in your database, using a report:

  SELECT biblionumber, deleted_biblionumber, ordernumber from aqorders;

* From the catalog, delete the items on the records and then the records

  Note: There is a warning about attached orders warning about consequences
  for acquisitions. I have chosen to keep it at this point in time, as
  there are still visible side effects of deleting the record, like bug 10758.

* Re-run the report and compare results to the first run. deleted_biblionumber
  should now be filled with the biblionumber of the deleted record.
  biblionumber should be NULL/empty.
Comment 9 Katrin Fischer 2023-05-04 13:12:24 UTC
Created attachment 150660 [details] [review]
Bug 33262: Unit tests

prove t/db_dependent/Biblio.t
Comment 10 Katrin Fischer 2023-05-04 13:28:56 UTC
Created attachment 150661 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.
Comment 11 Katrin Fischer 2023-05-04 13:28:59 UTC
Created attachment 150662 [details] [review]
Bug 33262: [DO NOT PUSH) Schema update
Comment 12 Katrin Fischer 2023-05-04 13:29:02 UTC
Created attachment 150663 [details] [review]
Bug 33262: Store the biblionumber of a deleted record in the order line

When a bibliographic record is deleted and linked to an order
in the acquisition module, the biblionumber in the order line is
currently deleted as well. This makes it impossible to tell what
was ordered in the first place. This is a big issue for libraries
that need to be able to track how money was spent over long periods
of time.

With this patch the biblionumber of the deleted record will be moved
to a new column deleted_biblionumber in the order line, keeping the FK
on biblionumber.

To test:
* Apply patch, run dataase update
* Create a order/basket in the acquisitions module
* Order several things, you can use existing records from the sample data
* Make sure you order one of the records at least twice
* Take a look at the aqorders table in your database, using a report:

  SELECT biblionumber, deleted_biblionumber, ordernumber from aqorders;

* From the catalog, delete the items on the records and then the records

  Note: There is a warning about attached orders warning about consequences
  for acquisitions. I have chosen to keep it at this point in time, as
  there are still visible side effects of deleting the record, like bug 10758.

* Re-run the report and compare results to the first run. deleted_biblionumber
  should now be filled with the biblionumber of the deleted record.
  biblionumber should be NULL/empty.
Comment 13 Katrin Fischer 2023-05-04 13:29:04 UTC
Created attachment 150664 [details] [review]
Bug 33262: Unit tests

prove t/db_dependent/Biblio.t
Comment 14 Jonathan Druart 2023-05-04 13:46:37 UTC
"Deleted bibliographic record, can't find title" should not we adjust that then?
Comment 15 Katrin Fischer 2023-05-04 14:08:20 UTC
(In reply to Jonathan Druart from comment #14)
> "Deleted bibliographic record, can't find title" should not we adjust that
> then?

This would be the next step, but I wanted to keep things small and manageable. this is taking care of the data only, but there are already bug 10758 and bug 10869 filed and linked.
Comment 16 Katrin Fischer 2023-05-04 15:09:24 UTC
Created attachment 150680 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.
Comment 17 Katrin Fischer 2023-05-04 15:09:27 UTC
Created attachment 150681 [details] [review]
Bug 33262: [DO NOT PUSH) Schema update
Comment 18 Katrin Fischer 2023-05-04 15:09:29 UTC
Created attachment 150682 [details] [review]
Bug 33262: Store the biblionumber of a deleted record in the order line

When a bibliographic record is deleted and linked to an order
in the acquisition module, the biblionumber in the order line is
currently deleted as well. This makes it impossible to tell what
was ordered in the first place. This is a big issue for libraries
that need to be able to track how money was spent over long periods
of time.

With this patch the biblionumber of the deleted record will be moved
to a new column deleted_biblionumber in the order line, keeping the FK
on biblionumber.

To test:
* Apply patch, run dataase update
* Create a order/basket in the acquisitions module
* Order several things, you can use existing records from the sample data
* Make sure you order one of the records at least twice
* Take a look at the aqorders table in your database, using a report:

  SELECT biblionumber, deleted_biblionumber, ordernumber from aqorders;

* From the catalog, delete the items on the records and then the records

  Note: There is a warning about attached orders warning about consequences
  for acquisitions. I have chosen to keep it at this point in time, as
  there are still visible side effects of deleting the record, like bug 10758.

* Re-run the report and compare results to the first run. deleted_biblionumber
  should now be filled with the biblionumber of the deleted record.
  biblionumber should be NULL/empty.
Comment 19 Katrin Fischer 2023-05-04 15:09:32 UTC
Created attachment 150683 [details] [review]
Bug 33262: Unit tests

prove t/db_dependent/Biblio.t
Comment 20 Katrin Fischer 2023-05-04 15:09:35 UTC
Created attachment 150684 [details] [review]
Bug 33262: Add new field to REST API route for orders

Without this patch the API driven list of pending orders
on the order receive page was broken.

With the patch, all should be well now.

prove -v t/db_dependent/api/v1/acquisitions_orders.t
Comment 21 David Nind 2023-05-05 00:46:29 UTC
Created attachment 150702 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2023-05-05 00:46:32 UTC
Created attachment 150703 [details] [review]
Bug 33262: [DO NOT PUSH) Schema update

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2023-05-05 00:46:35 UTC
Created attachment 150704 [details] [review]
Bug 33262: Store the biblionumber of a deleted record in the order line

When a bibliographic record is deleted and linked to an order
in the acquisition module, the biblionumber in the order line is
currently deleted as well. This makes it impossible to tell what
was ordered in the first place. This is a big issue for libraries
that need to be able to track how money was spent over long periods
of time.

With this patch the biblionumber of the deleted record will be moved
to a new column deleted_biblionumber in the order line, keeping the FK
on biblionumber.

To test:
* Apply patch, run dataase update
* Create a order/basket in the acquisitions module
* Order several things, you can use existing records from the sample data
* Make sure you order one of the records at least twice
* Take a look at the aqorders table in your database, using a report:

  SELECT biblionumber, deleted_biblionumber, ordernumber from aqorders;

* From the catalog, delete the items on the records and then the records

  Note: There is a warning about attached orders warning about consequences
  for acquisitions. I have chosen to keep it at this point in time, as
  there are still visible side effects of deleting the record, like bug 10758.

* Re-run the report and compare results to the first run. deleted_biblionumber
  should now be filled with the biblionumber of the deleted record.
  biblionumber should be NULL/empty.

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2023-05-05 00:46:38 UTC
Created attachment 150705 [details] [review]
Bug 33262: Unit tests

prove t/db_dependent/Biblio.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2023-05-05 00:46:41 UTC
Created attachment 150706 [details] [review]
Bug 33262: Add new field to REST API route for orders

Without this patch the API driven list of pending orders
on the order receive page was broken.

With the patch, all should be well now.

prove -v t/db_dependent/api/v1/acquisitions_orders.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 Nick Clemens 2023-05-05 12:04:08 UTC
Created attachment 150734 [details] [review]
Bug 33262: Add deleted_biblionumber column to aqorders table

This adds a new column deleted_biblionumber to the aqorders table.
This will allow us to store the biblionumber of a deleted record,
so we will still be able to tell what has been ordered once the
record is deleted.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 27 Nick Clemens 2023-05-05 12:04:10 UTC
Created attachment 150735 [details] [review]
Bug 33262: [DO NOT PUSH) Schema update

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Nick Clemens 2023-05-05 12:04:13 UTC
Created attachment 150736 [details] [review]
Bug 33262: Store the biblionumber of a deleted record in the order line

When a bibliographic record is deleted and linked to an order
in the acquisition module, the biblionumber in the order line is
currently deleted as well. This makes it impossible to tell what
was ordered in the first place. This is a big issue for libraries
that need to be able to track how money was spent over long periods
of time.

With this patch the biblionumber of the deleted record will be moved
to a new column deleted_biblionumber in the order line, keeping the FK
on biblionumber.

To test:
* Apply patch, run dataase update
* Create a order/basket in the acquisitions module
* Order several things, you can use existing records from the sample data
* Make sure you order one of the records at least twice
* Take a look at the aqorders table in your database, using a report:

  SELECT biblionumber, deleted_biblionumber, ordernumber from aqorders;

* From the catalog, delete the items on the records and then the records

  Note: There is a warning about attached orders warning about consequences
  for acquisitions. I have chosen to keep it at this point in time, as
  there are still visible side effects of deleting the record, like bug 10758.

* Re-run the report and compare results to the first run. deleted_biblionumber
  should now be filled with the biblionumber of the deleted record.
  biblionumber should be NULL/empty.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Nick Clemens 2023-05-05 12:04:16 UTC
Created attachment 150737 [details] [review]
Bug 33262: Unit tests

prove t/db_dependent/Biblio.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Nick Clemens 2023-05-05 12:04:18 UTC
Created attachment 150738 [details] [review]
Bug 33262: Add new field to REST API route for orders

Without this patch the API driven list of pending orders
on the order receive page was broken.

With the patch, all should be well now.

prove -v t/db_dependent/api/v1/acquisitions_orders.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Tomás Cohen Arazi 2023-05-05 12:29:32 UTC
Well done, Katrina.
Comment 32 Tomás Cohen Arazi 2023-05-05 13:15:25 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 33 Martin Renvoize 2023-05-12 14:05:44 UTC
Many hands makes light work, thankyou everyone!

Pushed to 22.11.x for the next release
Comment 34 Lucas Gass 2023-05-15 22:49:39 UTC
Backported to 22.05.x for upcoming 22.05.13
Comment 35 Arthur Suzuki 2023-05-23 18:33:51 UTC
Conflicts when trying to apply to 21.11, can you provide a backport?