---- 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"
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.
Created attachment 12694 [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.
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>
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
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)
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.
(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
(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.
(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 ...
(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.
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.
(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.
Patch pushed to master