This is a sync issue between schema and upgrades. MASTER structure: CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`) ON DELETE CASCADE ON UPDATE CASCADE $DBversion = '20.06.00.022'; ALTER TABLE itemtypes ADD CONSTRAINT itemtypes_ibfk_1 FOREIGN KEY (parent_type) REFERENCES itemtypes (itemtype) NOTE: When you do not add CASCADE, you fallback to RESTRICT ! Main question is now: Which is right? Should the delete be cascading or not? I would opt for RESTRICT here. A pragmatical choice too; deleting item types will normally not be done so much :)
My feeling would be that if you delete a parent itemtype, it should set the field to NULL in the 'child itemtypes'. Does that make sense? IIUC quietly deleting the 'children' seems a dangerous move, although it should hopefully not be possible while items exist for them.
(In reply to Katrin Fischer from comment #1) > My feeling would be that if you delete a parent itemtype, it should set the > field to NULL in the 'child itemtypes'. Does that make sense? Yes. That is the same as RESTRICT. > IIUC quietly deleting the 'children' seems a dangerous move, although it > should hopefully not be possible while items exist for them. Looking closer, the items table has no FK on itype. So you could delete itemtypes but the items will not be deleted. But note that the interface will not allow you to delete the itemtype. But you could in a script or via sql etc, ending up with items that have not existing itemtype. But that is outside the narrower scope of this report. And probably already reported somewhere else..
(In reply to Marcel de Rooy from comment #2) > (In reply to Katrin Fischer from comment #1) > > My feeling would be that if you delete a parent itemtype, it should set the > > field to NULL in the 'child itemtypes'. Does that make sense? > > Yes. That is the same as RESTRICT. Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot delete the parent type if there still is a child itemtype.
Created attachment 139678 [details] [review] Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1 Change from CASCADE to restrict. In harmony with dbrev 20.06.00.022. Test plan: Run the dbrev. Bonus: update itemtypes set parent_type='VM' where itemtype='CF'; delete from itemtypes where itemtype='VM'; => ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`koha_myclone`.`itemtypes`, CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`)) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Self signoff
RM: Dont forget DBIx update please, not included here
(In reply to Marcel de Rooy from comment #3) > (In reply to Marcel de Rooy from comment #2) > > (In reply to Katrin Fischer from comment #1) > > > My feeling would be that if you delete a parent itemtype, it should set the > > > field to NULL in the 'child itemtypes'. Does that make sense? > > > > Yes. That is the same as RESTRICT. > > Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot > delete the parent type if there still is a child itemtype. Hm, I not sure about this one. You might just add a parent itemtype for testing, the only purpose right now is the max checkouts, later decide to remove it... It's not a required/mandatory field, so I'd day NULL would be ok, maybe with a confirmation warning in the GUI. This way you need to remove it in all itemtypes first, more clicks, but I can live it, just think the other is nicer :)
(In reply to Katrin Fischer from comment #7) > (In reply to Marcel de Rooy from comment #3) > > (In reply to Marcel de Rooy from comment #2) > > > (In reply to Katrin Fischer from comment #1) > > > > My feeling would be that if you delete a parent itemtype, it should set the > > > > field to NULL in the 'child itemtypes'. Does that make sense? > > > > > > Yes. That is the same as RESTRICT. > > > > Actually no :) You describe SET NULL but I opt for RESTRICT. You cannot > > delete the parent type if there still is a child itemtype. > > Hm, I not sure about this one. > > You might just add a parent itemtype for testing, the only purpose right now > is the max checkouts, later decide to remove it... It's not a > required/mandatory field, so I'd day NULL would be ok, maybe with a > confirmation warning in the GUI. > > This way you need to remove it in all itemtypes first, more clicks, but I > can live it, just think the other is nicer :) This would be an enhancment actually. Now I restore the sync gap between dbrev and structure. You are invited to open a new report? :)
Created attachment 140072 [details] [review] Bug 30490: Adjust constraint itemtypes.itemtypes_ibfk_1 Change from CASCADE to restrict. In harmony with dbrev 20.06.00.022. Test plan: Run the dbrev. Bonus: update itemtypes set parent_type='VM' where itemtype='CF'; delete from itemtypes where itemtype='VM'; => ERROR 1451 (23000): Cannot delete or update a parent row: a foreign key constraint fails (`koha_myclone`.`itemtypes`, CONSTRAINT `itemtypes_ibfk_1` FOREIGN KEY (`parent_type`) REFERENCES `itemtypes` (`itemtype`)) Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Pushed to master for 22.11. Nice work everyone, thanks!