Bug 1963 - Problem with deleted biblio in a virtual shelf
Summary: Problem with deleted biblio in a virtual shelf
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: 3.10
Hardware: All All
: P3 normal (vote)
Assignee: Frédéric Demians
QA Contact:
URL: http://bookshelves/shelves.pl
Keywords:
Depends on: 8915
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-21 04:29 UTC by Pascale Nalon
Modified: 2013-12-05 20:01 UTC (History)
4 users (show)

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


Attachments
Bug 1963 Problem with deleted biblio in a virtual shelf (1.53 KB, patch)
2012-10-05 09:30 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 1963 Problem with deleted biblio in a virtual shelf (1.63 KB, patch)
2012-10-05 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:36:38 UTC


---- Reported by pascale.nalon@ensmp.fr 2008-03-21 04:29:15 ----

Deleting a biblio, that is present in a virtual shelf, doesn't remove the biblio from the virtual shelf.
Instead you get a blank line and no more possibility to remove it.
It seems better to delete the biblio both in the catalog and in the virtual shelf.

This bug seems to be corrected in the V3.0.



---- Additional Comments From chris.nighswonger@liblime.com 2008-06-19 13:44:30 ----



*** This bug has been marked as a duplicate of http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=1889 ***



--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:36 UTC  ---

This bug was previously known as _bug_ 1963 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=1963

Actual time not defined. Setting to 0.0
The original assignee of this bug does not have
   an account here. Reassigning to the default assignee
   for the component, gmcharlt@gmail.com.
   Previous assignee was jmf@liblime.com.
This bug was marked DUPLICATE in the database it was moved from.
    Changing resolution to "MOVED"

Comment 1 Frédéric Demians 2012-10-05 09:24:19 UTC
This bug reappear in HEAD/3.8. When trying to display a virtual shelf
(list) containing a deleted biblio, this error message is displayed:

Software error:

Can't call method "field" on an undefined value at
....C4/Koha.pm line 1231.

A fix is possible by modifying SQL query retrieving biblios, just
skipping records from virtual shelf table with no linked record in biblio
table.
Comment 2 Frédéric Demians 2012-10-05 09:30:07 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2012-10-05 12:14:11 UTC
Created attachment 12696 [details] [review]
Bug 1963 Problem with deleted biblio in a virtual shelf

This bug reappear in HEAD/3.8. When trying to display a virtual shelf
(list) containing a deleted biblio, this error message is displayed:

Software error:

Can't call method "field" on an undefined value at ....C4/Koha.pm line
1231.

This fix modify SQL query retrieving biblios, just skipping records from
virtual shelf table with no linked record in biblio table: LEFT JOIN
replace with JOIN.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 4 Paul Poulain 2012-10-12 21:15:25 UTC
QA comment:
 * this patch fixes the display, but not the main problem: we should not let a list have a biblio that has been removed. A follow-up would be useful to clean the database then add a constraint
 * I agree that fixing the display is a first step, but this bug should not be closed until the main problem is fixed.

passed QA for this one
Comment 5 Paul Poulain 2012-10-12 21:20:13 UTC
mmm... before pushing, I had a doubt and checked kohastructure. The constraint already exists.

  CONSTRAINT `shelfcontents_ibfk_2` FOREIGN KEY (`biblionumber`) REFERENCES `biblio` (`biblionumber`) ON DELETE CASCADE ON UPDATE CASCADE,


So, Frederic, this case should not happen. Could you check your structure, and do you have an explanation.

