Bug 18284 - Biblio metadata are not moved to the deleted table when a biblio is deleted
Summary: Biblio metadata are not moved to the deleted table when a biblio is deleted
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 17196
Blocks:
  Show dependency treegraph
 
Reported: 2017-03-16 18:22 UTC by Jonathan Druart
Modified: 2017-03-26 19:51 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted (4.86 KB, patch)
2017-03-16 18:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted (5.03 KB, patch)
2017-03-16 20:54 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted (5.12 KB, patch)
2017-03-17 10:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18284: [QA Follow-up] Add test descriptions (1.19 KB, patch)
2017-03-17 10:07 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-03-16 18:22:38 UTC
There is a deletedbiblio_metadata table but it is not populated when a biblio is deleted. Since we have a ON DELETE constraint on biblio_metadata.biblionumber, the row is deleted when the biblio entry is deleted => data lost!

We absolutely need this one before the next release.
Comment 1 Jonathan Druart 2017-03-16 18:27:12 UTC
Created attachment 61184 [details] [review]
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted

There is a deletedbiblio_metadata table but it is not populated when a
biblio is deleted. Since we have a ON DELETE constraint on
biblio_metadata.biblionumber, the row is deleted when the biblio entry
is deleted => data lost!

Test plan:
- Create a biblio
- Delete it

=> Without this patch the deletedbiblio_metadata table is not populated
with the biblio_metadata row related to the biblio

=> With this patch applied you should see that the row has been moved.
Comment 2 Marc Véron 2017-03-16 20:54:53 UTC
Created attachment 61193 [details] [review]
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted

There is a deletedbiblio_metadata table but it is not populated when a
biblio is deleted. Since we have a ON DELETE constraint on
biblio_metadata.biblionumber, the row is deleted when the biblio entry
is deleted => data lost!

Test plan:
- Create a biblio
- Delete it

=> Without this patch the deletedbiblio_metadata table is not populated
with the biblio_metadata row related to the biblio

=> With this patch applied you should see that the row has been moved.

Followed test plan, behaves as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Martin Renvoize 2017-03-17 07:03:04 UTC
I wish we flagged deleted rather than had deleted_* tables.. this sort of thing would be far less fraught with dangers that way.

I would also do this at the DBIC level personally.. with a monkeypatch of the standard delete call.
Comment 4 Martin Renvoize 2017-03-17 07:10:24 UTC
Comment on attachment 61193 [details] [review]
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted

Review of attachment 61193 [details] [review]:
-----------------------------------------------------------------

New FIXME..?

::: C4/Biblio.pm
@@ +3361,4 @@
>      my $sth = $dbh->prepare("SELECT * FROM biblio WHERE biblionumber=?");
>      $sth->execute($biblionumber);
>  
> +    # FIXME There is a transaction in _koha_delete_biblio_metadata

Why add the FIXME rather than adding the code to actually solve the problem?

