Each time a developer adds a new syspref, he must fill installer/data/mysql/sysprefs.sql. When a patch modifying this file is pushed, the patch does not apply anymore and the developer has to rebase his patch and resubmit it. If this file is sorted by syspref names, conflicts will appear less often.
Created attachment 19763 [details] [review] Bug XXXXX: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous.
Created attachment 19764 [details] [review] Bug 10610: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous.
Applying: Bug 10610: sysprefs.sql should be sorted by syspref names fatal: sha1 information is lacking or useless (installer/data/mysql/sysprefs.sql). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 10610: sysprefs.sql should be sorted by syspref names
Created attachment 20275 [details] [review] iBug 10610: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous.
I got 414 with the current script (master a36b3ad43a6a91f800d0d8a148a217d738236d7e), and 416 with the patch. Can you please check, and if you don't get the same I'll dump and compare.
Created attachment 20323 [details] [review] iBug 10610: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous.
(In reply to Srdjan Jankovic from comment #5) > I got 414 with the current script (master > a36b3ad43a6a91f800d0d8a148a217d738236d7e), and 416 with the patch. Can you > please check, and if you don't get the same I'll dump and compare. Oops, I suspect I was not on master when I generate the file.
Created attachment 20359 [details] [review] iBug 10610: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous. Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 20408 [details] [review] iBug 10610: sysprefs.sql should be sorted by syspref names By sorting the sql syspref file, the developpers will be happy to avoid conflicts on this file each time a patch modifying it is pushed. Test plan: - create a new DB with a new table named systempreferences (with the same structure as the one you know). - insert the current sysprefs.sql file into this table. - note the number of rows in the systempreferences table. - apply this patch. - delete all rows of the systempreferences table. - insert the new sysprefs.sql file. - verify the number of rows is the same as the previous. Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Excellent idea! I wonder if there is any way to add a check for out of order sysprefs in koha-qa.pl, after this patch has been pushed.
(In reply to Kyle M Hall from comment #10) > Excellent idea! I wonder if there is any way to add a check for out of order > sysprefs in koha-qa.pl, after this patch has been pushed. Yep, it should certainly be possible to do something.
(In reply to Jonathan Druart from comment #11) > (In reply to Kyle M Hall from comment #10) > > Excellent idea! I wonder if there is any way to add a check for out of order > > sysprefs in koha-qa.pl, after this patch has been pushed. > > Yep, it should certainly be possible to do something. I like the idea of this patch, and will push it shortly unless I find something wrong, but I'm opposed to putting a sorting requirement into koha-qa.pl. Sorting the script provides a small convenience by reducing the chance of merge conflicts, but doesn't per se provide a functional benefit. I'd really hate to see a patch fail QA on account of a new syspref getting inserted in the "wrong" place.
(In reply to Galen Charlton from comment #12) > (In reply to Jonathan Druart from comment #11) > > (In reply to Kyle M Hall from comment #10) > > > Excellent idea! I wonder if there is any way to add a check for out of order > > > sysprefs in koha-qa.pl, after this patch has been pushed. > > > > Yep, it should certainly be possible to do something. > > I like the idea of this patch, and will push it shortly unless I find > something wrong, but I'm opposed to putting a sorting requirement into > koha-qa.pl. Sorting the script provides a small convenience by reducing the > chance of merge conflicts, but doesn't per se provide a functional benefit. > I'd really hate to see a patch fail QA on account of a new syspref getting > inserted in the "wrong" place. I rather imagined that as a warning than a blocker stuff. The change could be made by the QAer with a small followup if the pref is not inserted in the good place.
Agreed, it's like horseshoes and hand-grenades. "Close enough" will do just fine. Kyle (In reply to Jonathan Druart from comment #13) > (In reply to Galen Charlton from comment #12) > > (In reply to Jonathan Druart from comment #11) > > > (In reply to Kyle M Hall from comment #10) > > > > Excellent idea! I wonder if there is any way to add a check for out of order > > > > sysprefs in koha-qa.pl, after this patch has been pushed. > > > > > > Yep, it should certainly be possible to do something. > > > > I like the idea of this patch, and will push it shortly unless I find > > something wrong, but I'm opposed to putting a sorting requirement into > > koha-qa.pl. Sorting the script provides a small convenience by reducing the > > chance of merge conflicts, but doesn't per se provide a functional benefit. > > I'd really hate to see a patch fail QA on account of a new syspref getting > > inserted in the "wrong" place. > > I rather imagined that as a warning than a blocker stuff. The change could > be made by the QAer with a small followup if the pref is not inserted in the > good place.
Pushed to master, thanks Jonathan! I'm OK with adding a warning to qa-tools provided that it never becomes a blocker.
I added a new test for qa tools: http://git.koha-community.org/gitweb/?p=qa-test-tools.git;a=commit;h=fa56a572468d7582c20be0149c16ec11b1970deb