Bug 27842 - Incorrect biblionumber handling in serials subscriptions
Summary: Incorrect biblionumber handling in serials subscriptions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi (tcohen)
URL:
Keywords:
Depends on: 21901
Blocks: 28298
  Show dependency treegraph
 
Reported: 2021-03-02 19:29 UTC by Nick Clemens (kidclamp)
Modified: 2021-12-13 21:10 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:
21.05.00,20.11.06,20.05.12
Circulation function:


Attachments
Bug 27842: Add tests (2.22 KB, patch)
2021-03-03 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27842: Update serial.biblionumber when a subscription is linked with another record (1.61 KB, patch)
2021-03-03 11:23 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27842: Update serial.biblionumber when a subscription is linked with another record (1.66 KB, patch)
2021-03-03 12:13 UTC, Peter Vashchuk
Details | Diff | Splinter Review
Bug 27842: Add tests (2.29 KB, patch)
2021-03-04 11:27 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27842: Add tests (2.29 KB, patch)
2021-03-04 12:11 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27842: Add tests (2.29 KB, patch)
2021-03-04 12:12 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27842: Update serial.biblionumber when a subscription is linked with another record (1.73 KB, patch)
2021-03-04 12:12 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 27842: (follow-up) Correct existing serail/subscriptionhistory before deletion (2.11 KB, patch)
2021-03-04 12:43 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 27842: Add tests (2.33 KB, patch)
2021-04-27 14:43 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 27842: Update serial.biblionumber when a subscription is linked with another record (1.77 KB, patch)
2021-04-27 14:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 27842: (follow-up) Correct existing serail/subscriptionhistory before deletion (2.16 KB, patch)
2021-04-27 14:44 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2021-03-02 19:29:42 UTC
It seems that a librarian can edit the biblionumber for a given subscription, however, when receiving serials the biblionumber is pulled from the existing serials, so all new received issues are received on the old biblionumber

This is problematic in light of bug 21901 - you can change the biblionumber, the serials still point to the old biblionumber - and deleting the biblio deletes the serials. Additionally, the update will delete serials where this was the case

To recreate:
1 - Create a new subscription
2 - Receive an issue
3 - Edit the subscription and change the biblionumber
4 - Receive another issue
5 - SELECT * FROM serial
6 - Both have the old number
7 - Delete the old biblio
8 - The serials are deleted
9 - You also cannot now receive for this issue or generate serials
Comment 1 Martin Renvoize (ashimema) 2021-03-03 09:11:49 UTC
Humm.. should the old serials follow the biblionumber change too.. or should the logic be changed such that newly received serials get the update bib and old one's keep the old bib?
Comment 2 Jonathan Druart 2021-03-03 11:10:11 UTC
Why do we have serial.biblionumber?
We should get the biblio link using serial->subscription->biblionumber
Comment 3 Jonathan Druart 2021-03-03 11:23:04 UTC
Created attachment 117565 [details] [review]
Bug 27842: Add tests
Comment 4 Jonathan Druart 2021-03-03 11:23:07 UTC
Created attachment 117566 [details] [review]
Bug 27842: Update serial.biblionumber when a subscription is linked with another record

It seems that a librarian can edit the biblionumber for a given subscription, however, when receiving serials the biblionumber is pulled from the existing serials, so all new received issues are received on the old biblionumber.

This is problematic in light of bug 21901 - you can change the biblionumber, the serials still point to the old biblionumber - and deleting the biblio deletes the serials. Additionally, the update will delete serials where this was the case

Test plan:
1 - Create a new subscription
2 - Receive an issue
3 - Edit the subscription and change the biblionumber
4 - Receive another issue
5 - SELECT biblionumber FROM serial
=> the biblionumber link must have been updated
Comment 5 Peter Vashchuk 2021-03-03 12:13:34 UTC
Created attachment 117567 [details] [review]
Bug 27842: Update serial.biblionumber when a subscription is linked with another record

It seems that a librarian can edit the biblionumber for a given subscription, however, when receiving serials the biblionumber is pulled from the existing serials, so all new received issues are received on the old biblionumber.

