Bug 9071 - Updates adding systempreferences should verify the absence of those preferences prior to attempting to add them
Summary: Updates adding systempreferences should verify the absence of those preferenc...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: Main
Hardware: All All
: P1 - high major
Assignee: Chris Nighswonger
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-13 16:31 UTC by Chris Nighswonger
Modified: 2015-12-03 22:00 UTC (History)
7 users (show)

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

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Nighswonger 2012-11-13 16:31:23 UTC 
Performing a blind INSERT of a new system preference causes the webinstaller to complain loudly. If the user happens to have DEBUG set, updatedatabase.pl bails on the error.

In any case, this is generally bad behavior, and should be fixed.
Comment 1 Liz Rea 2012-11-16 02:37:24 UTC 
It would also clear up those annoying "this table already exists" errors for devs (when their database already has the table due to testing or whatever). I approve.
Comment 2 Owen Leonard 2014-07-15 16:54:42 UTC 
Is this bug still valid, and if so should it still be assigned?

This sounds like something that might be considered as a coding guideline rather than something to be patched, or was it planned to retroactively fix updatedatabase.pl?
Comment 3 Chris Nighswonger 2014-07-15 18:22:22 UTC 
(In reply to Owen Leonard from comment #2)
> Is this bug still valid, and if so should it still be assigned?

AFAIK it manifests in upgrades from older versions. Not sure if it shows up when upgrading from more recent versions.

> 
> This sounds like something that might be considered as a coding guideline
> rather than something to be patched, or was it planned to retroactively fix
> updatedatabase.pl?

It was a hope to code up a routine to be called before each INSERT to verify existence. A coding guidline would be helpful as well.

Setting it back to NEW since I'm not in the position to fix it atm.
Comment 4 Jonathan Druart 2015-04-01 11:16:05 UTC 
The good practice would be to have an "insert ignore into" statement.
But actually you are right, we missed a lot of "insert into" without any check.

QAers should keep a closed eye on this.
Comment 5 Marcel de Rooy 2015-04-01 11:26:59 UTC 
Sounds like we can close this report?
Comment 6 Chris Nighswonger 2015-04-01 11:30:01 UTC 
(In reply to M. de Rooy from comment #5)
> Sounds like we can close this report?

Had the problem been addressed in a sustainable way? I'm not sure it has.
Comment 7 Jonathan Druart 2015-04-01 11:34:32 UTC 
It should be added to the coding guidelines and catched by QAers for new patches.
Comment 8 Chris Nighswonger 2015-04-01 11:40:57 UTC 
(In reply to Jonathan Druart from comment #7)
> It should be added to the coding guidelines and catched by QAers for new
> patches.

I'd like to see it added and a link posted here when the bug is marked resolved for continuity's sake. I'm working on my phone or I'd do it.