Bug 7493 - Deleting a record with comments breaks
Summary: Deleting a record with comments breaks
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: 3.6
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks: 8149
  Show dependency treegraph
 
Reported: 2012-02-02 11:50 UTC by Magnus Enger
Modified: 2013-12-05 19:59 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 7493 - Deleting a record with comments breaks "Recent Comments" (8.41 KB, patch)
2012-02-17 16:59 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7493 [REVISED] Deleting a record with comments breaks "Recent Comments" (8.41 KB, patch)
2012-02-28 16:12 UTC, Owen Leonard
Details | Diff | Splinter Review
[Signed-off] Bug 7493 [REVISED] Deleting a record with comments breaks "Recent Comments" (8.43 KB, patch)
2012-03-02 13:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7493 [REVISED] Deleting a record with comments breaks "Recent Comments" (7.53 KB, patch)
2012-03-05 14:14 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2012-02-02 11:50:18 UTC
A library deleted a record that had comments attached to it, and now they get an error when they click on "Recent Comments" in the OPAC: 

Software error:
Can't call method "field" on an undefined value at /usr/share/koha/lib/C4/Koha.pm line 1216.

This is version 3.6.3.
Comment 1 Owen Leonard 2012-02-06 15:06:45 UTC
I think the solution to this problem is to add a foreign key constraint so that when bibliographic or patron records are deleted the reviews table is updated. I propose to:

- delete reviews when bibliographic records are deleted
- set set reviews.borrowernumber to NULL when patron records are deleted.

The database update making these changes will not run properly if there are already instances of this bug--reviews with deleted biblios or patrons--so some kind of update will have to be run in addition to setting the constraint.

Does anyone have any comments or suggestions about the efficiency/efficacy of using these queries for that purpose?

DELETE FROM reviews WHERE biblionumber IN (SELECT biblionumber from deletedbiblio);

UPDATE reviews SET borrowernumber = NULL WHERE borrowernumber IN (SELECT borrowernumber FROM deletedborrowers);
Comment 2 Katrin Fischer 2012-02-12 20:00:16 UTC
Hi Owen, 
I am not sure about the SQL, but I think the changes you suggest makes sense and are logical. I am not sure if we might want to delete reviews with the patron account too (privacy issues?).
Comment 3 Owen Leonard 2012-02-17 16:59:06 UTC Comment hidden (obsolete)
Comment 4 Julian Maurice 2012-02-23 10:26:37 UTC
I don't know when or how it happened but I had in database 1 line in biblio and 1 line in deletedbiblio with the same biblionumber, which cause comments on this biblio to be deleted.
I'm sure this is not a common case, and it should not happen, but I would prefer that deletions are done with an SQL query like this

DELETE FROM reviews WHERE biblionumber **NOT** IN (SELECT biblionumber from
**biblio**);
(same for patrons)
Comment 5 Owen Leonard 2012-02-28 16:12:04 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2012-02-28 16:23:58 UTC
The revised patch takes Julian's suggestion to use "NOT IN" biblio and borrowers.
Comment 7 Julian Maurice 2012-03-02 13:36:06 UTC Comment hidden (obsolete)
Comment 8 Ian Walls 2012-03-02 15:46:38 UTC
Database constraints added in safe and sustainable fashion, with cleanup for upgrades.  Template changes are clean.  Marking as Passed QA.
Comment 9 Owen Leonard 2012-03-05 14:14:28 UTC
Created attachment 8011 [details] [review]
Bug 7493 [REVISED] Deleting a record with comments breaks "Recent Comments"

Adding foreign key constraint on biblio and borrowers:

- delete reviews when bibliographic records are deleted
- set set reviews.borrowernumber to NULL when patron records are deleted.

Before these constraints are set the database update script will
clean up existing instances of these problems by deleting reviews of
deleted biblios and setting to NULL reviews.borrowernumber for deleted
patrons.

In comments moderation, the list of comments will indicate "(deleted patron")
if the review was submitted by a patron who has since been deleted.

In the OPAC display of comments will omit the patron name altogether
if the patron has since been deleted.

To test:

1. CONFIRM THAT THE DATABASE UPDATE RUNS CORRECTLY

Before applying the patch:

 - delete the record for a title which has a review
 - delete a patron who has submitted a review (on a different title).

Viewing /cgi-bin/koha/opac-showreviews.pl should result in an error.

Apply the patch and make sure the database update runs. Viewing
the recent comments page in the OPAC should no longer result in an
error. The title with the comment from a deleted patron should
show no patron name on these pages:

 - Recent comments (opac-showreviews.pl)
 - Recent comments RSS (opac-showreviews.pl?format=rss)
 - Detail page (opac-detail.pl)

Comments from other patrons should show correctly.

2. CONFIRM THAT NEW DELETIONS PERFORM CORRECTLY

After applying the patch:

 - delete the record for a title which has a review
 - delete a patron who has submitted a review (on a different title).

Viewing /cgi-bin/koha/opac-showreviews.pl should NOT result in an error.
The review of the title which you deleted should no longer appear in the
list of recent comments.

The title with the comment from a deleted patron should
show no patron name on these pages:

 - Recent comments (opac-showreviews.pl)
 - Recent comments RSS (opac-showreviews.pl?format=rss)
 - Detail page (opac-detail.pl)

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 10 Owen Leonard 2012-03-05 14:15:14 UTC
The patch no longer applied so I rebased and fixed conflicts.
Comment 11 Jared Camins-Esakov 2012-05-09 17:04:54 UTC
This bug will be included in the Koha 3.6.5 release.