This is problematic in light of bug 21901 - you can change the biblionumber, the serials still point to the old biblionumber - and deleting the biblio deletes the serials. Additionally, the update will delete serials where this was the case

Test plan:
1 - Create a new subscription
2 - Receive an issue
3 - Edit the subscription and change the biblionumber
4 - Receive another issue
5 - SELECT biblionumber FROM serial
=> the biblionumber link must have been updated

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>
Comment 6 Kyle M Hall (khall) 2021-03-04 11:27:22 UTC
Created attachment 117677 [details] [review]
Bug 27842: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Kyle M Hall (khall) 2021-03-04 12:11:43 UTC
Created attachment 117696 [details] [review]
Bug 27842: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall (khall) 2021-03-04 12:12:08 UTC
Created attachment 117697 [details] [review]
Bug 27842: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Kyle M Hall (khall) 2021-03-04 12:12:18 UTC
Created attachment 117698 [details] [review]
Bug 27842: Update serial.biblionumber when a subscription is linked with another record

It seems that a librarian can edit the biblionumber for a given subscription, however, when receiving serials the biblionumber is pulled from the existing serials, so all new received issues are received on the old biblionumber.

This is problematic in light of bug 21901 - you can change the biblionumber, the serials still point to the old biblionumber - and deleting the biblio deletes the serials. Additionally, the update will delete serials where this was the case

Test plan:
1 - Create a new subscription
2 - Receive an issue
3 - Edit the subscription and change the biblionumber
4 - Receive another issue
5 - SELECT biblionumber FROM serial
=> the biblionumber link must have been updated

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Nick Clemens (kidclamp) 2021-03-04 12:43:30 UTC
Created attachment 117712 [details] [review]
Bug 27842: (follow-up) Correct existing serail/subscriptionhistory before deletion
Comment 11 Nick Clemens (kidclamp) 2021-03-04 12:46:23 UTC
I added a followup which should be tested - before we add the constraints and delete we should update serial and subscription history

To test:
1 - Checkout a version before 21901
2 - Reset db
3 - Add a subscription
4 - Edit the subscription to a new biblionumber
5 - Update database
6 - Confirm the subscriptionid still has entries in serial and subscriptionhistory
Comment 12 Jonathan Druart 2021-03-04 14:46:49 UTC
Can someone test the last patch please?
Comment 13 Katrin Fischer 2021-03-04 21:55:07 UTC
I wonder if we are on the right track here by changing the information in past serials or if this doesn't destroy useful information.

Serials are known to be odd. When you look at the national serials database they have a graph view on their record detail pages where you can see how often they change their names, merge with others... divide etc. It's pretty, but also scary!
And for our catalogers every change like this means a new record.

So it makes sense that we can change which record a subscription is attached to. The name change would not necessarily affect the subscription itself.

So could it be by design that the original biblionumber was stored? As you said, we don't actually need it, as we have the current biblionumber in the subscription. If it was thought to be for keeping some history, it could make sense and the fix would maybe to stop it from deleting the issues when the record is deleted. We could also think about updating the biblionumber of the next expected issue when the biblionumber is changed.

Not sure about this - just something I was wondering about when I noticed the biblionumber in serials in the past.
Comment 14 Jonathan Druart 2021-03-05 09:29:16 UTC
Nicolas, Séverine, could you give us your input on this one?
Comment 15 Nicolas Legrand 2021-03-05 09:52:15 UTC
(In reply to Jonathan Druart from comment #14)
> Nicolas, Séverine, could you give us your input on this one?

This is far beyond my own skills and from my point of view, understanding serials is a bit like understanding the Necronomicon. Nevertheless, Séverine is working on it with our serials specialists :).
Comment 16 Nick Clemens (kidclamp) 2021-03-17 12:46:46 UTC
The database update from  21901 causes data loss - while we can discuss future handling, currently a serial that changed biblios and the old record was deleted will remove all subscriptionhistory and serial entries for the subscription upon upgrade. Should I move that patch to its own bug to prevent data loss for libraries so we can move that forward?
Comment 17 Katrin Fischer 2021-03-21 15:36:14 UTC
The data loss angle is the one I am worried about as well. With the solution proposed here we lose the former history. That's why I was thinking we might want to check on why the field was there in the first place.

