Bug 18284

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.

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>

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 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

OK, I can see why it's not at the DBIC level.. C4 module not Koha.. my bad.  :(

(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.

(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?

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>

Created attachment 61209 [details] [review]
Bug 18284: [QA Follow-up] Add test descriptions

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(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.

(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.

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.

(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.

Pushed to Master - Should be in the May 17.05 Release.  Thanks much!

Depends on bug 17196 which is not in 16.11.x - not needed there.