The constraint has been added by commit
d6f8fde9 installer/data/mysql/kohastructure.sql (Marcel de Rooy       2011-12-08 16:10:57 +0100 2128)   CONSTRAINT `shel

is it possible that your database has been created before and the updatedatabase failed (because of invalid data) ?

(not pushing this patch for now, needs a little bit more investigation)
Comment 6 Frédéric Demians 2012-10-13 07:21:23 UTC
Thanks a lot Paul for catching that!

Yes, there should have a constraint on shelfcontents.biblionumber field.
And there isn't one.

For others who would like to test there table:

  SHOW CREATE TABLE virtualshelfcontents

I have several Koha DB without constraints on that table, and on other
tables. This issue doesn't come from an updatedatabase.pl having failed,
but from tables which have been in the past associated with MyISAM
storage engine rather than InnoDB. MyISAM doesn't support referential
integrity constraints. So if a table was MyISAM during an update adding
a new constraint, the constraint was just not created. 

So, please, push this patch. The constraint issue is more general, and
related to storage Engine used. InnoDB is now mandatory. InnoDB is the
MySQL default storage engine since version 5.1, replacing MyISAM. So the
problem may occur less and less. But for legacy Koha DB, it can happen,
and it can have surprising effect like this bug.
Comment 7 Paul Poulain 2012-10-16 16:07:22 UTC
(In reply to comment #6)
> So, please, push this patch. The constraint issue is more general, and
> related to storage Engine used. InnoDB is now mandatory. InnoDB is the
> MySQL default storage engine since version 5.1, replacing MyISAM. So the
> problem may occur less and less. But for legacy Koha DB, it can happen,
> and it can have surprising effect like this bug.

mmm... definetely, i'm not for pushing this patch: without the patch, the problem is visible (even if, I agree, it's not easy to link the error with the DB problem)
With the patch it's no more visible, and, as we say in french, it's "hiding the dust under the carpet"
The correct solution is to fix your database problem.

I'll ask other QA team member for their opinion
Comment 8 Jared Camins-Esakov 2012-10-16 16:10:47 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > So, please, push this patch. The constraint issue is more general, and
> > related to storage Engine used. InnoDB is now mandatory. InnoDB is the
> > MySQL default storage engine since version 5.1, replacing MyISAM. So the
> > problem may occur less and less. But for legacy Koha DB, it can happen,
> > and it can have surprising effect like this bug.
> 
> mmm... definetely, i'm not for pushing this patch: without the patch, the
> problem is visible (even if, I agree, it's not easy to link the error with
> the DB problem)
> With the patch it's no more visible, and, as we say in french, it's "hiding
> the dust under the carpet"
> The correct solution is to fix your database problem.
> 
> I'll ask other QA team member for their opinion

I disagree. Frédéric submitted a patch which fixes the constraint problem, but in the meantime, this is a good workaround for those libraries that have the problem. There is absolutely no reason why library patrons should have to deal with errors, ever.
Comment 9 Paul Poulain 2012-10-16 16:24:28 UTC
(In reply to comment #8)

> I disagree. Frédéric submitted a patch which fixes the constraint problem,
> but in the meantime, this is a good workaround for those libraries that have
> the problem. There is absolutely no reason why library patrons should have
> to deal with errors, ever.

Jared, it does not fix the constraint problem, it fixes the perl error.
The proper way to fix the problem is to issue 3 SQL commands:
1- delete all wrong entries
2- switch to innodb
3- add the constraint

* DELETE FROM virtualshefcontent WHERE biblionumber NOT IN (SELECT biblio.biblionumber FROM biblio WHERE biblio.biblionumber=virtualshelfcontent.biblionumber)
 * ALTER TABLE virtualshelfcontent ENGINE=innodb
 * ALTER TABLE virtualshelfcontent ADD CONSTRAINT ...
Comment 10 Jared Camins-Esakov 2012-10-16 16:29:16 UTC
(In reply to comment #9)
> (In reply to comment #8)
> 
> > I disagree. Frédéric submitted a patch which fixes the constraint problem,
> > but in the meantime, this is a good workaround for those libraries that have
> > the problem. There is absolutely no reason why library patrons should have
> > to deal with errors, ever.
> 
> Jared, it does not fix the constraint problem, it fixes the perl error.
> The proper way to fix the problem is to issue 3 SQL commands:
> 1- delete all wrong entries
> 2- switch to innodb
> 3- add the constraint
> 
> * DELETE FROM virtualshefcontent WHERE biblionumber NOT IN (SELECT
> biblio.biblionumber FROM biblio WHERE
> biblio.biblionumber=virtualshelfcontent.biblionumber)
>  * ALTER TABLE virtualshelfcontent ENGINE=innodb
>  * ALTER TABLE virtualshelfcontent ADD CONSTRAINT ...

Not this patch. Frédéric submitted a separate patch for updating the constraints. Bug 8915. However, that one requires a great deal of testing, I think. This patch will serve to keep patrons from being exposed to errors while we make sure there are no undesired side effects.
Comment 11 Marcel de Rooy 2012-10-17 11:25:22 UTC
I would favor pushing this patch, if we fix the constraint stuff on another report (including some lines in updatedatabase to correct the historical "ballast" from MyISAM etc.)
This patches fixes a display problem that theoretically should not occur, but in practice does. The other report would deal with the wrong data.
Comment 12 Kyle M Hall 2012-10-17 13:27:42 UTC
(In reply to comment #11)
> I would favor pushing this patch, if we fix the constraint stuff on another
> report (including some lines in updatedatabase to correct the historical
> "ballast" from MyISAM etc.)
> This patches fixes a display problem that theoretically should not occur,
> but in practice does. The other report would deal with the wrong data.

I tend to agree. This patch may just be 'hiding' the underlying problem, but we are now aware of it. As long as we follow up on this issue with a correction 'under the hood', I see no reason not to push this as a quick fix from the user stand point.
Comment 13 Paul Poulain 2012-10-17 16:06:43 UTC
Patch pushed to master