Bug 7726 - AllowItemsOnHoldCheckout has columns in wrong order in sysprefs.sql
Summary: AllowItemsOnHoldCheckout has columns in wrong order in sysprefs.sql
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low trivial (vote)
Assignee: Paul Poulain
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 13:09 UTC by Magnus Enger
Modified: 2015-06-04 23:24 UTC (History)
4 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 7726: explanation and options that were in the wrong order (3.47 KB, patch)
2012-03-27 07:56 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7726: explanation and options that were in the wrong order (3.52 KB, patch)
2012-04-09 08:28 UTC, Chris Cormack
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-03-16 13:09:47 UTC
installer/data/mysql/sysprefs.sql has this, on line 341: 

INSERT INTO systempreferences (variable,value,options,explanation,type) VALUES ('AllowItemsOnHoldCheckout',0,'Do not generate RESERVE_WAITING and RESERVED warning when checking out items reserved to someone else. This allows self checkouts for those items.','','YesNo');

The order of the columns should be switched to this:

(variable,value,explanation,options,type)

The problem is also present in updatedatabase.pl:

$dbh->do("INSERT INTO `systempreferences` (variable,value,options,explanation,type) VALUES ('AllowItemsOnHoldCheckout',0,'Do not generate RESERVE_WAITING and RESERVED warning when checking out items reserved to someone else. This allows self checkouts for those items.','','YesNo')");

This does probably not have any practical implications (hence severity = trivial), since the "explanation" is not used anymore and "options" is not used when the type is YesNo (I think?).
Comment 1 Paul Poulain 2012-03-27 07:56:50 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2012-04-07 06:04:38 UTC
(In reply to comment #1)
> Created attachment 8643 [details] [review]
> Bug 7726: explanation and options that were in the wrong order
> 
> I also removed the ` mysql-ism

If these files are to become non database specific, moving them out of the mysql dir makes sense. Removing mysqlisms from mysql specific files doesn't seem like a hugely useful thing to do. 

In this case it doesn't break anything, in the case of removing them all from the mysql/kohastructure.sql it renders that file unable to create a database. So a blanket policy of removing them under all circumstances is dangerous. Making updatedatabase.pl db agnostic makes sense, making the schemas .. not so much.
Comment 3 Chris Cormack 2012-04-09 08:28:34 UTC
Created attachment 9039 [details] [review]
Bug 7726: explanation and options that were in the wrong order

I also removed the ` mysql-ism

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 4 Ian Walls 2012-04-09 20:06:03 UTC
Updates an old DBRev to fix the issue, rather than updating the values in the existing database.  The result, for anyone who has upgraded since 3.07.00.012, will not fix their database values.

This is exceedingly minor, since these values are not displayed in the syspref editor, but better do fix this completely than only partially.

Marking Failed QA
Comment 5 Paul Poulain 2012-04-12 08:47:18 UTC
(In reply to comment #4)
> Updates an old DBRev to fix the issue, rather than updating the values in
> the existing database.  The result, for anyone who has upgraded since
> 3.07.00.012, will not fix their database values.
> 
> This is exceedingly minor, since these values are not displayed in the
> syspref editor, but better do fix this completely than only partially.
> 
> Marking Failed QA

I this had been released I would agree. But since it hasn't, I disagree.
And:
* I don't have time to provide a new patch
* releasing with this mistake (in less than 2 weeks) would be a shame

So, I think this patch should pass QA, pushed, and I can move it to "assigned" again, for a more complete fix later.
Comment 6 Marcel de Rooy 2012-04-16 07:19:55 UTC
QA Comment:
I would not object to push it. It resolves a bug (the sooner, the better). I agree with Ian that you could also send a followup to fix installations that already past this point.
Passed QA
Comment 7 Paul Poulain 2012-04-19 09:33:01 UTC
Patch pushed to master before 3.8 release.

Switching back to ASSIGNED though, to provide a fix for installations already updated
Comment 8 Katrin Fischer 2014-11-09 18:13:07 UTC
This has been pushed to 3.8, we didn't get any complaints and it would only have affected people installing an unstable release for a short amount of tim - I am closing this.