Should we just have a patch undoing the constraint for now?
Comment 18 Nick Clemens (kidclamp) 2021-03-22 15:27:43 UTC
(In reply to Katrin Fischer from comment #17)
> The data loss angle is the one I am worried about as well. With the solution
> proposed here we lose the former history. That's why I was thinking we might
> want to check on why the field was there in the first place.
> 
> Should we just have a patch undoing the constraint for now?

I don't think we are losing anything by setting the biblionumber to the current.

In the case of a serial having had 2,3, or more biblionumbers, we are only using the first one - so while one previous connection was "preserved" any future ones were not. Even with one change preserved I don't see any evidence that it was intentional
Comment 19 Katrin Fischer 2021-03-23 07:37:02 UTC
(In reply to Nick Clemens from comment #18)
> (In reply to Katrin Fischer from comment #17)
> > The data loss angle is the one I am worried about as well. With the solution
> > proposed here we lose the former history. That's why I was thinking we might
> > want to check on why the field was there in the first place.
> > 
> > Should we just have a patch undoing the constraint for now?
> 
> I don't think we are losing anything by setting the biblionumber to the
> current.
> 
> In the case of a serial having had 2,3, or more biblionumbers, we are only
> using the first one - so while one previous connection was "preserved" any
> future ones were not. Even with one change preserved I don't see any
> evidence that it was intentional

Can you explain about only using the first one? As I read it we are updating all of them:

UPDATE serial JOIN subscription USING (subcriptionid) SET serial.biblionumber = subscription.biblionumber WHERE serial.biblionumber != subscription.biblionumber

Why not just update the receive process to use the subscription for pulling the biblionumber and change the constraint to set null on delete?
Comment 20 Nick Clemens (kidclamp) 2021-03-23 09:58:35 UTC
(In reply to Katrin Fischer from comment #19)
> (In reply to Nick Clemens from comment #18)
> > (In reply to Katrin Fischer from comment #17)
> > > The data loss angle is the one I am worried about as well. With the solution
> > > proposed here we lose the former history. That's why I was thinking we might
> > > want to check on why the field was there in the first place.
> > > 
> > > Should we just have a patch undoing the constraint for now?
> > 
> > I don't think we are losing anything by setting the biblionumber to the
> > current.
> > 
> > In the case of a serial having had 2,3, or more biblionumbers, we are only
> > using the first one - so while one previous connection was "preserved" any
> > future ones were not. Even with one change preserved I don't see any
> > evidence that it was intentional
> 
> Can you explain about only using the first one? As I read it we are updating
> all of them:
> 
> UPDATE serial JOIN subscription USING (subcriptionid) SET
> serial.biblionumber = subscription.biblionumber WHERE serial.biblionumber !=
> subscription.biblionumber
> 
> Why not just update the receive process to use the subscription for pulling
> the biblionumber and change the constraint to set null on delete?

Sorry I meant, the serial biblionumber is always the same, i.e.:
Create subscription on biblionumber 1 and recieve a serial, it uses biblionumber1
Edit subscription to use biblionumber 2 and receive a serial, it uses biblionumber 1
Edit subscription to use biblionumber 3 and receive a serial, it uses biblionumber on
Edit subscription to use biblionumber 4...and so on

We are not keeping a history of the biblionumber, but rather keeping only the original biblionumber

I don't see that storing 'NULL' or one older biblionumber gains us much over storing the current bibloionumber
Comment 21 Katrin Fischer 2021-03-26 11:30:07 UTC
> Sorry I meant, the serial biblionumber is always the same, i.e.:
> Create subscription on biblionumber 1 and recieve a serial, it uses
> biblionumber1
> Edit subscription to use biblionumber 2 and receive a serial, it uses
> biblionumber 1
> Edit subscription to use biblionumber 3 and receive a serial, it uses
> biblionumber on
> Edit subscription to use biblionumber 4...and so on
> 
> We are not keeping a history of the biblionumber, but rather keeping only
> the original biblionumber
> 
> I don't see that storing 'NULL' or one older biblionumber gains us much over
> storing the current bibloionumber

Thx for clearing this up! In that case I see no issue in changing the data.
Comment 22 Victor Grousset/tuxayo 2021-04-11 23:44:00 UTC
I did the test plan of the 1st patch and got this result in the DB

100 is the initial bibnumber and 101 is the edited one.
+----------+--------------+--------+
| serialid | biblionumber | status |
+----------+--------------+--------+
|        1 |          101 |      2 |
|        2 |          101 |      2 |
|        3 |          100 |      1 |
+----------+--------------+--------+

This is not expected right?
Comment 23 Katrin Fischer 2021-04-12 09:52:55 UTC
Filed bug 28133 for my thoughts of a possible future use of the biblionumber field in serials.
Comment 24 Jonathan Druart 2021-04-12 09:59:10 UTC
I bet you get it if you receive another serial item.

It's because GetSubscription does not return subscription.biblionumber as the biblionumber, but subscriptionhistory.biblionumber instead.

So 2 questions:
- Should we update subscriptionhistory.biblionumber=subscription.biblionumber when a subscription is linked to a new biblio?
- If so, what's the point of subscriptionhistory.biblionumber? Shouldn't we remove it and use the subscriptionid FK?
Comment 25 Victor Grousset/tuxayo 2021-04-24 00:25:01 UTC
> I bet you get it if you receive another serial item.

IIUC another serial item <=> another issue. So yes, if I got right on the step 4 of the test plan (had to impro--, hum, learn a lot about serial)
Comment 26 Tomás Cohen Arazi (tcohen) 2021-04-27 14:43:58 UTC
Created attachment 120239 [details] [review]
Bug 27842: Add tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi (tcohen) 2021-04-27 14:44:05 UTC
Created attachment 120240 [details] [review]
Bug 27842: Update serial.biblionumber when a subscription is linked with another record

It seems that a librarian can edit the biblionumber for a given subscription, however, when receiving serials the biblionumber is pulled from the existing serials, so all new received issues are received on the old biblionumber.

This is problematic in light of bug 21901 - you can change the biblionumber, the serials still point to the old biblionumber - and deleting the biblio deletes the serials. Additionally, the update will delete serials where this was the case

Test plan:
1 - Create a new subscription
2 - Receive an issue
3 - Edit the subscription and change the biblionumber
4 - Receive another issue
5 - SELECT biblionumber FROM serial
=> the biblionumber link must have been updated

Signed-off-by: Petro Vashchuk <stalkernoid@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi (tcohen) 2021-04-27 14:44:11 UTC
Created attachment 120241 [details] [review]
Bug 27842: (follow-up) Correct existing serail/subscriptionhistory before deletion

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Jonathan Druart 2021-04-28 09:16:55 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 30 Fridolin Somers 2021-04-30 09:10:25 UTC
Pushed to 20.11.x for 20.11.06
Comment 31 Alex Arnaud 2021-05-07 08:33:55 UTC
Hello,

Changes on DBRev 19.12.00.076 break the updatedatabase (typo). I fixed that on bug 28298.
Appart from that, shouldn't these changes be done in a new DBRev ? I think Koha 20.05 or 20.11 will never get the DB fix.
Comment 32 Fridolin Somers 2021-05-07 08:51:32 UTC
I remove from 20.11.x since it needs more work
Comment 33 Jonathan Druart 2021-05-07 08:54:04 UTC
Please follow-up on bug 28298 ASAP.
Comment 34 Fridolin Somers 2021-05-12 14:00:07 UTC
Pushed to 20.11.x for 20.11.06
Comment 35 Andrew Fuerste-Henry 2021-05-24 19:08:34 UTC
Pushed to 20.05.x for 20.05.12
Comment 36 Victor Grousset/tuxayo 2021-05-24 19:20:21 UTC
Just tested, 19.11.x not affected, no backport.