Nested transactions work well in DBIC whenever I've used them
Comment 5 Martin Renvoize 2017-03-17 07:13:33 UTC
OK, I can see why it's not at the DBIC level.. C4 module not Koha.. my bad.  :(
Comment 6 Olli-Antti Kivilahti 2017-03-17 08:07:11 UTC
(In reply to Martin Renvoize from comment #3)
> I wish we flagged deleted rather than had deleted_* tables.. this sort of
> thing would be far less fraught with dangers that way.
> 
> I would also do this at the DBIC level personally.. with a monkeypatch of
> the standard delete call.

I kinda agree. Having two separate tables makes doing reports very very awkward.

But I guess there would be a performance penalty of having 5 years of history in the biblioitems-table eh?
After over two and a half years of service, our DB has 153569 deletedbiblio- and 1422962 biblio-rows.

deletebiblio has no effect performance-wise. Martin I am 100% with you on this topic.


Martin. Take a look at Bug 18265 and comment on what these cultural Marxists are about to do.
Comment 7 Marcel de Rooy 2017-03-17 09:47:25 UTC
(In reply to Olli-Antti Kivilahti from comment #6)
> comment on what these cultural Marxists are about to do.

Hmm
Quote: "about 99.9% of the time "Cultural Marxism" is a snarl word used to attack anyone with progressive tendencies"
Link with Nazi Germany?

imo we could do without such terms?
Comment 8 Marcel de Rooy 2017-03-17 10:06:53 UTC
Created attachment 61208 [details] [review]
Bug 18284: (bug 17196 follow-up) Move biblio metadata when a biblio is deleted

There is a deletedbiblio_metadata table but it is not populated when a
biblio is deleted. Since we have a ON DELETE constraint on
biblio_metadata.biblionumber, the row is deleted when the biblio entry
is deleted => data lost!

Test plan:
- Create a biblio
- Delete it

=> Without this patch the deletedbiblio_metadata table is not populated
with the biblio_metadata row related to the biblio

=> With this patch applied you should see that the row has been moved.

Followed test plan, behaves as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2017-03-17 10:07:25 UTC
Created attachment 61209 [details] [review]
Bug 18284: [QA Follow-up] Add test descriptions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-03-17 10:09:39 UTC
(In reply to Martin Renvoize from comment #4)
> Why add the FIXME rather than adding the code to actually solve the problem?

Passed QA since we need the deleted xml.
But I agree that the FIXME and the transaction stuff needs further attention.
The result of the new txn_do call is not even checked.
Comment 11 Jonathan Druart 2017-03-17 12:26:08 UTC
(In reply to Martin Renvoize from comment #3)
> I wish we flagged deleted rather than had deleted_* tables.. this sort of
> thing would be far less fraught with dangers that way.

I am just following the existing pattern, used twice already. Moreover the table already exists and we want to keep FK. Without 2 differents we could not keep the FK.

> I would also do this at the DBIC level personally.. with a monkeypatch of
> the standard delete call.

With a critical or blocker I always try to provide as small fix as possible to avoid any integration problems.
Without using DBIx::Class I avoid creating 2 new packages + provide tests (~100 lines minimum).

(In reply to Martin Renvoize from comment #4)
> Comment on attachment 61193 [details] [review] [review]
> New FIXME..?

Yes? What's wrong with FIXME?
I provide code that is not good, I highlight it in the codebase.

> ::: C4/Biblio.pm
> @@ +3361,4 @@
> >      my $sth = $dbh->prepare("SELECT * FROM biblio WHERE biblionumber=?");
> >      $sth->execute($biblionumber);
> >  
> > +    # FIXME There is a transaction in _koha_delete_biblio_metadata
> 
> Why add the FIXME rather than adding the code to actually solve the problem?
> 
> Nested transactions work well in DBIC whenever I've used them

Yes of course, I use them in these tests and they pass.
Comment 12 Martin Renvoize 2017-03-17 12:37:19 UTC
I'm happy with this.. just thought the FIXME worth pointing out.  I'm hoping there's an intention to go back and fix the fixme 'tis all ;)

Is there a broader discussion anywhere regarding delete_* tables.. that was more a throw away comment to be honest.  I was tempted to pass qa on it myself but hadn't yet fully tested it.
Comment 13 Jonathan Druart 2017-03-17 12:40:33 UTC
(In reply to Martin Renvoize from comment #12)
> I'm happy with this.. just thought the FIXME worth pointing out.  I'm hoping
> there's an intention to go back and fix the fixme 'tis all ;)
> 
> Is there a broader discussion anywhere regarding delete_* tables.. that was
> more a throw away comment to be honest.  I was tempted to pass qa on it
> myself but hadn't yet fully tested it.

Yes, and especially the last 2 months as we faced critical issues, like bug 18242 for instance, or bug 18003.
Comment 14 Brendan Gallagher 2017-03-22 10:31:53 UTC
Pushed to Master - Should be in the May 17.05 Release.  Thanks much!
Comment 15 Katrin Fischer 2017-03-26 19:51:39 UTC
Depends on bug 17196 which is not in 16.11.x - not needed there.