Bug 7167 - updatedatabase improvements
Summary: updatedatabase improvements
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P1 - high new feature (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 6836 7119 9191
Blocks: 9327 6190 6328 9326 9328
  Show dependency treegraph
 
Reported: 2011-11-05 11:21 UTC by Paul Poulain
Modified: 2015-06-04 23:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Large patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
New version for updatedatabase (28.89 KB, patch)
2011-11-23 14:21 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 follow-up (24.65 KB, patch)
2011-11-23 14:22 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 testing patch (1.78 KB, patch)
2011-11-23 14:23 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 testing patch (1.78 KB, patch)
2011-11-23 15:12 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 testing patch (1.78 KB, patch)
2011-11-25 22:22 UTC, Paul Poulain
Details | Diff | Splinter Review
New version for updatedatabase (28.89 KB, patch)
2011-11-25 22:23 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 follow-up (26.39 KB, patch)
2011-11-25 22:23 UTC, Paul Poulain
Details | Diff | Splinter Review
PDF of Paul's OO file for the OS challenged among us :-) (995.76 KB, application/pdf)
2011-11-30 18:17 UTC, Chris Nighswonger
Details
Bug 7167 testing patch [Signoff] (1.89 KB, patch)
2011-11-30 19:15 UTC, Chris Nighswonger
Details | Diff | Splinter Review
New version for updatedatabase [Signoff] (29.00 KB, patch)
2011-11-30 19:19 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167 follow-up [Signoff] (26.50 KB, patch)
2011-11-30 19:20 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167 follow-up 2 perlcritic & numbers display & partial apply depending on DEBUG (7.65 KB, patch)
2011-12-12 14:18 UTC, Paul Poulain
Details | Diff | Splinter Review
New version for updatedatabase (46.73 KB, patch)
2011-12-16 09:06 UTC, Paul Poulain
Details | Diff | Splinter Review
New version for updatedatabase (46.43 KB, patch)
2012-02-27 15:34 UTC, Paul Poulain
Details | Diff | Splinter Review
New version for updatedatabase (46.45 KB, patch)
2012-03-12 11:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167 testing patch (1.77 KB, patch)
2012-05-31 13:53 UTC, Paul Poulain
Details | Diff | Splinter Review
New version for updatedatabase (46.22 KB, patch)
2012-05-31 13:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167, tiny display change, separating koha number and DBrev applied (4.03 KB, patch)
2012-05-31 13:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 follow-up display tweaking (3.99 KB, patch)
2012-05-31 13:53 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167, tiny display change, separating koha number and DBrev applied (4.08 KB, patch)
2012-06-09 14:38 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 7167 follow-up display tweaking (4.05 KB, patch)
2012-06-09 14:40 UTC, Chris Cormack
Details | Diff | Splinter Review
New version for updatedatabase (58.46 KB, patch)
2012-06-28 15:37 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Now, we check versions on mainpage.pl and after login (6.30 KB, patch)
2012-07-06 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Delete 3.9 versions in the old updatedatabase.sql (42.47 KB, patch)
2012-07-06 10:55 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Improve the update.pl script (7.00 KB, patch)
2012-07-06 10:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Squashed patch with some amendments (52.60 KB, patch)
2012-07-13 16:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Amended patch (52.78 KB, patch)
2012-07-25 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (45.96 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Now, we check versions on mainpage.pl and after login (6.35 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Improve the update.pl script (7.08 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Reimplementing Marcel's suggestions & fixes (2.53 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Remove check_coherency (8.56 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Changing in .sql parsing (1.42 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (5.44 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Improve display + factorize get_queries (17.18 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: FIX typos & moving a script to a proper place (16.04 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Add dependency File::Find::Rule (807 bytes, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: We want to execute non-numeric version with the -all option (1.99 KB, patch)
2012-07-31 16:06 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (45.96 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Now, we check versions on mainpage.pl and after login (6.35 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Improve the update.pl script (7.08 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Reimplementing Marcel's suggestions & fixes (2.53 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Remove check_coherency (8.56 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Changing in .sql parsing (1.42 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (5.44 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Improve display + factorize get_queries (17.18 KB, patch)
2012-08-01 09:42 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: FIX typos & moving a script to a proper place (16.04 KB, patch)
2012-08-01 09:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Add dependency File::Find::Rule (807 bytes, patch)
2012-08-01 09:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: We want to execute non-numeric version with the -all option (1.99 KB, patch)
2012-08-01 09:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 reindentation & removing dead code (2.65 KB, patch)
2012-08-01 09:43 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Don't raise an error in routine TableExists (954 bytes, patch)
2012-08-13 08:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (46.07 KB, patch)
2012-11-13 15:48 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Don't raise an error in routine TableExists (989 bytes, patch)
2012-11-13 15:52 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (46.07 KB, patch)
2012-11-13 15:53 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Now, we check versions on mainpage.pl and after login (6.39 KB, patch)
2012-11-13 15:57 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Improve the update.pl script (7.13 KB, patch)
2012-11-13 15:58 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Reimplementing Marcel's suggestions & fixes (2.57 KB, patch)
2012-11-13 15:58 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Remove check_coherency (8.60 KB, patch)
2012-11-13 15:58 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Changing in .sql parsing (1.46 KB, patch)
2012-11-13 15:59 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (5.48 KB, patch)
2012-11-13 16:00 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Improve display + factorize get_queries (17.22 KB, patch)
2012-11-13 16:00 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: FIX typos & moving a script to a proper place (16.08 KB, patch)
2012-11-13 16:00 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Add dependency File::Find::Rule (851 bytes, patch)
2012-11-13 16:01 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: We want to execute non-numeric version with the -all option (2.03 KB, patch)
2012-11-13 16:01 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167 reindentation & removing dead code (2.69 KB, patch)
2012-11-13 16:01 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Don't raise an error in routine TableExists (989 bytes, patch)
2012-11-13 16:02 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Screenshot of the ugly search header after rebasing change set (76.71 KB, image/png)
2012-11-14 20:53 UTC, Chris Nighswonger
Details
Bug 7167: FIX merge (706 bytes, patch)
2012-11-15 08:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (4.93 KB, patch)
2012-11-19 11:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (46.13 KB, patch)
2012-11-19 15:34 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Improve the update.pl script (7.19 KB, patch)
2012-11-19 15:36 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Now, we check versions on mainpage.pl and after login (6.45 KB, patch)
2012-11-19 15:37 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Remove check_coherency (8.66 KB, patch)
2012-11-19 15:45 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Changing in .sql parsing (1.52 KB, patch)
2012-11-19 15:45 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (5.54 KB, patch)
2012-11-19 15:45 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Improve display + factorize get_queries (17.28 KB, patch)
2012-11-19 15:45 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: FIX typos & moving a script to a proper place (16.15 KB, patch)
2012-11-19 15:46 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Add dependency File::Find::Rule (915 bytes, patch)
2012-11-19 15:46 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: We want to execute non-numeric version with the -all option (2.09 KB, patch)
2012-11-19 15:46 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: reindentation & removing dead code (2.76 KB, patch)
2012-11-19 15:46 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Don't raise an error in routine TableExists (1.03 KB, patch)
2012-11-19 15:47 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: FIX merge (809 bytes, patch)
2012-11-19 15:47 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Add .pl and .sql examples (2.19 KB, patch)
2012-11-19 15:47 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (67.02 KB, patch)
2012-11-28 13:50 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167 follow-up setting DBrevision, before pushing (1.36 KB, patch)
2012-11-28 13:50 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 7167: Adds Unit tests for C4::Update::Database (9.19 KB, patch)
2012-12-03 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Followup: Removes useless global variables (1.72 KB, patch)
2012-12-03 15:39 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Adds Unit tests for C4::Update::Database (8.54 KB, patch)
2012-12-03 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (67.06 KB, patch)
2012-12-03 17:07 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167 follow-up setting DBrevision, before pushing (1.47 KB, patch)
2012-12-03 17:08 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Followup: Removes useless global variables (1.82 KB, patch)
2012-12-03 17:08 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167: Adds Unit tests for C4::Update::Database (8.65 KB, patch)
2012-12-03 17:08 UTC, Chris Nighswonger
Details | Diff | Splinter Review
Bug 7167 follow-up: allow Makefile.PL to work (3.49 KB, patch)
2012-12-28 11:18 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: New version for updatedatabase (66.66 KB, patch)
2013-03-14 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167 follow-up setting DBrevision, before pushing (1.43 KB, patch)
2013-03-14 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Followup: Removes useless global variables (1.78 KB, patch)
2013-03-14 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167: Adds Unit tests for C4::Update::Database (8.61 KB, patch)
2013-03-14 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 7167 follow-up: allow Makefile.PL to work (3.49 KB, patch)
2013-03-14 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 7167 follow-up: allow Makefile.PL to work (3.56 KB, patch)
2013-03-21 10:41 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2011-11-05 11:21:16 UTC
During the hackfest in Mumbaï, 5 of us had a brainstorming about
database changes to try to improve it.
Here are the goals:
* must result in patches being easier to test by testers (to sign-off)
* must result in less conflict when trying to apply a patch with a DB
update when another patch has been applied (the .XXXX problem)
* must fix the linear problem (where patches are applied in a completly
linear way)
* be easy to implement


We throwed many ideas, here is the one that seems both easy and usefull :
* patches would be submitted in a specific file, in atomicupdate directory
* a new file (YAML) would be added, that would have 2 columns, one for
the version number, one for the atomicupdate file name
* updatedatabase wouldn't change anymore: we would just add something like :
LoadYAMLFile()
ForEachLine {
    if (C4::Context->preference("Version") < FirstColumnYAML) {
        exec(secondColumnYAML);
        SetVersion (FirstColumnYAML);
    }
}
The YAML file would be updated only by the Release Manager, when he
pushes a patch.

This idea fully achieve most of our initial goals:
* (very) easy to implement
* it will be easier to test a patch : is there an atomic update in the
patch ? OK, to test it, i'll first execute the file
* there won't be any conflict anymore of a patch applied and your .XXXX
file not applying anymore, needing a rebase
* it fixes partially the linear problem = if you have a local branch,
you can see what has been applied just looking in the yaml file.
Let's say Limoges library has sponsored stuff that has resulted in
atomicupdate/limogeupdate.pl
Those changes are live in Limoges, but still not in official/community
version.
The Limoges YAML file will be:
3.07.0001 Community_change1.pl
3.07.0002 community_change2.pl
localchange limogesupdate.pl
3.07.0003 community_change3.pl
the "localchange" in first column will mean for the updater : "OK, apply
the change but don't change the version number"

When the localchange has been applied on master community branch, the
official/community YAML file will look like:
3.07.0001 Community_change1.pl
3.07.0002 community_change2.pl
3.07.0003 community_change3.pl
3.07.0004 limogesupdate.pl

Switching back limoges to a community version would require some manual
checking, but that will be *much* easier than what we have today :
* diff limoges.yaml official.yaml
* apply community changes if/where needed
* UPDATE systempreferences SET kohaversion=3.07.0004 manually
It's done !

And if the localchange that has been made is never merged into master,
whatever the reason, it will be easier to keep track of it too.

And if you want to apply a submitted patch to your local setup before
it's pushed onto master, you can, just by running the file in
atomicupdate directory. And add a "localchange" line to your YAML file
to remember the change you've made.
Comment 1 Paul Poulain 2011-11-23 14:21:31 UTC Comment hidden (obsolete)
Comment 2 Paul Poulain 2011-11-23 14:22:53 UTC Comment hidden (obsolete)
Comment 3 Paul Poulain 2011-11-23 14:23:08 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-11-23 15:12:39 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2011-11-23 15:19:23 UTC
The 3 patch attached differs from what has been announced. 90% of the work had already been done by Jonathan, so I keep his work.

Here is how it works:
* each database update is stored in a numbered file, in installer/data/mysql/versions
* database updates can be .sql or .pl files. 2 skeletons are provided to explain how it works
* there is no more automatic checking of the database update on each page. The librarian/sysadmin must go to the admin/updatedatabase.pl page on each update.
* the updatedatabase.pl script keep track of all updates that have been applied, how it went. And it's non-linear: 3.7.2.3 can be applied after 3.7.2.4
* The about.pl will display the highest db update applied, but maybe there are some missing, it's only the highest applied


TEST PLAN:
* install a 3.6.x,
* checkout master branch
* apply the 3 patches attached to this bug
* go to admin > updatedatabase (last line on the right). You'll see 3 upgrades (that are fakes, they are here just for testing purposes)
* install the 1st one. It should say "OK"
* check about the version is now 3.07.01.001
* install the 2nd one. It should say "OK and about should say 3.07.01.002
* install the 3rd one. It should say "already exist", you can force OK, version should say 3.07.01.003

do it again:
* install a 3.6.x,
* checkout master branch
* apply the 3 patches attached to this bug
* go to admin > updatedatabase (last line on the right). You'll see 3 upgrades (that are fakes, they are here just for testing purposes)
* install the 2st one. It should say "OK"
* check about the version is now 3.07.01.002
* install the 1nd one. It should say "OK and about should still say 3.07.01.002
* install the 3rd one. It should say "already exist", and you can force OK, version should say 3.07.01.004

do it again:
* install a 3.6.x,
* checkout master branch
* apply the 3 patches attached to this bug
* go to admin > updatedatabase (last line on the right). You'll see 3 upgrades (that are fakes, they are here just for testing purposes)
* install all of them by clicking UPDATE =>  it should say "OK" for 2 and "not OK" for the 3rd one, and version should be 3.07.01.003

do it again:
* install a master
* apply the 3 patches attached to this bug
* go to admin > updatedatabase (last line on the right). You'll see 3 upgrades (that are fakes, they are here just for testing purposes)
* Koha should say that they have already been applied, because they were here on initial install
Comment 6 Sébastien Marie 2011-11-23 17:09:38 UTC
(In reply to comment #5)

> And it's non-linear: 3.7.2.3 can be applied after 3.7.2.4

Just a question about non-linear update. How is handle dependencies between patchs ? The admin should just take care when update his database in non linear order ?



Else, there is a FIXME in "admin/updatedatabase.pl" about permission (FIXME: add a new flag)
Comment 7 Paul Poulain 2011-11-25 17:13:49 UTC
new patches, with explanations coming soon, removing "patch sent"
Comment 8 Paul Poulain 2011-11-25 22:22:53 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2011-11-25 22:23:05 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2011-11-25 22:23:15 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2011-11-25 22:29:43 UTC
There is a document explaining how it works available at: https://depot.biblibre.com/ppoulain/updatedb%20for%203.8.odt
Comment 12 Chris Nighswonger 2011-11-30 17:55:37 UTC
Just a note for testing clarification:

Apply all three patches in the order they are attached to this bug report.
Comment 13 Chris Nighswonger 2011-11-30 17:58:53 UTC
This appears to add a new dependency: Modern::Perl
Comment 14 Ian Walls 2011-11-30 18:01:46 UTC
Modern::Perl is a desirable dependency to add, but yes, it needs to be explicitly included in the Perl deps and Debian packages before this can pass QA.
Comment 15 Chris Nighswonger 2011-11-30 18:17:26 UTC Comment hidden (obsolete)
Comment 16 Katrin Fischer 2011-11-30 18:58:34 UTC
Sorry, I have not run through it yet, but wondering about the screenshot in the pdf document, showing the about page.

I think displaying all applied database updates there will very soon be very confusing, making this page hard to read.

I think stable versions should still have a version number in the systempreferences table? Why not display that number there and a link to the new update page behind that? Or perhaps:

<Version number from systempreferences ...<last applied update> <link> ?
Comment 17 Chris Nighswonger 2011-11-30 19:06:10 UTC
(In reply to comment #16)
> I think displaying all applied database updates there will very soon be very
> confusing, making this page hard to read.

I agree. Space will quickly run out there.

Perhaps just showing the most recent stable version and then provide a link back to the admin/updatedatabase.pl page would do.
Comment 18 Chris Nighswonger 2011-11-30 19:07:07 UTC
admin/updatedatabase.pl needs tablesorters badly. This list will quickly become long and unruly. Being able to sort on the "status" column will be important.
Comment 19 Chris Nighswonger 2011-11-30 19:15:15 UTC Comment hidden (obsolete)
Comment 20 Chris Nighswonger 2011-11-30 19:19:30 UTC Comment hidden (obsolete)
Comment 21 Chris Nighswonger 2011-11-30 19:20:14 UTC Comment hidden (obsolete)
Comment 22 Chris Nighswonger 2011-11-30 21:11:51 UTC
(In reply to comment #18)
> admin/updatedatabase.pl needs tablesorters badly. This list will quickly become
> long and unruly. Being able to sort on the "status" column will be important.

OK, I just noted the use of Datatables here and applied the patch. This addresses this concern. But the cosmetics need help. :)
Comment 23 Chris Nighswonger 2011-11-30 21:14:53 UTC
Signing off. These patches work as advertised. The cosmetic issue can be easily addressed in a subsequent patch or two. Functionally things work fine as far as I can tell.
Comment 24 Paul Poulain 2011-12-01 07:54:34 UTC
(In reply to comment #22)
> (In reply to comment #18)
> > admin/updatedatabase.pl needs tablesorters badly. This list will quickly become
> > long and unruly. Being able to sort on the "status" column will be important.
> 
> OK, I just noted the use of Datatables here and applied the patch. This
> addresses this concern. But the cosmetics need help. :)

Seems you've missed comment 1: datatable is already in this patch. But until the datatable bug has been signed-off/pushed, you won't see it, and just get a full table. I add the dependancy (bug 6836)



(In reply to comment #16)
> I think displaying all applied database updates there will very soon be very
> confusing, making this page hard to read.
I was sure someone would rise this problem ;-) I think we could do something like X...Y, Z...T :
For example:
3.06.00.0001...3.06.00.005, 3.06.00.007...3.06.00.012, 3.07.00.001
I don't think this should prevent from pushing as it is today, it's just a matter of how we present things.

Note that I also think there are too many digits in our numbers, and with this new behaviour, we could simplify things. I'll drop a mail to koha-devel to start the thread of version number (was thinking of this since some days/weeks)
Comment 25 Marcel de Rooy 2011-12-05 12:36:50 UTC
Comments from some (nut full) testing (no QA):

I see 3.07.01.xxx in the db revs.
But what is 3.07.01 ? When do you reach 3.07.02 now? After you installed all 50 dbrevs that are marked with 3.07.01?
Is that mechanism implemented in current code? When and how are we stepping to 3.07.02 ? 

About/version: 3.0600002 / 3.07.01.0002 / 3.07.01.0003
Listing them all is not helpful. 
I would say list the version number: 3.07.01 meaning something like: everything below has been installed. And list the numbers of dbrevs installed for 3.07.02 and higher and the number pending. 
So this would be: 3.07.01 with 36 db revisions installed; 23 db revisions pending.
I agree that this kind of change is cosmetic but should follow quickly. 

Question: Do you allow someone to install dbrev 3.07.03.xxx while not all 3.07.02 are installed? I would say No. 
Code should check that. Can we test it? 

Non-superlibrarian could not login while not all updates were executed. But he should! 
Superlibrarian is able to login. Clicking homepage goes to update page, but clicking modules works fine. But I want homepage! I only like the warning if I am logging in. 
I assumed that we allowed the situation now that not all dbrevs are executed, but the code does not. Should be adjusted.
Comment 26 Katrin Fischer 2011-12-05 12:43:22 UTC
>I assumed that we allowed the situation now that not all dbrevs are executed,
>but the code does not. Should be adjusted.

I am not sure we should encourage that. It will make it really hard to find out what's going wrong. People should at least be forced to resolve their database updates. Like marking something 'OK' that did not apply, but was a duplicate message.
Comment 27 Marcel de Rooy 2011-12-05 14:10:38 UTC
> I am not sure we should encourage that. It will make it really hard to find out
> what's going wrong. People should at least be forced to resolve their database
> updates. Like marking something 'OK' that did not apply, but was a duplicate
> message.

With some discussion on IRC in mind, it might be interesting to add a preference (yet another ;) on the Admin page that defaults to Linear updating (and enforces to run all updates in sequence before using Koha). Only if Delinear would be selected (by a developer), allow selective updating and do not force running all updates before using Koha. Just remind an admin at login time.
Comment 28 Paul Poulain 2011-12-05 21:42:29 UTC
(In reply to comment #25)
> Comments from some (nut full) testing (no QA):
> 
> I see 3.07.01.xxx in the db revs.
> But what is 3.07.01 ? When do you reach 3.07.02 now? After you installed all 50
It's just here for testing purposes, and won't be pushed.
With this new system, the numbers don't matter. That's why I started a thread about that in koha-devel.
We could decide to mark all DB revs 3.08.01 immediatly, or use an increasing number, like 4, 5, 6, ...

> Listing them all is not helpful. 
Yep, I plan to add an improvement to display applied patches like this
3.07.01.001...3.07.01.015 (if we have applied all revs btw 001 and 0015)
and 3.07.01.001...3.07.01.015, 3.07.01.0017 (if we have applied all revs between 001 and 015, and the 017)

> So this would be: 3.07.01 with 36 db revisions installed; 23 db revisions
> pending.
> Question: Do you allow someone to install dbrev 3.07.03.xxx while not all
> 3.07.02 are installed? I would say No. 
> Code should check that. Can we test it? 

I've proposed, and will provide an improvement for that, the following behaviour:
* if DEBUG=0, then you will reach the update page from mainpage.pl until you've made all update. So you *can't* switch an update.
* if DEBUG=1, then you won't reach the update page automatically. For testers/testing purposes, it's a desired behaviour (improvement: today, you have only the DEBUG=0 behaviour, which is OK)

Thus, you'll get:
* non linear updates, with almost the same behaviour as today, for productions setups
* mandatory updates, except if you've DEBUG=1 (and in this case, you know what you're doing)
Comment 29 Paul Poulain 2011-12-12 14:18:33 UTC Comment hidden (obsolete)
Comment 30 Paul Poulain 2011-12-12 14:20:49 UTC
Comment on attachment 6722 [details] [review]
Bug 7167 follow-up 2 perlcritic & numbers display & partial apply depending on DEBUG

This follow-up deals with comment 28 :
> > Listing them all is not helpful. 
> Yep, I plan to add an improvement to display applied patches like this
> 3.07.01.001...3.07.01.015 (if we have applied all revs btw 001 and 0015)
> and 3.07.01.001...3.07.01.015, 3.07.01.0017 (if we have applied all revs
> between 001 and 015, and the 017)
> 
> > So this would be: 3.07.01 with 36 db revisions installed; 23 db revisions
> > pending.
> > Question: Do you allow someone to install dbrev 3.07.03.xxx while not all
> > 3.07.02 are installed? I would say No. 
> > Code should check that. Can we test it? 
> 
> I've proposed, and will provide an improvement for that, the following
> behaviour:
> * if DEBUG=0, then you will reach the update page from mainpage.pl until you've
> made all update. So you *can't* switch an update.
> * if DEBUG=1, then you won't reach the update page automatically. For
> testers/testing purposes, it's a desired behaviour (improvement: today, you
> have only the DEBUG=0 behaviour, which is OK)
> 
> Thus, you'll get:
> * non linear updates, with almost the same behaviour as today, for productions
> setups
> * mandatory updates, except if you've DEBUG=1 (and in this case, you know what
> you're doing)
Comment 31 Marcel de Rooy 2011-12-15 09:14:21 UTC
Just some comments from testing again:

The distinction between nonlinear and linear by using DEBUG is a good thing!

Patch 2 and 3 need rebasing. If you do, could you squash everything? Will make it easier for additional testers.

If the last db rev fails (as in your examples), you allow access to the staff client (in standard mode) and you are not going to update page again, I wonder if that's what you want. There is a problem and shouldn't it be solved first?

If in dev mode I run the third db rev only WHILE table already EXISTS, I feel that the returned message is not so clear. And if I do not mark it as OK and I come back at update page, I can still execute it again. Would it not be better to mark it as Failed?

Clicking Home in dev mode still brings me to update page (earlier comment!).

About says: 3.6.2.1 / 3.7.1.3, but I did not install 1 & 2 ? Personally, I do not prefer the A .. B, C .. D listing that is now possible too.

Since this is a larger change, I do not feel that my tests and [probably] those of Chris N really cover all situations. Wouldn't it be better to ask for some additional signoffs too?
Comment 32 Paul Poulain 2011-12-16 09:06:20 UTC Comment hidden (obsolete)
Comment 33 Paul Poulain 2011-12-16 09:12:32 UTC
(In reply to comment #31)
> Just some comments from testing again:
> Patch 2 and 3 need rebasing. If you do, could you squash everything? Will make
> it easier for additional testers.
done

> If the last db rev fails (as in your examples), you allow access to the staff
> client (in standard mode) and you are not going to update page again, I wonder
> if that's what you want. There is a problem and shouldn't it be solved first?
yes, it's what I wanted: if DB rev fails, it's usually because a patch has already been applied, or your DB structure differ because of some specific things you've made. It should not prevent you from running Koha. The option saying "mark as OK" must not be mandatory too, as it means "I know what I did"

So there are 3 options, that will be usefull for debugging if someone faces a bug:
* patch applied
* patch applied, but failed, and the user did nothing
* patch applied, failed, and the user dealed with it (and has set "mark OK")

> If in dev mode I run the third db rev only WHILE table already EXISTS, I feel
> that the returned message is not so clear. And if I do not mark it as OK and I
> come back at update page, I can still execute it again. Would it not be better
> to mark it as Failed?
> 
> Clicking Home in dev mode still brings me to update page (earlier comment!).
mmm... not sure I understand those lines.

> Since this is a larger change, I do not feel that my tests and [probably] those
> of Chris N really cover all situations. Wouldn't it be better to ask for some
> additional signoffs too?

I would say no: jonathan has tested a lot, it's live at BibLibre customers like StEtienne (without my follow-up though), i've tested it, chris_n and you too. That's enough. One can explain for years how to swimm, nothing will replace jumping in the pool (frenchism suspected... )
Comment 34 Marcel de Rooy 2011-12-19 10:34:16 UTC
In reply to Paul's last comment:

Thanks for responding. Well, I can live with its current form, but I feel that its development has not been completely finished ;)

About testing, it is good that Jonathan and you tested thoroughly as the developers of this feature, but strictly speaking you only have one external [developer] signoff (assuming that your customer is not really testing it..)
Lifting this particular patch to Passed QA should IMO be done by the QAM himself. Will mail this to Ian too.
Comment 35 Paul Poulain 2011-12-27 15:32:14 UTC
(In reply to comment #34)
> In reply to Paul's last comment:
> 
> Thanks for responding. Well, I can live with its current form, but I feel that
> its development has not been completely finished ;)
is it just a feeling or you've something specific in mind ?

> About testing, it is good that Jonathan and you tested thoroughly as the
> developers of this feature, but strictly speaking you only have one external
> [developer] signoff (assuming that your customer is not really testing it..)

well he's not testing, he is even not knowing he is using that ;-)
It's just that we use this system, and the customer is happy with the result.

agreed we have only 1 signoff & it's a strategic thing, so it's worth having a 2nd one. You could add it (QAing does not mean you can't signoff at the same time !), or Ian ?

Still, I consider this improvement as a must-pass-asap, and it should be on the top of the list for everybody !

> Lifting this particular patch to Passed QA should IMO be done by the QAM
> himself. Will mail this to Ian too.
Comment 36 Ian Walls 2012-01-06 13:27:28 UTC
I've been leery about this development for a few reasons:

1. it seems that, what we initially discussed for this is not what we got, as far as code.  There was some BibLibre work that was similar, so that was used instead of implementing the idea from Mumbaï in it's entirety.

2. making changes to the way database updates are handled effects all versions of Koha.  This process needs to remain stable at all costs.  Thus any change must be most thoroughly tested

3. there is a rush order on this, which makes me uncomfortable.  Yes, it would be nice to start the release cycle with a new method, but we're already well along the development timeframe for 3.8, and I think changing midstream is going to introduce more complications than it's worth

4. Frankly, I don't feel the need to rush this.  This project offers no particular benefits to the end user.  It's only really helpful to testers and developers.  I'm all for making it easier to test patches, but not at the risk of breaking upgrades for any set of Koha users.  I'd rather see this implemented in the next release cycle, so we have adequate time to plan and test.

5. Moving database updates into the atomic update directory is a good thing, for sure, but assigning them numbers makes them inherently linear, which we're trying to avoid.

6. I disagree with removing the Version check on every page.  We're in an asynchronous web environment; the system could be upgraded while folks are mid-stream in their work (not a recommended practice, but one that we need to acknowledge as possible).  If a database change significantly affects the structure of the area in which people are working, the information they are submitting/querying could become corrupted, or not work at all.  The code and the database must be kept synchronous, at all times, with no need for manual action.

Following is my counter-proposal for what a change to updatedatabase should look like.  Marking as Failed QA for now.
Comment 37 Ian Walls 2012-01-06 13:36:26 UTC
Counter proposal:

Just like we initially thought
* patches would be submitted in a specific file, in atomicupdate directory
* a new file (YAML) would be added, that would have 2 columns, one for
the version number, one for the atomicupdate file name
* updatedatabase wouldn't change anymore: we would just add something like :
LoadYAMLFile()
ForEachLine {
    if (C4::Context->preference("Version") < FirstColumnYAML) {
        exec(secondColumnYAML);
        SetVersion (FirstColumnYAML);
    }
}
The YAML file would be updated only by the Release Manager, when he
pushes a patch.

But additionally,

* Each atomic update would be named descriptively, not numerically.  Preferrably referencing the bug number
* The updates would all have the following 4 functions:
  CHECK: determines if the change is needed
  DESCRIBE: display the text for what this change is supposed to do
  DO:  execute the change
  UNDO:  revert the change
* CHECK would always be run first.  If the change is warranted, DESCRIBE would be run.  After a confirmation from the user, DO is run.
* Similar process could be implemented to downgrade Koha, using UNDO instead of DO.

This solves linearity, in that any update can DOne in any order.

It also makes reverting patches easier; if a patch you're testing contains a DB update, you can run the UNDO before switching back to master.  This prevents cumulative database corruption from testing DB-rev patches

Duplicate Key entries and other problems that could come from running an update twice are avoided.

Update descriptions can be viewed in advance, so you know what you're going to get.
Comment 38 Katrin Fischer 2012-01-06 13:47:44 UTC
I am really not sure that sequence for database update never matters. The nonlinear approach seems dangerous to me.
Comment 39 Paul Poulain 2012-01-06 13:58:24 UTC
(In reply to comment #38)
> I am really not sure that sequence for database update never matters. The
> nonlinear approach seems dangerous to me.

My proposal is, in fact, semi non-linear: all DBrevs are numbered, thus they have an order.
But each number can be applied individually.
It means that, if you apply DBrev 1 and 3, you can also, later, apply BDrev 2

It means that a DBrev that is not ready to be pushed don't block everything/everybody and you don't have to renumber everything or use temporary numberings (or the XXX system we use atm)
Comment 40 Paul Poulain 2012-01-06 14:00:36 UTC
(In reply to comment #37)
> Counter proposal:

Do you plan to submit a patch that implement this behaviour?

If the answer is no i'll answer to each of your points
Comment 41 Chris Nighswonger 2012-01-06 14:02:54 UTC
(In reply to comment #36)

Up front, I think Ian raises some very good points.

> I've been leery about this development for a few reasons:

<snip>

> Following is my counter-proposal for what a change to updatedatabase should
> look like.  Marking as Failed QA for now.

However, I'm not sure that the correct "mechanism" here is a "Failed QA." In my opinion QA is pretty objective. ie. does the code do precisely what it claims to do. None of the points listed indicate that the code submitted does not behave as advertised. They are all good points, but fall more into the pail of the "is this the best way to do it" discussion. Maybe I'm wrong in that thought. If so, feel free to correct me.

I fear this may set a precedent of QA "vetoing" features for reasons other than objective code related problems. Of course, refusing to QA a feature could result in a "pocket veto" of sorts as well. But I think that a conscientious RM should not push any feature to master about which serious concerns have been raised.

(For the record, this is not intended to be an indication of my opinion on the modus operendi of a new update system. It is merely a procedural objection.)
Comment 42 Ian Walls 2012-01-06 14:04:30 UTC
Paul,


I don't have a patch implementing my proposal yet, no, and it's unlikely I'll get one written any time this month or next.  But that doesn't stop others from taking the core idea, expounding on it, and implementing.
Comment 43 Ian Walls 2012-01-06 14:08:47 UTC
Chris N,

QA is not strictly assessing the validity of the code and it's conformity to our established coding guidelines.  It also takes into consideration larger implications of the code being submitted, the possibilities of regressions and the risks of introducing new bugs.  Perhaps I'm overstating the scope as others understand it, but this is my understanding.

That said, having another status to mark this other than "Failed QA" would be good, because the code itself isn't technically invalid.  Perhaps "Planning" or "In Discussion"... ?
Comment 44 Chris Nighswonger 2012-01-06 14:19:29 UTC
(In reply to comment #43)
> Chris N,
> 
> QA is not strictly assessing the validity of the code and it's conformity to
> our established coding guidelines.  It also takes into consideration larger
> implications of the code being submitted, the possibilities of regressions and
> the risks of introducing new bugs.  Perhaps I'm overstating the scope as others
> understand it, but this is my understanding.

I don't think that this is an overstatement of QA responsibilities. I just think that QA rejection should include concrete, demonstration issues. ie. What are the exact regressions/bugs along with examples of exactly how those will manifest. If the code is demonstrably buggy or regression-causing, it should fail QA. It should not fail QA on issues which are anticipated/suspected, but no confirmed IMHO.

> 
> That said, having another status to mark this other than "Failed QA" would be
> good, because the code itself isn't technically invalid.  Perhaps "Planning" or
> "In Discussion"... ?

Such a choice would be nice. "In Discussion" would be an unambiguous tag.
Comment 45 Paul Poulain 2012-01-06 14:38:55 UTC
(In reply to comment #41)
> However, I'm not sure that the correct "mechanism" here is a "Failed QA." In my
> opinion QA is pretty objective. ie. does the code do precisely what it claims
> to do. None of the points listed indicate that the code submitted does not
> behave as advertised. They are all good points, but fall more into the pail of
> the "is this the best way to do it" discussion. Maybe I'm wrong in that
> thought. If so, feel free to correct me.

Thanks chris_n !
That was my 1st reaction, but I didn't wanted to write it immediatly to avoid being suspected to refuse the discussion.
I agree that QA should be objective.
OTOH, Ian & me had a long discussion about this improvement at KohaCon, and I consider his thoughts more a follow-up of this discussion than a QA.

Agreed we should have a "discussion" status !

Now answering Ian points:
(In reply to comment #36)
> 1. it seems that, what we initially discussed for this is not what we got, as
> far as code.  There was some BibLibre work that was similar, so that was used
> instead of implementing the idea from Mumbaï in it's entirety.
You're right here. That does not mean it's not a good piece of code.

> 2. making changes to the way database updates are handled effects all versions
> of Koha.  This process needs to remain stable at all costs.  Thus any change
> must be most thoroughly tested
you're right, and we (jonathan & me) have spent a *lot* of time testing this one.
I also must point that this patch is not exclusive of the actual DBrev system : both are still working. It means that patches already submitted with updatedatabase can still be tested/pushed

> 3. there is a rush order on this, which makes me uncomfortable.  Yes, it would
> be nice to start the release cycle with a new method, but we're already well
> along the development timeframe for 3.8, and I think changing midstream is
> going to introduce more complications than it's worth

The rush level has been lowered by the clarification made on DBrevs, and the mails i've sent today (as well as the update of master i also made today)

But:
* the 3.6 updatedatabase has a limit for testing patches as well as a risk for backporting DBrevs to 3.4, addressed by this patch
* this bug is an absolute necessity for the sandbox system i'm working on. Without it, it won't work (because updatedatabase have a XXX, so no patch with a DBrev will apply)

> 4. Frankly, I don't feel the need to rush this.  This project offers no
> particular benefits to the end user.  It's only really helpful to testers and
> developers.  I'm all for making it easier to test patches, but not at the risk
> of breaking upgrades for any set of Koha users.  I'd rather see this
> implemented in the next release cycle, so we have adequate time to plan and
> test.

Does it mean your only fear is that it could break something ? and you haven't tested or found anything broken ?
Yes, it's a core change, but we should not fear doing core change just because it's in the core !

> 5. Moving database updates into the atomic update directory is a good thing,
> for sure, but assigning them numbers makes them inherently linear, which we're
> trying to avoid.
yes and no : the numbering ensure we have an order, but it's not linear because we can have holes in the numbering.

> 6. I disagree with removing the Version check on every page.  We're in an
> asynchronous web environment; the system could be upgraded while folks are
> mid-stream in their work (not a recommended practice, but one that we need to
> acknowledge as possible).  If a database change significantly affects the
> structure of the area in which people are working, the information they are
> submitting/querying could become corrupted, or not work at all.  The code and
> the database must be kept synchronous, at all times, with no need for manual
> action.
we disagree here : upgrading a software is not something you do without knowing you're doing it. There is a procedure, and the check has been reintroduced on every mainpage, It's enough !
Note that, if you continue to argue it must be done, then i've a counter proposal:
at the end of each upgrade, run truncate session, thus the user will be disconnected, and, on connection, he will be sent to database update !
Comment 46 Paul Poulain 2012-02-24 11:20:56 UTC
I tried to write a page for starting discussion on the wiki, but I can't find what is to be discussed.

We have:
* a patch with a new method to update database, that solve most problems (linearity and frequent merge conflicts that requires a rebase). Problems raised by testers have been solved.
* a counter-proposal that has no patch, and won't have in a defined future.

So, In my opinion, this patch should just be reseted to "signed-off" QAed. If some functionnal real problem is raised, then deal with it.

But "there's another way to do it" is not a reason to stop this patch. There are zillions of cases where "there's another way to do it", (probably for all ENH)

Switching back to "signed-off" -will check it still apply asap-
Comment 47 Chris Nighswonger 2012-02-24 13:08:49 UTC
For the record: I think that this feature will need to be QA'd by someone completely unassociated with Biblibre before being pushed to master.
Comment 48 Paul Poulain 2012-02-24 13:20:08 UTC
(In reply to comment #47)
> For the record: I think that this feature will need to be QA'd by someone
> completely unassociated with Biblibre before being pushed to master.

of course. So we've Marcel and Ian as possible QAers
Comment 49 Marcel de Rooy 2012-02-25 18:41:01 UTC
Applying: New version for updatedatabase
warning: C4/Auth.pm has type 100644, expected 100755
error: patch failed: C4/Auth.pm:35
error: C4/Auth.pm: patch does not apply
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css:2133
error: koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css: patch does not apply
error: patch failed: mainpage.pl:29
error: mainpage.pl: patch does not apply
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merged C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Auto-merged installer/data/mysql/kohastructure.sql
Auto-merged koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css
Auto-merged mainpage.pl
CONFLICT (content): Merge conflict in mainpage.pl
Failed to merge in the changes.
Comment 50 Paul Poulain 2012-02-27 15:34:05 UTC Comment hidden (obsolete)
Comment 51 Paul Poulain 2012-02-27 15:40:20 UTC
Rebased version uploaded.

The rebase was easy:
* trivial conflict in mainpage.pl (consequence of the removal of authtypeloop in bug 6875)
* trivial conflict in C4/Auth.pm (consequence of the memcache moved to ENV stuff)
* trivial conflict in css

HOWEVER = setting this patch back to "assigned", and asking a question to Jonathan, the original author

Jonathan, 2 questions:
* why is there something in staff-global.css:
+tr.dragClass td {
+    background-color: grey;
+    color: yellow;
+}
+.underline {
+    text-decoration : underline;
+}

grep -R "dragClass" * show this class is used nowhere in the code.

* the display datatable is a little bit strange, probably a consequence of bug 6836 changes that you haven't managed
Comment 52 Jonathan Druart 2012-02-28 08:45:42 UTC
Paul, the "dragClass" class in css file was not in my original patch. You can remove this declaration.

If table in admin/updatedatabase is different from others datatables, it is probably owed to modification of bug 6836.
Comment 53 Paul Poulain 2012-03-12 10:09:48 UTC
(In reply to comment #52)
> Paul, the "dragClass" class in css file was not in my original patch. You
> can remove this declaration.
> 
> If table in admin/updatedatabase is different from others datatables, it is
> probably owed to modification of bug 6836.

coud you take care of the follow-up ? (I can't find time for it myself...)
Comment 54 Jonathan Druart 2012-03-12 11:19:51 UTC Comment hidden (obsolete)
Comment 55 Paul Poulain 2012-05-31 13:53:19 UTC Comment hidden (obsolete)
Comment 56 Paul Poulain 2012-05-31 13:53:25 UTC Comment hidden (obsolete)
Comment 57 Paul Poulain 2012-05-31 13:53:30 UTC Comment hidden (obsolete)
Comment 58 Paul Poulain 2012-05-31 13:53:33 UTC Comment hidden (obsolete)
Comment 59 Paul Poulain 2012-05-31 13:58:43 UTC
Those 3 patches have been rebased against HEAD, and a tiny follow-up for tweaking display has been added.

Could someone (jonathan ?) signoff the new follow-up (http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=9862).

Note : this patch is very important for having sandboxes being efficients, as for now, they fail on each patch that has an updatedatabase.

Chris_n has signed the initial patch, it's still valid, the rebase was trivial.

So once the follow-up is signed-off, we can go to QA I think !

thanks
Comment 60 Chris Cormack 2012-06-09 14:38:19 UTC Comment hidden (obsolete)
Comment 61 Chris Cormack 2012-06-09 14:40:41 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2012-06-11 12:59:51 UTC
Some question to revive the topic (for me) before starting QA (..), also due to the fact that these patches deviated from the initial starting point as described.

Commit message talks about koha-conf.xml changes? Is that obsolete?
What is the exact use of the yaml file now? Only listing the versions directory? Or also for a new db revision number?
If I recall correctly, you could switch modes via $ENV{DEBUG} in koha-httpd, but it only seems now to hide the Execute button in normal mode. Am I correct?
Wasn't it meant to enable/disable linear vs non-linear updates?
Comment 63 Marcel de Rooy 2012-06-11 13:14:16 UTC
Argument "test01" isn't numeric in numeric comparison (<=>) at /usr/share/koha/testclone/admin/updatedatabase.pl line 88
Comment 64 Paul Poulain 2012-06-11 14:18:20 UTC
(In reply to comment #62)
> Commit message talks about koha-conf.xml changes? Is that obsolete?
If you're talking of:
    add a section
     <installdir>/path/to/your/install/dir</installdir>
    in your koha-conf.xml
I'll ask jonathan to reupload a patch with the commit message fixed & other QA comments if there are.
> What is the exact use of the yaml file now? Only listing the versions
> directory? 
Yes
> Or also for a new db revision number?
Nope. They are stored in new tables (updatedb_error, updatedb_query, updatedb_report)
> If I recall correctly, you could switch modes via $ENV{DEBUG} in koha-httpd,
> but it only seems now to hide the Execute button in normal mode. Am I
> correct?
> Wasn't it meant to enable/disable linear vs non-linear updates?
??? I don't understand what you mean here.


(In reply to comment #63)
> Argument "test01" isn't numeric in numeric comparison (<=>) at
> /usr/share/koha/testclone/admin/updatedatabase.pl line 88

Can you explain what you made ? I suspect you have added a test SQL in installer/data/mysql/versions/ and called it test01.sql. It's wrong, it must be a number 3.09.00.014 for example (could also be 123.456, but it must be numbers+. only)
Comment 65 Marcel de Rooy 2012-06-11 18:42:34 UTC
(In reply to comment #64)
> > If I recall correctly, you could switch modes via $ENV{DEBUG} in koha-httpd,
> > but it only seems now to hide the Execute button in normal mode. Am I
> > correct?
> > Wasn't it meant to enable/disable linear vs non-linear updates?
> ??? I don't understand what you mean here.
If you set DEBUG=1, what is the difference? Seems to me only hiding the Execute button. I see that it indeed prevents non-linear updating. Question solved more or less; will come back to it in QA.

> Can you explain what you made ? I suspect you have added a test SQL in
> installer/data/mysql/versions/ and called it test01.sql. It's wrong, it must
> be a number 3.09.00.014 for example (could also be 123.456, but it must be
> numbers+. only)
Yes, I made that file. Instead of such an error message, I would propose just to ignore files that do not match the numbering scheme. Perhaps mark them as inappropriate on the new updatedatabase form, or so. (In a folder you can always expect files with other names..) Will return to that too soon.

Thanks.
Comment 66 Paul Poulain 2012-06-13 10:57:24 UTC
(In reply to comment #65)
> (In reply to comment #64)
> > > If I recall correctly, you could switch modes via $ENV{DEBUG} in koha-httpd,
> > > but it only seems now to hide the Execute button in normal mode. Am I
> > > correct?
> > > Wasn't it meant to enable/disable linear vs non-linear updates?
> > ??? I don't understand what you mean here.
> If you set DEBUG=1, what is the difference? Seems to me only hiding the
> Execute button. I see that it indeed prevents non-linear updating. Question
> solved more or less; will come back to it in QA.
OK, got it !

The idea behind this was that, in production environment, there is no reason not to apply all patches. So if DEBUG=0, having only the option to apply all update sounds OK.
If DEBUG=1, a *developer* can find some reason not to apply everything.

> > Can you explain what you made ? I suspect you have added a test SQL in
> > installer/data/mysql/versions/ and called it test01.sql. It's wrong, it must
> > be a number 3.09.00.014 for example (could also be 123.456, but it must be
> > numbers+. only)
> Yes, I made that file. Instead of such an error message, I would propose
> just to ignore files that do not match the numbering scheme. Perhaps mark
> them as inappropriate on the new updatedatabase form, or so. (In a folder
> you can always expect files with other names..) Will return to that too soon.
If someone submit a patch including a file with a wrong name it won't pass QA, that's as simple as that, I'm not sure it's needed to check for the format.

Also note that, when the patch will be pushed, i'll start a wiki page where ppl, when submitting a patch with a DB update, will be able to mark/reserve a number to avoid having a conflict. And if a number does not make his way into Koha finally, that's not a big problem, we can let the number empty or create a fake DB update
Comment 67 Jared Camins-Esakov 2012-06-13 11:34:59 UTC
> > > Can you explain what you made ? I suspect you have added a test SQL in
> > > installer/data/mysql/versions/ and called it test01.sql. It's wrong, it must
> > > be a number 3.09.00.014 for example (could also be 123.456, but it must be
> > > numbers+. only)
> > Yes, I made that file. Instead of such an error message, I would propose
> > just to ignore files that do not match the numbering scheme. Perhaps mark
> > them as inappropriate on the new updatedatabase form, or so. (In a folder
> > you can always expect files with other names..) Will return to that too soon.
> If someone submit a patch including a file with a wrong name it won't pass
> QA, that's as simple as that, I'm not sure it's needed to check for the
> format.

I think it would be best to catch that error and inform the user there is an incorrectly named file. The goal is to lower the barrier to entry for developers, and if a developer doesn't understand the correct naming scheme, they should get useful feedback ("a file with an invalid name was provided") instead of a scary error.

> Also note that, when the patch will be pushed, i'll start a wiki page where
> ppl, when submitting a patch with a DB update, will be able to mark/reserve
> a number to avoid having a conflict. And if a number does not make his way
> into Koha finally, that's not a big problem, we can let the number empty or
> create a fake DB update
Comment 68 Marcel de Rooy 2012-06-13 11:39:37 UTC
(In reply to comment #67)
> I think it would be best to catch that error and inform the user there is an
> incorrectly named file. The goal is to lower the barrier to entry for
> developers, and if a developer doesn't understand the correct naming scheme,
> they should get useful feedback ("a file with an invalid name was provided")
> instead of a scary error.

I am still working on this report. But in advance: I think we should have two sets of dbrevs: the numbered ones belonging to approved, pushed patches and unnumbered ones, belonging to patches in dev stage. These last set should only be available in dev mode and can (theoretically) be executed in any order.
Comment 69 Jared Camins-Esakov 2012-06-13 11:46:46 UTC
(In reply to comment #68)
> (In reply to comment #67)
> > I think it would be best to catch that error and inform the user there is an
> > incorrectly named file. The goal is to lower the barrier to entry for
> > developers, and if a developer doesn't understand the correct naming scheme,
> > they should get useful feedback ("a file with an invalid name was provided")
> > instead of a scary error.
> 
> I am still working on this report. But in advance: I think we should have
> two sets of dbrevs: the numbered ones belonging to approved, pushed patches
> and unnumbered ones, belonging to patches in dev stage. These last set
> should only be available in dev mode and can (theoretically) be executed in
> any order.

That makes sense to me. If everything has to have a numeric dbrev, even code that might not ever work well enough to make it into Koha, we might as well just continues to use the current updatedatabase.pl.
Comment 70 Marcel de Rooy 2012-06-13 14:25:07 UTC
QA comment:
Larger patch with greater impact. 
Good work! Code generally looks good.
Nice idea. Will make testing patches with dbrevs easier.
Because of the impact of this patch, I think that we still need some adjustments before it passes qa. The points below may look like a lot of work, but will not need that much code changes. I believe that it can be done within some hours. (Please note that I would really LIKE to see this feature be pushed soon!)

I change status to In Discussion and will ask for input from the dev list. Before you change the patch, it would be good to have some additional community consensus about it (when given).

My main (functional) point is actually: make a distinction between numbered dbrevs (from master) and unnumbered dbrevs (from test patches or custom development). An upgrade should always run the numbered dbrevs in linear order. The unnumbered ones can be executed in any order from the new update form if you are in dev mode. This makes a wiki with picking dbrev numbers unneeded. The RM still takes care of the numbering (rename a file). A developer just adds a file without dbrev number in a new patch, perhaps referrring to a bug report. There will be no gaps in the numbering scheme. About lists the correct official Koha version and the applied 'custom' dbrevs.

YAML: As I understand, the idea was initially to have dbrevs in a yaml file. In the current implementation, only the version dir is in that file. Additionally, you do not use the dirload, only the fileload. For simplicity, I recommend to remove the YAML file and associated code. The versions directory can be hardcoded in my opinion within the install/data.. region. I would suggest to rename it to dbrev or something similar, and have numbered version subdirectories within it.
Example: This will eventually lead to dirs and files like dbrevs/3.09/3.09.00.001.sql or dbrevs/3.10/3.10.02.003.pl, etc.

Development mode: Currently, you rely on $ENV{DEBUG} from koha-httpd file. I would turn this into a systempref. It is easier for developers to toggle it (compared to restarting httpd service). 

Skeleton: I see it in a regex as well as some code inserting into foo. Assume that it was used in testing. Please remove it. In actual use, you will find enough examples to find your way. Or just put some comments into a readme file.

Mainpage: Some code was removed from Auth.pm. The version check is now in mainpage.pl itself. I would rather have that check in an appropriate module. It was in Auth.pm. You could leave it there? I think we should let the version check stay within the scope of the checkauth call included in the get_user_and_template call. (See also comment on Auth.pm below).

Authorization (Auth.pm): I agree that if a user already logged in, it is not needed to check the version every time. The ("new") initial check in checkauth if the db contains a version, is fine. The former moment of the "complete" version check was too early. I think we should leave it in checkauth, but do it at a later moment: If we start a new session and we verified the password, then we should do the extended version check (database versus kohaversion.pl). That is close to line 748 in the adjusted Auth.pm (depending on the value of $return). This will prevent Koha from checking it every time, but only for a new session. If the version check fails at that moment, we should redirect to installer step 3 (updatestructure).

Note that there is some problem in current code, that forces me to login twice when there is an update available. (It redirects to admin/updatedatabase, but I must relogin again.) Please correct. Probably you should only pass the $cookie in your redirect call too. Like:
print $query->redirect(-uri=>"/cgi-bin/koha/admin/updatedatabase.pl",-cookie=>$cookie);

Upgrading: When coming from an older version (before 3.9.0.x), you should run the old updatedatabase and after that you should run the new dbrevs in the dbrev directories. This is currently done in two passes. First it runs the old update. You think that it is ready. When you login, you are prompted to the new update form. It should not be too difficult to merge this into one pass (less confusing). Please adjust install.pl for that: You should check if it is still needed to call the old update before running the new one (for numbered dbrevs only).

Update form: Since numbered dbrevs are run via installer, only allow executing unnumbered dbrevs here (in dev mode). In normal mode, you can only browse history here. (I would suggest to not even show the unnumbered dbrevs if you are in normal mode.)

File structure: As mentioned above, make directories for a Koha release. Getting all updates means fetching the updates from a few directories. This makes the feature more future-proof.

Stable version: The md5 test will be of good use here! If we backport this to 3.8.X, we could already check what updates we already have run from 3.9 folder with the md5 checksum. When upgrading from 3.8 to 3.10, some dbrevs are new, others will be incorporated already. So this remark only serves the purpose of discussing "Should we also get this into stable already (within reasonable time)?".

admin/updatedatabase.pl: Line 33 adds a fixme to new code: Add a new flag?

Commit message: please make it up-to-date. E.g. section on installdir.
Comment 71 Marcel de Rooy 2012-06-20 12:59:24 UTC
Well, we had this bug a week in discussion with feedback from Jared, Ian, Robin, Chris N and Mason. This makes me draw the following conclusions with reference to my former QA comment:

1) My suggestion to turn dev mode on via a preference was not accepted. So we can leave that as it is (via $ENV{DEBUG}).
2) The distinction between numbered dbrevs and unnumbered ones should be added. Note however that my suggestion to not show them in normal mode can be discarded.
3) Although only Ian is rather hesitant in accepting this solution, his idea of reverting/undoing a dbrev is quite interesting for a future followup.

Could you please adjust the patch with these comments on the former QA comment in mind? Thanks.
Comment 72 Jonathan Druart 2012-06-28 15:37:13 UTC Comment hidden (obsolete)
Comment 73 Paul Poulain 2012-07-04 15:43:20 UTC
(In reply to comment #72)
> > File structure: As mentioned above, make directories for a Koha release.
> > Getting all updates means fetching the updates from a few directories. This
> > makes the feature more future-proof.
> 
> I create all of .sql and .pl until version 3.09.00.018 (current master)
Jonathan, I think that what Marcel was suggesting was to have :
mysql/versions/3.09.00/001.sql
mysql/versions/3.09.00/002.sql
mysql/versions/3.09.00/003.sql

+ if we do that shouldn't we remove those lines from data/mysql/updatedatabase.pl ?
+ that will cause some trouble to ppl running master (we can announce it, not a real problem, just warning)

> > admin/updatedatabase.pl: Line 33 adds a fixme to new code: Add a new flag?
> Finally I use the "parameters" flag. I don't know if we want a special
> permission.
sounds OK to me

About:
> 2) The distinction between numbered dbrevs and unnumbered ones should be
> added. Note however that my suggestion to not show them in normal mode can
> be discarded.
you said nothing, so I checked : it works ! I created a bug 1.sql file, and it applies smoothly.
So our workflow could become : "when you submit a patch that has a DB update, put it in a file named bug NNNN.sql or bug NNNN.pl, the RM will take care of the final number when pushing it"

That would result in no more merge conflicts, sandboxes working well in any circumtances, I love this patch ;-)

Tested a lot again, work well, switching to signed off again, Marcel, your opinion about data/mysql/updatedatabase.pl that should be cleaned from 3.09.* patches ? [Note that, in the meantime, we've reached 22 already ;-) ]

If we want to do it that way, it means that all patches pending sign-off/QA will have to be updated to take care of this new behaviour. Not a big deal, just noticing it
Comment 74 Marcel de Rooy 2012-07-05 07:00:38 UTC
Will be looking at it today. Comment in preparation..
Comment 75 Marcel de Rooy 2012-07-05 08:31:14 UTC
QA Comment:
Jonathan: I am very sorry, but I fear that you missed my main point somehow from comment70 /comment71 about the distinction between numbered and unnumbered dbrevs. Please reread. Just in short: In user mode installer should run the numbered dbrevs; the update form only lists both dbrevs for reference. There is no need to run dbrevs from there in that mode.
In dev mode installer still runs the numbered dbrevs, the update form allows you to run the other ones in any order.
The current patch still allows me to run both groups. (And I still think that you can make this adjustment without changing too much..)

Secondly, see my comment on Mainpage AND Authorization. Please move the extended version check back from mainpage to checkauth but later in time (only for new sessions). Note also that the double login problem still remains. After applying your patch, and installing the new cpan module, I had to login twice again. (See the cookie comment?) 

File structure: Paul is right. I meant subdirs per version. Note that I did not mean creating the files for 001 to 018. Since they are in updatedatabase already, I would suggest to remove them from this patch (*sorry*). 

Upgrading: You wrote:
> I don't know how I can do this simply and properly. if someone feels motivated...
This point is closely related to the first comment. Please look at install.pl around line 324. We need to record the exact dbrev when this patch is pushed, and compare against that. If we are lower, run first old updatedatabase. And after that, make a call to your new code to run the numbered dbrevs in the versions dir(s). I am convinced that you can do that very well and properly :-)
See also my earlier comment on Stable version. This needs some additional precaution, since we will not implement it in master and 3.8.X at the same time. (The version compare in 3.8.X will be different from master.) When upgrading to 3.10, you cannot completely prevent running some dbrevs for a second time. (But some could be skipped when comparing your checksums between 3.8 and 3.9 folder.) This issue will reappear at every new version (e.g. comparing checksums again between 3.10 and 3.11 folder, etc.)

New (minor) point: C4/Update/Database: New dependency introduced: Can't locate File/Find/Rule.pm in @INC. Do you really need this one? You only need to get some file names? Just use glob?

Hope that this helps.
Comment 76 Paul Poulain 2012-07-05 08:45:44 UTC
(In reply to comment #75)
> QA Comment:
> Jonathan: I am very sorry, but I fear that you missed my main point somehow
> from comment70 /comment71 about the distinction between numbered and
> unnumbered dbrevs. Please reread. Just in short: In user mode installer
> should run the numbered dbrevs; the update form only lists both dbrevs for
> reference. There is no need to run dbrevs from there in that mode.
> In dev mode installer still runs the numbered dbrevs, the update form allows
> you to run the other ones in any order.
question : if you're in user mode, it means you're running a published version right ? So there should not be any unnumbered dbrev ?
conclusion = isn't the current behaviour correct ?

> Secondly, see my comment on Mainpage AND Authorization. Please move the
> extended version check back from mainpage to checkauth but later in time
> (only for new sessions). Note also that the double login problem still
> remains. After applying your patch, and installing the new cpan module, I
> had to login twice again. (See the cookie comment?) 
I won't object as long as it does not require checking on *every* page.

> See also my earlier comment on Stable version. This needs some additional
> precaution, since we will not implement it in master and 3.8.X at the same
> time. (The version compare in 3.8.X will be different from master.) When
> upgrading to 3.10, you cannot completely prevent running some dbrevs for a
> second time. (But some could be skipped when comparing your checksums
> between 3.8 and 3.9 folder.) This issue will reappear at every new version
> (e.g. comparing checksums again between 3.10 and 3.11 folder, etc.)
Couldn't we also decide that 3.8 DBrevs are made on the old updatedatabase.pl, DBrevs for 3.10 are in the new one ?
So no need to backport this enhancement to 3.8.
Comment 77 Marcel de Rooy 2012-07-05 08:53:04 UTC
(In reply to comment #76)
> question : if you're in user mode, it means you're running a published
> version right ? So there should not be any unnumbered dbrev ?
> conclusion = isn't the current behaviour correct ?
Update should only list/execute the unnumbered ones. You could have a published version plus some custom work. You install that in dev mode and then put Koha back to user mode. I guess that means that you normally should not have unexecuted unnumbered dbrevs..

> Couldn't we also decide that 3.8 DBrevs are made on the old
> updatedatabase.pl, DBrevs for 3.10 are in the new one ?
> So no need to backport this enhancement to 3.8.
Good for me. But this issue will come up again  between 3.10 and 3.11. You will have dbrevs in 3.10 folder; they are a subset of the 3.11 folder.
Comment 78 Marcel de Rooy 2012-07-05 08:54:29 UTC
(In reply to comment #77)
> Update should only list/execute the unnumbered ones. 
List all, of course..
Comment 79 Paul Poulain 2012-07-05 15:41:06 UTC
(In reply to comment #77)
> (In reply to comment #76)
> > question : if you're in user mode, it means you're running a published
> > version right ? So there should not be any unnumbered dbrev ?
> > conclusion = isn't the current behaviour correct ?
> Update should only list/execute the unnumbered ones. You could have a
> published version plus some custom work. You install that in dev mode and
> then put Koha back to user mode. I guess that means that you normally should
> not have unexecuted unnumbered dbrevs..

I feel we're misunderstanding ourselves:
* if you're on a production server, you've only numbered DBrevs + your own local revisions (that can be numbered -bad idea- or no)
* if you're on a test/dev server, you've numbered DBrevs (pushed already), plus unnumbered DBrevs that comes from patches you want to test.

It's not related to being in dev mode or no. The dev mode just let you choose DBrevs one by one, where the normal mode apply everything, and that's all.

> > Couldn't we also decide that 3.8 DBrevs are made on the old
> > updatedatabase.pl, DBrevs for 3.10 are in the new one ?
> > So no need to backport this enhancement to 3.8.
> Good for me. But this issue will come up again  between 3.10 and 3.11. You
> will have dbrevs in 3.10 folder; they are a subset of the 3.11 folder.
that's not a problem = you'll have to apply all patches coming from any folder anyway, isn't it ?

> New (minor) point: C4/Update/Database: New dependency introduced: Can't locate 
> File/Find/Rule.pm in @INC. Do you really need this one? You only need to get some file 
> names? Just use glob?
jonathan says glob doesn't handle recursivity, so this package fit our needs better.

Jonathan will submit a patch that :
* check dbrev on check auth page
* add the cookie to avoid duplicate login
* delete 3.9 from old updatedatabase, to start from a clean situation = 3.9-10 => everything in a 3.9 directory.

The workflow would then be =
* if you submit a patch that update database and should be ported to 3.8 => use the "old" updatedatabase.pl script
* if you submit a patch that update database and will be for 3.10 => use the new mechanism and provide a version/bug NNNN.sql file. The RM will take care of moving it to 3.9/dbrevnumber.sql when pushing
Comment 80 Jonathan Druart 2012-07-06 10:55:22 UTC Comment hidden (obsolete)
Comment 81 Jonathan Druart 2012-07-06 10:55:44 UTC Comment hidden (obsolete)
Comment 82 Jonathan Druart 2012-07-06 10:56:07 UTC Comment hidden (obsolete)
Comment 83 Katrin Fischer 2012-07-06 11:18:04 UTC
Hi Jonathan, 
I have been following this bug, but not sure I understand everything correctly. I have been wondering about the version check - you write it will be checked on login and after login. I think with our current system the system preference is checked everywhere and the OPAC goes into maintenance mode (like when you switch the system preference on). Will it work like this with the new system too? I think it would be safer, because some changes might break OPAC functionality until the update is finished.
Comment 84 Jonathan Druart 2012-07-06 13:12:38 UTC
Hi Katrin,

Currently, we check on each page if DB is up-to-date (e.g. if version in kohaversion.pl > syspref "Version").
With this new system, I think it is better to modify the workflow to decrease the number of sql queries.
The idea is:
1/ switch on the syspref OpacMaintenance before an upgrade.
2/ update your sources
3/ Check on admin/updatedatabase.pl if exist new version to execute
4/ switch off the OpacMaintenance.
Comment 85 Paul Poulain 2012-07-06 13:56:19 UTC
(In reply to comment #84)
> Hi Katrin,
> 
> Currently, we check on each page if DB is up-to-date (e.g. if version in
> kohaversion.pl > syspref "Version").
> With this new system, I think it is better to modify the workflow to
> decrease the number of sql queries.
> The idea is:
> 1/ switch on the syspref OpacMaintenance before an upgrade.
> 2/ update your sources
> 3/ Check on admin/updatedatabase.pl if exist new version to execute
> 4/ switch off the OpacMaintenance.

100% agreeing =
1- upgrades must be planned & organized, setting OpacMaintenance manually when starting is a good behaviour
2- it's not only a matter of "it's better to modify the workflow", it's a matter of "it would cost too much ressources to check on every page"

My opinion here is that we have a positive balance. (It must be announced clearly though, i'll take care of this announcement)
Comment 86 Marcel de Rooy 2012-07-09 08:25:46 UTC
Jonathan:
Could you please squash these patches? 
I will be looking into this report again soon (high on my list; and will not depend on another signoff). Although anybody of course is more than welcome to add a signoff..
Note that you could leave the old signoff's in the commit message as long as it is clear that the patch was modified after that.
Thanks!
Comment 87 Paul Poulain 2012-07-09 08:36:18 UTC
(In reply to comment #86)
> Jonathan:
> Could you please squash these patches? 
mmm... The main patch is very large, finding what the follow-up does is hard, so I think squashing in this case is not a good idea for you. Once you've set passed QA, we can always squash before commiting.

(once exception: the patch that move *.sql, which could have been squashed, I agree. But that does not prevent you from QAing, we will just have to squash later)
Comment 88 Marcel de Rooy 2012-07-09 08:44:30 UTC
(In reply to comment #87)
> mmm... The main patch is very large, finding what the follow-up does is
> hard, so I think squashing in this case is not a good idea for you. Once
> you've set passed QA, we can always squash before commiting.
> 
> (once exception: the patch that move *.sql, which could have been squashed,
> I agree. But that does not prevent you from QAing, we will just have to
> squash later)

My idea was to get rid of lines adding and removing the same thing again..
Comment 89 Jared Camins-Esakov 2012-07-09 13:24:24 UTC
Just to confirm, the updates can be .pl files, right? We have a lot of updates that require some sort of logic that can't easily be done in SQL.
Comment 90 Jonathan Druart 2012-07-09 13:48:17 UTC
(In reply to comment #89)
> Just to confirm, the updates can be .pl files, right?

Yes, see the attachment 10667 [details] [review] for examples.
Comment 91 Marcel de Rooy 2012-07-13 11:49:09 UTC
Looking at this report again now..
Comment 92 Marcel de Rooy 2012-07-13 12:42:39 UTC
Please bear with me :) New file(s) forthcoming..
Comment 93 Marcel de Rooy 2012-07-13 16:16:03 UTC
I squashed the commits and removed the sql files from them.
I hoped to get this patch in Passed QA status, but found too many errors. (The latest changes cannot have been tested extensively.)
I started with a few small amendments, but this kept growing.. (See commit message)
After all, I spent the whole afternoon on testing and debugging. This went far beyond regular QA..
My sugggestion is that Jonathan continues at this point.

Some points still to consider:
The turnover point is now hardcoded at 3.9.0.25 at two places. If I upgrade from 3.9.0.24 to a fictitious 3.9.0.26 (in a separate dbrev), install somehow does not run the 3.9.0.26. Although update on the command line does.
About shows a version 25 with rev applied 26. Should be 26 without.
While looking at the code so extensively, I have been wondering if the get_queries and execute stuff is really needed. Why not run the file? Actually, I did for perl dbrevs now. (But let us discuss that first.)

After fixing these issues, please tell what you actually tested. Run tests from older versions to this version and include some new test versions on top of it.
Comment 94 Marcel de Rooy 2012-07-13 16:16:54 UTC Comment hidden (obsolete)
Comment 95 Jonathan Druart 2012-07-16 13:03:20 UTC
(In reply to comment #93)

> While looking at the code so extensively, I have been wondering if the
> get_queries and execute stuff is really needed. Why not run the file?
> Actually, I did for perl dbrevs now. (But let us discuss that first.)

_get_queries allows us to find potential problems with queries to execute (coherency on syspref, table/column name, permission) and to return a friendly structure ( {queries => [], comments => [] } ).
With your suggestion, how can we know (to display) the queries to execute (or which had been executed) and the comments ?
Comment 96 Marcel de Rooy 2012-07-16 13:33:24 UTC
(In reply to comment #95)
> _get_queries allows us to find potential problems with queries to execute
> (coherency on syspref, table/column name, permission) and to return a
> friendly structure ( {queries => [], comments => [] } ).
> With your suggestion, how can we know (to display) the queries to execute
> (or which had been executed) and the comments ?

First, I do not want to oppose this code (at this stage in time). It is in itself of course good to check for potential problems, etc. 

I am just wondering: Are we really helping the developers or would it become more complex? Suppose that I create a sql file with some statements spanning multiple lines. Would it work with your approach? Why should we build some kind of mysql tool if we can run the file with mysql just as is done during install for other things?

I discovered that in a perl file I needed to define routines for these queries and comments, and I did not think that it was easier. With just a use Context, a db handle and the "old style" sql statements things seemed easier..
Comment 97 Paul Poulain 2012-07-20 12:26:29 UTC
(In reply to comment #96)
> I discovered that in a perl file I needed to define routines for these
> queries and comments, and I did not think that it was easier. With just a
> use Context, a db handle and the "old style" sql statements things seemed
> easier..

The idea behind this feature is not to ease developers, but future administration of the database.

Suppose you've handled 3.05.00.017 upgrade. It does a different SQL request depending on your MARC flavour.
With this patch, you will know, *6 months later*, which SQL requests you *had* executed. Whatever your parameters are *today*

The .pl can do complex things to build queries, but what we're doing is execute SQL, exactly like when you have .sql file.

I agree it's a little bit more for developers (but only a little bit more), but can be really helpfull for dbadmin, so it's worth the price.

About other points:
* Ascending order for table on admin/updatedatabase
  => it was not a mistake. When you'll upgrade in production, you'll be happy to see what's new & need to be applied first. With the patch as it was, one would see that he has to apply 3 patches.

* Important typo $koha39=~ should be assignment. Updated version number in install and Auth due to removing older sql files.
  => agreed it was a mistake

* Typo _get_queries: removed the call after all; it is easier to leave SQL in perl files outside the update tables. Added some checks for emptiness.
 => see below, I think it's should be kept

* Corrected execution of perl dbrevs (filepath).
 => I don't understand

(note about the updated patch = it's easier to have follow-up in a case like this one, it makes much more clear what has been changed ! The RM or QAer can squash all patches when things are OK, just before pushing)
Comment 98 Marcel de Rooy 2012-07-20 13:53:05 UTC
(In reply to comment #97)
> (note about the updated patch = it's easier to have follow-up in a case like
> this one, it makes much more clear what has been changed ! The RM or QAer
> can squash all patches when things are OK, just before pushing)

I completely agree on that, but as described it was not my intention nor hope..
Will soon come back to the other points
Comment 99 Marcel de Rooy 2012-07-25 13:12:39 UTC Comment hidden (obsolete)
Comment 100 Marcel de Rooy 2012-07-25 13:13:47 UTC
> Suppose you've handled 3.05.00.017 upgrade. It does a different SQL request depending on your MARC flavour. With this patch, you will know, *6 months later*, which SQL requests you *had* executed. Whatever your parameters are *today*.
Sorry, I do not understand. I do not see any logic in the code that makes that happen. I do not think that we actually need that either.

Additional observations: 
If a sql command in a .sql file spans two lines, the patch does disapprove it:
Errors : Error : 1113 => A table must have at least 1 column Error : 1064 => You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'id int)' at line 1
Note: IMHO we should not write a sql parser. And if we accept .sql files, we should be able to handle them all. I still strongly prefer to run .sql files via mysql; this is something that you also expect to happen.
Side note: If a db rev fails, how do I get rid of the Failed-lines on the update screen?
Side note: The About form lists db revs that failed as if they have been applied correctly.

update.pl: When run from installer, I think that update.pl should only run the numeric db revs. Note that the current code around line 52/53 does not handle that correctly:
Argument "\x{74}\x{65}..." isn't numeric in numeric ge (>=) at installer/data/mysql/update.pl line 52.
I just added a provisional test now to get further. Please correct.
Maybe you need an additional parameter here? 

chmod 755 was needed on update.pl; the system call in install.pl to update.pl did not work for that reason.
And of course, if you run it that way include a shebang line. Did that now too.
Please add copyright as well.

Output from webinstaller now includes: 3.09.00.027 => ARRAY(0x1b1da88). Please correct.
The webinstaller lists the wrong new version; it now lists 3.9.0.26 but it will install 27 too.
The subsequent calls to IPC run and print_std do not work somehow. Only the output from the second call is shown.

IMO The logic in C4/Auth.pm  around line 590 is not correct. It should redirect to installer for all numeric db revs and redirect only to the new update for the non-numerical ones. 

Before moving further: Do we continue with the squashed patch or restore the old ones? Note that you have to correct quite some things there. Probably comparing the latest squased patch with squashing the original ones could help.
Comment 101 Jonathan Druart 2012-07-26 09:33:00 UTC
(In reply to comment #100)

Hi Marcel, 

First thanks for your umpteenth QA.

When I proposed this path, I didn't think that would be as difficult to integrate :)

>Sorry, I do not understand. I do not see any logic in the code that makes that happen. I do not think that we actually need that either.

I don't understand neither :)
When I imagined and developped this patch, I wanted something robust and intelligent.
Most part of the routine of the new module (C4/Update/Database.pm) is present to provide these functionnalities.
If we wanted for Koha a simple update database script, it would have been created several years ago. Because this script could have been written in 1 day with 100 lines of code.

To provide a more robust script, we have to accept some compromises. For example, each .pl update version must contain a _get_queries routine to be called by a parent script. Perhaps it seems less easy to write, but I don't think so. Indeed, all the common part of code (like get a dbh, execute, get errors (or not), etc.) are factorised.

However, either .pl and .sql are "intelligent", or both are "stupid". But it will be weird if they have 2 differents behaviours depending on their extensions.


> If a sql command in a .sql file spans two lines, the patch does disapprove it
Have a look at attachment 10549 [details] [review], file installer/data/mysql/versions/3.09.00.014.sql
It's a feature :)
If you peeks at the C4::Update::Database::get_queries routine, you can see a management of the DELIMITER mysql statement. It permits to write something like:
    -- some comments
    DELIMITER //
    INSERT INTO my_table
        (field1, field2) VALUES
        ("value1", "value2")//

I think it is what you want, isn't it ?


> Note: IMHO we should not write a sql parser. And if we accept .sql files, we should be able to handle them all. I still strongly prefer to run .sql files via mysql; this is something that you also expect to happen.

See below. If we just want a script like:
if ($filename =~ /.pl$/ ) {
    system( "perl $filename" );
} else {
    system( "mysql < $filename" );
}

5 lines are sufficient :)

> Side note: If a db rev fails, how do I get rid of the Failed-lines on the update screen?
What do you call 'the update screen' ? It is the table containing all versions ?

If I get an error like 
    3.09.00.005 -- This file ( /home/koha/src/installer/data/mysql/versions/3.09/3.09.00.005.pl ) still already execute in version 3.09.00.005 : same md5 (682eb7514afeee03d5ff29824dd8a621) [Mark as OK]
I can click on 'Mark as OK'. It will mark this db version as 'It is corrected'.

If a new db version contains:
    -- comments
    DELIMITER //
    CREATE TABLE tags (field1 integer);

Then, the check_coherency fails (we already have a tags table). It permits to have a personnal message of the failure and translate it into the template.
We can simply extend this function to add other verification.
Then the screen displays:
    Report :
        3.09.00.044 -- COHERENCY: Table tags already exists ; [Mark as OK]
    Your datebase is not up to date.
    1 update available [UPDATE 3.09.00.044] 

If you click on the 'Mark as OK' link, the following screen displays 'Your database is up to date' (I agree with you, I could have written: "All your base are belong to us", but it is less comprehensible :) )

> Side note: The About form lists db revs that failed as if they have been applied correctly.
It works like into the actual version. A version is executed and the version of koha is this new version for all cases (failure or success). We consider it is the job of the guy who execute the update to maintain a DB up-to-date.


Others comments are technicals and can be fixed.

I think we want to be on the same wavelength before we continue this development (especially for the first point).

Perhaps, somebody else can given his opinion and open the discussion.
    
I hope will be understood :)
Comment 102 Jared Camins-Esakov 2012-07-26 13:23:25 UTC
(In reply to comment #101)
> To provide a more robust script, we have to accept some compromises. For
> example, each .pl update version must contain a _get_queries routine to be
> called by a parent script. Perhaps it seems less easy to write, but I don't
> think so. Indeed, all the common part of code (like get a dbh, execute, get
> errors (or not), etc.) are factorised.

I like centralizing database access and eliminating direct use of dbh (at least wherever possible). I have no objection to saving the SQL that was run, and did not check the code to see if it works. If it doesn't and that is one of the stated features, it should.

However, I don't like parsing the SQL ourselves. Why don't we just open the SQL file, save the contents into the variable that stores the SQL, and run it directly? Writing our own SQL parser is sure to come back and bite us. Moreover, requiring special delimiters seems like an unnecessary burden.
Comment 103 Chris Nighswonger 2012-07-26 13:44:22 UTC
(In reply to comment #102)
> 
> However, I don't like parsing the SQL ourselves. Why don't we just open the
> SQL file, save the contents into the variable that stores the SQL, and run
> it directly? Writing our own SQL parser is sure to come back and bite us.
> Moreover, requiring special delimiters seems like an unnecessary burden.

I'm also leery of writing our own SQL parser. The "kiss" philosophy strikes me as appropriately applied here. The less customization, the fewer maintenance, etc. issues.

(I'm thinking specifically of C4::SQLHelper here... :-P )
Comment 104 Jonathan Druart 2012-07-27 10:01:34 UTC
Hi Jared and Chris,

Thanks for your feedback :)

In fact, when I started this development, I thought about a structure to manage. This structure is
    { errors => [], queries => [], comments => [] }
It is everywhere in this patch:
  * DB: see tables updatedb_error, updatedb_query and updatedb_report
  * perl files: see C4/Update/Database.pm and admin/updatedatabase.pl and search queries, comments, errors
  * template files: The structure is the same (see routine list_versions_already_knows)

If we have not the same structure with these informations (errors, comments, queries), a big part of this development is useless.
To obtain these informations we have to "parse" the sql file. Moreover the pl file have to return a consistent and adapted structure.

This sql "parsing" is in the routine C4::Update::Database::get_queries (~20 lines). It's not darkness :
    If the line begin with '--' it's a comment.
    If we want to change the delimiter, we change the input record separator. Indeed, by default it's the semicolon character. We want to use this sql statement to know when the query is terminated.
    Else it's a query

The routine check_coherency seems to cause problems. In fact, we can bypass this check. But if we want, in the future, translate the errors we have to encapsulate errors for a better usability. Else we would be displaying an error from the DBMS (then not translatable).


Regarding the delimiter, it is NOT mandatory. But it is if you want to write a query on several lines.

Finally, if we have not these informations, the patch should to be completely rewrite...
Comment 105 Katrin Fischer 2012-07-27 10:38:54 UTC
I think displaying the errors from the DBMS untranslated will be most useful. Upgrades are supposed to be run by a system administrator of some kind and they will have to find out what's going wrong on database level, if there is a problem. Being able to google the original error message might be more useful than a not so exact translated message we could come up with.
Comment 106 Chris Nighswonger 2012-07-27 13:23:22 UTC
(In reply to comment #104)
> This sql "parsing" is in the routine C4::Update::Database::get_queries (~20
> lines). It's not darkness :
>     If the line begin with '--' it's a comment.
>     If we want to change the delimiter, we change the input record
> separator. Indeed, by default it's the semicolon character. We want to use
> this sql statement to know when the query is terminated.
>     Else it's a query

Perhaps we are not understanding what is meant by "parsing" in this context. Does parsing (as used here) simply mean differentiating between a SQL comment and a SQL query? If so, we may be ok.

> 
> The routine check_coherency seems to cause problems. In fact, we can bypass
> this check. But if we want, in the future, translate the errors we have to
> encapsulate errors for a better usability. Else we would be displaying an
> error from the DBMS (then not translatable).

I agree with Katrin here. We should pass the error directly as received from the DBMS to avoid linguistical confusion.
Comment 107 Paul Poulain 2012-07-27 13:25:43 UTC
(In reply to comment #102)
> However, I don't like parsing the SQL ourselves. Why don't we just open the
> SQL file, save the contents into the variable that stores the SQL, and run
> it directly? Writing our own SQL parser is sure to come back and bite us.
> Moreover, requiring special delimiters seems like an unnecessary burden.

Putting my RM hat away, and taking my Jonathan-boss one for this comment.

Jonathan (and me) invested a lot of time and efforts -sponsored by no-one- to develop this improvement because we thought it was worth it.
We made many changes to fit various comments, and took many time to explain what we did and why we did it. We don't want to invest more and more time, and don't think it's fair to refrain this patch because of "don't like" or "could be done another way".
If there are true QA-related comments, we will deal with them. But please, stop adding requirements to this patch. Or we will abandon it, it will not be pushed and we will stick with the current updatedatabase mechanism.
Comment 108 Jonathan Druart 2012-07-27 13:37:26 UTC
(In reply to comment #106)

> I agree with Katrin here. We should pass the error directly as received from
> the DBMS to avoid linguistical confusion.

Yes, as I said previously, it's possible to simply remove these checks (routine check_coherency).
Comment 109 Chris Nighswonger 2012-07-27 13:37:56 UTC
(In reply to comment #107)
> If there are true QA-related comments, we will deal with them. But please,
> stop adding requirements to this patch. Or we will abandon it, it will not
> be pushed and we will stick with the current updatedatabase mechanism.

I must say that this sounds a bit like coercion. At the least it carries a bad tone and is not what I would call constructive. I understand the concern (I think) but strongly object to the language of the above paragraph in particular.

Jonathan, himself, asked for more discussion: "Perhaps, somebody else can given his opinion and open the discussion." Several of us have obliged him. Quite frankly, you can take it or leave it. You (Paul) are the RM and have the full liberty to push or drop any given work. The three of us who have commented in response to Jonathan's request hardly represent the majority of the community. Quite obviously this work functions and does the job regardless of the outstanding objections/reservations. If the technical objections of the QA person have been met and you feel the work is sound, by all means push it. Just please do not resort to the sort of language which really serves only to intimidate. (I know... we all get frustrated. We just have to get over it and move forward.)

<tone>typed with a smile and the kindest of intentions</tone>
Comment 110 Paul Poulain 2012-07-27 14:01:00 UTC
(In reply to comment #109)
> (In reply to comment #107)
> > If there are true QA-related comments, we will deal with them. But please,
> > stop adding requirements to this patch. Or we will abandon it, it will not
> > be pushed and we will stick with the current updatedatabase mechanism.
> 
> I must say that this sounds a bit like coercion.
I apologize, it was not my goal at all (and that's why I've explicitely put my "jonathan boss" hat before writing)

> At the least it carries a
> bad tone and is not what I would call constructive.
again, it was not my goal. I just wanted to point that we're fearing that the discussion last forever. We've submitted 25 patches, 106 comments, that's still not done.

I think Katrin concern about being able to ggl the error message being more important than having it translatable is a good concern, and Jonathan agrees.

[ Maybe what make us (jonathan and me) react a little bit too strongly is that some things risen [c|w]ould result in a complete rewrite of the patch, which we won't do. In this case, the patch will just be abandonned -that's not a threat- ]
Comment 111 Chris Nighswonger 2012-07-27 14:26:12 UTC
(In reply to comment #106)
> (In reply to comment #104)
> > This sql "parsing" is in the routine C4::Update::Database::get_queries (~20
> > lines). It's not darkness :
> >     If the line begin with '--' it's a comment.
> >     If we want to change the delimiter, we change the input record
> > separator. Indeed, by default it's the semicolon character. We want to use
> > this sql statement to know when the query is terminated.
> >     Else it's a query
> 
> Perhaps we are not understanding what is meant by "parsing" in this context.
> Does parsing (as used here) simply mean differentiating between a SQL
> comment and a SQL query? If so, we may be ok.
> 

Based on a further look at the code, I withdraw my objections to "parsing" SQL in this context. If I understand correctly, all we are doing here is some basic sanity checks on the SQL to see of stuff exists or not as well as some basic "sorting out" of comments and possibly changing the delimiter.

Someone feel free to correct me if I'm wrong.
Comment 112 Paul Poulain 2012-07-31 16:04:59 UTC
(In reply to comment #100)

> update.pl: When run from installer, I think that update.pl should only run
> the numeric db revs.
I don't know why = a developer should be able to test from CLI. In production, there should not be any non numeric update.
If a library make a local update, it can be non numeric it doesn't matter, it's managed by the library.

> Note that the current code around line 52/53 does not
> handle that correctly:
> Argument "\x{74}\x{65}..." isn't numeric in numeric ge (>=) at
> installer/data/mysql/update.pl line 52.
> I just added a provisional test now to get further. Please correct.
> Maybe you need an additional parameter here? 
Problem fixed. If you have a non-numeric version number, it will be applied without reporting any error when you --all.

> chmod 755 was needed on update.pl; the system call in install.pl to
> update.pl did not work for that reason.
OK, fixed.

> And of course, if you run it that way include a shebang line. Did that now
> too.
OK, fixed.

> Please add copyright as well.
OK, fixed.

> Output from webinstaller now includes: 3.09.00.027 => ARRAY(0x1b1da88).
> Please correct.
> The webinstaller lists the wrong new version; it now lists 3.9.0.26 but it
> will install 27 too.
OK, fixed

> The subsequent calls to IPC run and print_std do not work somehow. Only the
> output from the second call is shown.

> IMO The logic in C4/Auth.pm  around line 590 is not correct. It should
> redirect to installer for all numeric db revs and redirect only to the new
> update for the non-numerical ones. 


(In reply to comment #105)
> I think displaying the errors from the DBMS untranslated will be most
> useful. Upgrades are supposed to be run by a system administrator of some
> kind and they will have to find out what's going wrong on database level, if
> there is a problem. Being able to google the original error message might be
> more useful than a not so exact translated message we could come up with.
Agreed, we've fixed that too

(In reply to comment #111)
> Based on a further look at the code, I withdraw my objections to "parsing"
> SQL in this context. If I understand correctly, all we are doing here is
> some basic sanity checks on the SQL to see of stuff exists or not as well as
> some basic "sorting out" of comments and possibly changing the delimiter.
> 
> Someone feel free to correct me if I'm wrong.
Chris, you're not wrong.

I'll submit a bunch of updated patch, all the tests we did, and a proposal for updating the wiki.

Chris, could you please signoff once again ? (or anyone else)

I've deeply reviewed this patch with Jonathan, it will be a real huge improvement for testers & users for the next release.
Unless someone object, I'll push this patch on thursday (European) morning
Comment 113 Paul Poulain 2012-07-31 16:05:25 UTC
All the tests we made with Jonathan:
Tests made:
• start from a clean situation (drop the 3 tables created by the new mechanism)
• checked that the old updatedatabase is still working by
∘ adding a 3.09.00.028 update, that is empty
∘ checked that we're redirected and the update is OK
• Created a 3.09/sample.sql that contains "UPDATE systempreferences SET value='nothing' WHERE variable='NOTHING_TOO';" with a comment "test doing nothing"
∘ verified that, on mainpage, we're redirected to new updatedatabase
∘ verified that, trying to acces, unidentified, to a page (in circulation), we're redirected to new updatedatabase
∘ going to mode DEBUG=1 in Apache config, confirmed that the button "Execute" appears. Checked that DEBUG=0 make the link disappearing
• Tested the 2 sample files (the .sql.sample and the .pl.sample) by copying them in 3.09/ directory
∘ Checked that Koha displays "2 updates available", let apply both in one click (UPDATE ALL)
• Tested that forcing applying a given update (by manually entering the number) result in a message saying it has already been executed
• checked that, if you have 2 files with the same content (ie: same md5sum, but not same name), the installer detect it's a duplicate
• Created an invalid update file, 
∘ Checked that applying it result in a message, with a link to force "mark as applied". 
∘ Checked that the link mark the DBRev as applied and the updater understand it must not bother
• Verified what happen when you have an invalid .pl file that does not even compile
∘ checked that there is an error thrown
∘ checked that the Perl error is reported (if the pl is invalid, it means the patch has not been applied, the "librarian" is a developer, displaying the Perl error is relevant)
∘ checked that you can't "force OK" this error = the .pl must be fixed, no reason to be able to force
• Verified non numeric behaviour
∘ created a file 3.09/Bug_1234.sql
∘ Tested that it appears first and can be applied
∘ Tested that, once the but has been renamed to a number (ie: it has been pushed), Koha detect it's a duplicate already applied
∘ Possible improvement = if you apply many non-numeric DBRevs because you're testing, they all appear in first place. This can be fixed by removing the entries in tables updatedb_error, updatedb_query, updatedb_report. An improvement could be to have this in the staff interface. For now:
‣ DELETE FROM updatedb_error WHERE version=?;
‣ DELETE FROM updatedb_query WHERE version=?;
‣ DELETE FROM updatedb_report WHERE version=?;
• Checked that, in CLI, misc/bin/updatedb.pl --all execute all available DB updates
• Checked mix of old and new updatedatabase mechanism:
∘ Applied some new system revisions
∘ Added some lines in installer/data/mysql/updatedatabase.pl, and checked the old versioning system runs
Comment 114 Paul Poulain 2012-07-31 16:05:43 UTC
Proposal for the wiki:

How to submit an database update ?
Let's suppose you need one for the bugzilla bug 1234.
Create a file in $KOHA/installer/data/mysql/version/.
It's suggested that the file is named Bug_1234.sql or Bug_1234.pl
You can find 2 example files in the version directory.
When you submit your patch, submit this file.
Anyone testing your patch will be redirected to the installer page, will see which changes your patch will do in the database, and will be able to apply this update.

Once the patch has been validated and is pushed by the Release Manager, it will be renamed to a numbered filename.
If your DBRev is the 33th one for version 3.10, it will be renamed as:
$KOHA/installer/data/mysql/version/3.09/3.0900033.sql
and pushed.

If your patch need to be backported to the previous (stable) version, the Release Maintainer will also rename it. Let's suppose it's the 46th dbrev for 3.8, your file will be renamed to
$KOHA/installer/data/mysql/version/3.08/3.0800046.sql

What will happend if I've already applied my patch locally as Bug_1234.sql ?
The magic of the updatedatabase will find that both are in fact the same thing. You'll just have to confirm that 3.09/3.0900033.sql is the same as Bug_1234.sql, and Koha will remember it, and won't try to apply it twice.

What will happend if I've updated to 3.8.6 that contains the 3.0800046.sql and I upgrade to 3.10.0 ?
The magic of the updatedatabase will find that both are in fact the same thing. You'll just have to confirm that 3.09/3.0900033.sql is the same as 3.0800046.sql, and Koha will remember it, and won't try to apply it twice.
Comment 115 Paul Poulain 2012-07-31 16:06:08 UTC Comment hidden (obsolete)
Comment 116 Paul Poulain 2012-07-31 16:06:12 UTC Comment hidden (obsolete)
Comment 117 Paul Poulain 2012-07-31 16:06:15 UTC Comment hidden (obsolete)
Comment 118 Paul Poulain 2012-07-31 16:06:19 UTC Comment hidden (obsolete)
Comment 119 Paul Poulain 2012-07-31 16:06:23 UTC Comment hidden (obsolete)
Comment 120 Paul Poulain 2012-07-31 16:06:26 UTC Comment hidden (obsolete)
Comment 121 Paul Poulain 2012-07-31 16:06:30 UTC Comment hidden (obsolete)
Comment 122 Paul Poulain 2012-07-31 16:06:34 UTC Comment hidden (obsolete)
Comment 123 Paul Poulain 2012-07-31 16:06:38 UTC Comment hidden (obsolete)
Comment 124 Paul Poulain 2012-07-31 16:06:42 UTC Comment hidden (obsolete)
Comment 125 Paul Poulain 2012-07-31 16:06:46 UTC Comment hidden (obsolete)
Comment 126 Katrin Fischer 2012-07-31 17:16:46 UTC
Hi Paul and Jonathan, I know that you have put a lot of work and thought into this, but it is a big change in a critical area. I would like to see this pass QA first before being pushed. 1 day (tomorrow, if you want to push on Thursday) seems a very short amount of time for the QA team or for anyone who wants to follow your test plan.
Comment 127 Chris Nighswonger 2012-08-01 00:26:42 UTC
(In reply to comment #113)
> All the tests we made with Jonathan:
> Tests made:
> • start from a clean situation (drop the 3 tables created by the new
> mechanism)
> • checked that the old updatedatabase is still working by
> ∘ adding a 3.09.00.028 update, that is empty
> ∘ checked that we're redirected and the update is OK

Here is as far as I got before hitting a show-stopper:

1. Performed a clean install over a clean branch off of a freshly updated master
2. git bz apply 7167; all patches applied cleanly to a fresh branch off master
3. Added a 3.09.00.028 update to test the correct functionality of the old update database mechanism after the application of the Bug 7167 code; Koha failed to redirect to the update screen after the addition of the test update.

Paul/Jonathan, were your tests run over a clean install on a fresh branch or over a branch which was developed on?
Comment 128 Chris Nighswonger 2012-08-01 00:56:59 UTC
(In reply to comment #127)
> (In reply to comment #113)
> > All the tests we made with Jonathan:
> > Tests made:
> > • start from a clean situation (drop the 3 tables created by the new
> > mechanism)
> > • checked that the old updatedatabase is still working by
> > ∘ adding a 3.09.00.028 update, that is empty
> > ∘ checked that we're redirected and the update is OK
> 
> Here is as far as I got before hitting a show-stopper:
> 
> 1. Performed a clean install over a clean branch off of a freshly updated
> master
> 2. git bz apply 7167; all patches applied cleanly to a fresh branch off
> master
> 3. Added a 3.09.00.028 update to test the correct functionality of the old
> update database mechanism after the application of the Bug 7167 code; Koha
> failed to redirect to the update screen after the addition of the test
> update.
> 
> Paul/Jonathan, were your tests run over a clean install on a fresh branch or
> over a branch which was developed on?

On a quick perusal of Auth.pm changes, I'm not sure how this step could ever work with the value of $koha39 hard-coded to 3.0900027. Perhaps I'm misunderstanding your test plan?
Comment 129 Paul Poulain 2012-08-01 07:46:27 UTC
(In reply to comment #128)
> > Paul/Jonathan, were your tests run over a clean install on a fresh branch or
> > over a branch which was developed on?
> 
> On a quick perusal of Auth.pm changes, I'm not sure how this step could ever
> work with the value of $koha39 hard-coded to 3.0900027. Perhaps I'm
> misunderstanding your test plan?

Yikes... when we made the tests, we made some changes to run installer (like adding fake revisions -new and old mechanism-) and, murphy's law, I made a mistake reverting them.

The following re-uploaded patches are exactly the same except for "reimplementing Reimplementing Marcel's suggestions & fixes" that check against 3.0900028 instead of 3.0900027

We have found another hidden problem, we will upload another follow-up in the next 2 hours
Comment 130 Paul Poulain 2012-08-01 09:42:19 UTC Comment hidden (obsolete)
Comment 131 Paul Poulain 2012-08-01 09:42:24 UTC Comment hidden (obsolete)
Comment 132 Paul Poulain 2012-08-01 09:42:28 UTC Comment hidden (obsolete)
Comment 133 Paul Poulain 2012-08-01 09:42:34 UTC Comment hidden (obsolete)
Comment 134 Paul Poulain 2012-08-01 09:42:39 UTC Comment hidden (obsolete)
Comment 135 Paul Poulain 2012-08-01 09:42:46 UTC Comment hidden (obsolete)
Comment 136 Paul Poulain 2012-08-01 09:42:54 UTC Comment hidden (obsolete)
Comment 137 Paul Poulain 2012-08-01 09:42:58 UTC Comment hidden (obsolete)
Comment 138 Paul Poulain 2012-08-01 09:43:03 UTC Comment hidden (obsolete)
Comment 139 Paul Poulain 2012-08-01 09:43:07 UTC Comment hidden (obsolete)
Comment 140 Paul Poulain 2012-08-01 09:43:11 UTC Comment hidden (obsolete)
Comment 141 Paul Poulain 2012-08-01 09:43:16 UTC Comment hidden (obsolete)
Comment 142 Paul Poulain 2012-08-01 09:50:37 UTC
The 12 patches attached does not differ a lot from the 11 attached yesterday:
 * The " Bug 7167: Reimplementing Marcel's suggestions & fixes" fixes the 3.09.00.028 problem (see comment 129)
 * The  "Bug 7167 reindentation & removing dead code" reindent a block and remove some useless (& bugguy) code

I've amended our test plan, so repeating it here:
Tests made:
• Start from a clean situation (drop the 3 tables created by the new mechanism drop table updatedb_error;drop table updatedb_query;drop table updatedb_report;)
• Checked that the old updatedatabase is still working by
  ∘ adding a 3.09.00.028 update, that is empty
  ∘ checked that we're redirected and the update is OK

• Created a 3.09/sample.sql that contains "UPDATE systempreferences SET value='nothing' WHERE variable='NOTHING_TOO';" with a comment "test doing nothing"
  ∘ verified that, on mainpage, we're redirected to new updatedatabase
  ∘ verified that, trying to acces, unidentified, to a page (in circulation), we're redirected to new updatedatabase
  ∘ going to mode DEBUG=1 in Apache config, confirmed that the button "Execute" appears. Checked that DEBUG=0 make the link disappearing

• Tested the 2 sample files (the .sql.sample and the .pl.sample) by copying them in 3.09/ directory
  ∘ Checked that Koha displays "2 updates available", let apply both in one click (UPDATE ALL)

• Tested that forcing applying a given update (by manually entering the number: /cgi-bin/koha/admin/updatedatabase.pl?op=update&version=3.09.01.001) result in a message saying it has already been executed

• checked that, if you have 2 files with the same content (ie: same md5sum, but not same name), the installer detect it's a duplicate

• Created an invalid update file, 
  ∘ Checked that applying it result in a message, with a link to force "mark as applied". 
  ∘ Checked that the link mark the DBRev as applied and the updater understand it must not bother

• Verified what happen when you have an invalid .pl file that does not even compile
  ∘ checked that there is an error thrown
  ∘ checked that the Perl error is reported (if the pl is invalid, it means the patch has not been applied, the "librarian" is a developer, displaying the Perl error is relevant)
  ∘ checked that you can't "force OK" this error = the .pl must be fixed, no reason to be able to force

• Verified non numeric behaviour
  ∘ created a file 3.09/Bug_1234.sql
  ∘ Tested that it appears first and can be applied
  ∘ Tested that, once the but has been renamed to a number (ie: it has been pushed), Koha detect it's a duplicate already applied
  ∘ Possible improvement = if you apply many non-numeric DBRevs because you're testing, they all appear in first place. This can be fixed by removing the entries in tables updatedb_error, updatedb_query, updatedb_report. An improvement could be to have this in the staff interface. For now:
    ‣ DELETE FROM updatedb_error WHERE version=?;
    ‣ DELETE FROM updatedb_query WHERE version=?;
    ‣ DELETE FROM updatedb_report WHERE version=?;

• Checked that, in CLI, misc/bin/updatedb.pl --all execute all available DB updates
• Checked mix of old and new updatedatabase mechanism:
  ∘ Applied some new system revisions
  ∘ Added some lines in installer/data/mysql/updatedatabase.pl, and checked the old versioning system runs

• Tested that a full install (drop database / create database / run installer.pl) works well
Comment 143 Marcel de Rooy 2012-08-01 11:31:53 UTC
(In reply to comment #126)
> Hi Paul and Jonathan, I know that you have put a lot of work and thought
> into this, but it is a big change in a critical area. I would like to see
> this pass QA first before being pushed. 1 day (tomorrow, if you want to push
> on Thursday) seems a very short amount of time for the QA team or for anyone
> who wants to follow your test plan.

If we deviate from the standard workflow, please check with the community or dev list first. Note also that many hours have been spent on QA+ing several versions of this development, but it never reached the final status yet (although I certainly hope that it will!).
Comment 144 Marcel de Rooy 2012-08-01 11:36:29 UTC
Just from glancing through some code:
+    # check if you're uptodate, and if you're not, head to updater
+    my $koha39 =~ "3.0900000";

Should this be an assignment? Note that I also corrected this before.
Comment 145 Marcel de Rooy 2012-08-01 11:37:30 UTC
(In reply to comment #144)
> Just from glancing through some code:
> +    # check if you're uptodate, and if you're not, head to updater
> +    my $koha39 =~ "3.0900000";
> 
> Should this be an assignment? Note that I also corrected this before.

Maybe too fast. Seems to be corrected by another patch...
Comment 146 Paul Poulain 2012-08-01 11:48:41 UTC
(In reply to comment #143)
> (In reply to comment #126)
> > Hi Paul and Jonathan, I know that you have put a lot of work and thought
> > into this, but it is a big change in a critical area. I would like to see
> > this pass QA first before being pushed. 1 day (tomorrow, if you want to push
> > on Thursday) seems a very short amount of time for the QA team or for anyone
> > who wants to follow your test plan.
> 
> If we deviate from the standard workflow, please check with the community or
> dev list first. Note also that many hours have been spent on QA+ing several
> versions of this development, but it never reached the final status yet
> (although I certainly hope that it will!).

As I've announced already, i'll be afk for 3 weeks, on friday. passed QA or not, I think/feel it's unwise to push such a patch just before leaving. If something goes wrong, everybody will be scared (including me !)

So I won't push this patch before coming back from holiday.

(In reply to comment #145)
> > Should this be an assignment? Note that I also corrected this before.
> 
> Maybe too fast. Seems to be corrected by another patch...

I hesitated to squash all patches or have follow-ups to clearly point the differences. I choose to keep follow-ups. I feel this question is an unsolvable one... both options have their advantages and their limits...
Comment 147 Jared Camins-Esakov 2012-08-01 12:15:40 UTC
I had a thought on this. Would it be possible to change the code to figure out the highest version in updatedatabase programmatically rather than hardcoding it in? It seems to me that pushing it when you get back would be a lot easier if you didn't have to edit the patch on your return and/or change the database update methods for all patches that have passed QA by then.
Comment 148 Paul Poulain 2012-08-01 12:38:37 UTC
(In reply to comment #147)
> I had a thought on this. Would it be possible to change the code to figure
> out the highest version in updatedatabase programmatically rather than
> hardcoding it in? It seems to me that pushing it when you get back would be
> a lot easier if you didn't have to edit the patch on your return and/or
> change the database update methods for all patches that have passed QA by
> then.

That's what we'd like to do, but failed to find how. If you've an idea...
Comment 149 Jared Camins-Esakov 2012-08-01 12:45:01 UTC
(In reply to comment #148)
> That's what we'd like to do, but failed to find how. If you've an idea...

Wouldn't just searching for the last occurrence of /^\$DBversion =/ and then evaluating that line do it?
Comment 150 Chris Nighswonger 2012-08-01 13:28:04 UTC
(In reply to comment #149)
> (In reply to comment #148)
> > That's what we'd like to do, but failed to find how. If you've an idea...
> 
> Wouldn't just searching for the last occurrence of /^\$DBversion =/ and then
> evaluating that line do it?

I think this is a great idea if it can be implemented.

Hardcoding--
Comment 151 Chris Nighswonger 2012-08-01 13:30:29 UTC
(In reply to comment #146)
> (In reply to comment #143)
> > (In reply to comment #126)
> > > Hi Paul and Jonathan, I know that you have put a lot of work and thought
> > > into this, but it is a big change in a critical area. I would like to see
> > > this pass QA first before being pushed. 1 day (tomorrow, if you want to push
> > > on Thursday) seems a very short amount of time for the QA team or for anyone
> > > who wants to follow your test plan.
> > 
> > If we deviate from the standard workflow, please check with the community or
> > dev list first. Note also that many hours have been spent on QA+ing several
> > versions of this development, but it never reached the final status yet
> > (although I certainly hope that it will!).
> 
> As I've announced already, i'll be afk for 3 weeks, on friday. passed QA or
> not, I think/feel it's unwise to push such a patch just before leaving. If
> something goes wrong, everybody will be scared (including me !)
> 
> So I won't push this patch before coming back from holiday.

I will also be afk until Monday beginning tomorrow, so I will not be able to continue testing and sign-off until then. I'd really like to be able to go through this test plan completely as well as read through the code another time before signing off.

Paul, I'll touch base with you Monday.
Comment 152 Jonathan Druart 2012-08-09 13:48:20 UTC
(subliminal message: test me during the GBSD ;-))
Comment 153 Jared Camins-Esakov 2012-08-12 16:40:05 UTC
I copied update_sample.pl.sample to update_sample.pl and made it executable, but got the following error:

update_sample -- Load functions in update_sample.pl failed (I can't load /home/jcamins/kohaclone/installer/data/mysql/versions/update_sample.pl. Please check the execute flag and if this file is a valid perl script (DBD::mysql::db do failed: Table 'koha.testtable' doesn't exist at /home/jcamins/kohaclone/C4/Update/Database.pm line 542. ) at /home/jcamins/kohaclone/C4/Update/Database.pm line 372. )

I think the problem is that TableExists doesn't work right, but I'm a little confused by the error message. It might also be that the samples have not been updated to match the current API, but I don't see what exactly the problem is.
Comment 154 Jonathan Druart 2012-08-13 08:38:44 UTC Comment hidden (obsolete)
Comment 155 Pierre Angot 2012-09-18 13:49:18 UTC
@Paul : can I test this patch on Sandbox ? You've said "Start from a clean situation", so I'm wondering ^^
Comment 156 Chris Nighswonger 2012-11-13 15:48:17 UTC Comment hidden (obsolete)
Comment 157 Chris Nighswonger 2012-11-13 15:52:41 UTC Comment hidden (obsolete)
Comment 158 Chris Nighswonger 2012-11-13 15:53:48 UTC Comment hidden (obsolete)
Comment 159 Chris Nighswonger 2012-11-13 15:57:50 UTC Comment hidden (obsolete)
Comment 160 Chris Nighswonger 2012-11-13 15:58:13 UTC Comment hidden (obsolete)
Comment 161 Chris Nighswonger 2012-11-13 15:58:29 UTC Comment hidden (obsolete)
Comment 162 Chris Nighswonger 2012-11-13 15:58:56 UTC Comment hidden (obsolete)
Comment 163 Chris Nighswonger 2012-11-13 15:59:44 UTC Comment hidden (obsolete)
Comment 164 Chris Nighswonger 2012-11-13 16:00:08 UTC Comment hidden (obsolete)
Comment 165 Chris Nighswonger 2012-11-13 16:00:33 UTC Comment hidden (obsolete)
Comment 166 Chris Nighswonger 2012-11-13 16:00:53 UTC Comment hidden (obsolete)
Comment 167 Chris Nighswonger 2012-11-13 16:01:16 UTC Comment hidden (obsolete)
Comment 168 Chris Nighswonger 2012-11-13 16:01:41 UTC Comment hidden (obsolete)
Comment 169 Chris Nighswonger 2012-11-13 16:01:57 UTC Comment hidden (obsolete)
Comment 170 Chris Nighswonger 2012-11-13 16:02:16 UTC Comment hidden (obsolete)
Comment 171 Chris Nighswonger 2012-11-13 16:03:11 UTC
Patch set rebased over current master.
Comment 172 Chris Nighswonger 2012-11-14 20:53:53 UTC
Created attachment 13456 [details]
Screenshot of the ugly search header after rebasing change set

After rebasing the patch set, the search header turns ugly as may be seen in the attached screenshot. Not sure why.
Comment 173 Jonathan Druart 2012-11-15 08:13:23 UTC Comment hidden (obsolete)
Comment 174 Jonathan Druart 2012-11-15 08:15:03 UTC
(In reply to comment #172)
> Created attachment 13456 [details]
> Screenshot of the ugly search header after rebasing change set
> 
> After rebasing the patch set, the search header turns ugly as may be seen in
> the attached screenshot. Not sure why.

Hi Chris,
Thanks for testing.
An error existed in the css file (caused by the rebase). Last patch fix the issue.
Comment 175 Chris Nighswonger 2012-11-15 18:06:13 UTC
Applied "as is" this change set appears to break the old updatedatabase.pl mechanism. If someone else could verify this it would be helpful.
Comment 176 Chris Nighswonger 2012-11-15 19:35:29 UTC
Comment on attachment 6414 [details] [review]
Bug 7167 testing patch

depricated in light of the mods to use *.pl and *.sql scripts in a subdir named after the version number.
Comment 177 Chris Nighswonger 2012-11-15 20:27:18 UTC
Below is the log of my testing.

At present there are three bugs (marked BUG) which prevent me from signing off on these patches. It would be good if someone else could run through this and verify these bugs as valid. Perhaps Jonathan can as well.

---------------------------------------------------

- Started from a fresh, default DB
- git-bz apply 7167
- Added a dummy update to the old updatedatabase.pl to verify functionality
- BUG: Koha fails to pick up on the version change, and I am taken to mainpage.pl
- Created a 3.09 subdir, moving the included sample files there.
- Refreshing mainpage.pl results in detection of the test update; I am redirected to admin/updatedatabase.pl
- I am notified that one update is available although there are two.
- I have DEBUG set and correctly see the "Execute" option as well as options to "Update all" or "Update" each.
- Unsetting DEBUG properly results in all options except "Update all" being removed.
- Noting that both files update files have the same base name "sample_update," I rename one "sample_update_1" and the interface now correctly shows that two updates are available. (Note: We might want to document that. I suspect it is preferrable to avoid duplicate filenames as much as possible regardless of the differing extensions.)
- Tested install of updates by clicking "Update All." The PL file applied fine while the SQL file failed due to some SQL syntax error.
- Text of error as displayed in the interface: "update_sample -- Error : 1064 => You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'INSERT INTO `systempreferences` VALUES ('TestSyspref3','2','set the level of err' at line 2;"
- BUG: A quick look at the SQL file shows that the SQL for the "complex" example is correct; I'm assuming that there is a bug in the updater code.
- Attempted to force installation of PL update which was already installed and correctly received this message: "update_sample_1 -- /home/cnighswonger/Repositories/koha.3.2.labels/installer/data/mysql/versions/3.09/update_sample_1.pl already executed in version update_sample_1 : same md5 (839b2b743adfda1ce905b1a99f2daecd)"
- Created a duplicate PL update with the same md5sum as the previously applied PL update; the updater recognized the update and redirected properly; upon attempting to apply the update, I correctly received the following message: "update_sample_2 -- /home/cnighswonger/Repositories/koha.3.2.labels/installer/data/mysql/versions/3.09/update_sample_2.pl already executed in version update_sample_1 : same md5 (839b2b743adfda1ce905b1a99f2daecd)"
- Created invalid PL and SQL update files; Selecting "Mark As Applied" behaved as expected in both cases.
- Created a PL update file with a huge syntax error in it; Upon attempting to apply it, the updater correctly returned the following error message: "update_sample_4 -- Load functions in update_sample_4.pl failed (I can't load /home/cnighswonger/Repositories/koha.3.2.labels/installer/data/mysql/versions/3.09/update_sample_4.pl. Please check the execute flag and if this file is a valid perl script (syntax error at /home/cnighswonger/Repositories/koha.3.2.labels/installer/data/mysql/versions/3.09/update_sample_4.pl line 24, near "....." ) at /home/cnighswonger/Repositories/koha.3.2.labels/C4/Update/Database.pm line 372. ) "
- BUG: I was not given the option of marking the aforementioned update as applied and consequently could not navigate away from the updater interface. This was unexpected behavior to me, as I would have expected to be able to return to normal operation of the staff interface in spite of the update failing. This probably needs to be added. If we do not want to permit forcing of this sort of thing, we should offer the user the option of completeing the updates at a later time perhaps setting a highly visible notification on the staff homepage to that effect. In any case, a more graceful recovery should be possible since we don't know the circumstances surrounding the user when such failure occurs.
- Created a non-numeric SQL update named bug_1234.sql; Updater detected this update and placed it at the top of the list; Application worked as expected.
- TRUNCATED updatedb_error, updatedb_query, updatedb_report; verified that the updater picked up all updates as unapplied
- Verified that 'misc/bin/updatedb.pl --all' applied all updates as expected.
Comment 178 Jonathan Druart 2012-11-16 11:01:13 UTC
(In reply to comment #177)
> Below is the log of my testing.

Hi Chris,
Thank you very much for testing :)

> - BUG: Koha fails to pick up on the version change, and I am taken to
> mainpage.pl

Yes, a line has to be replaced by the RM when this patch will be pushed (see installer/instal.pl l.323 to set $koha39 with your last (+1) version number.

> - Tested install of updates by clicking "Update All." The PL file applied
> fine while the SQL file failed due to some SQL syntax error.
> - Text of error as displayed in the interface: "update_sample -- Error :
> 1064 => You have an error in your SQL syntax; check the manual that
> corresponds to your MySQL server version for the right syntax to use near
> 'INSERT INTO `systempreferences` VALUES ('TestSyspref3','2','set the level
> of err' at line 2;"
> - BUG: A quick look at the SQL file shows that the SQL for the "complex"
> example is correct; I'm assuming that there is a bug in the updater code.

I can't reproduce that. Could you provide your test files please?

> - BUG: I was not given the option of marking the aforementioned update as
> applied and consequently could not navigate away from the updater interface.
> This was unexpected behavior to me, as I would have expected to be able to
> return to normal operation of the staff interface in spite of the update
> failing. This probably needs to be added. If we do not want to permit
> forcing of this sort of thing, we should offer the user the option of
> completeing the updates at a later time perhaps setting a highly visible
> notification on the staff homepage to that effect. In any case, a more
> graceful recovery should be possible since we don't know the circumstances
> surrounding the user when such failure occurs.

If a .pl file raises an error (here a syntax error), the queries are not executed (inevitably). So there are no entry in the sql tables and we don't know the "history" of this file. On master, the RM will not push an unexecutable file; on a dev install the developper will know what happen and will have to fix that (a simple way is to remove or move the file out of the versions directory).
Comment 179 Chris Nighswonger 2012-11-16 16:10:49 UTC
(In reply to comment #178)
> > - BUG: Koha fails to pick up on the version change, and I am taken to
> > mainpage.pl
> 
> Yes, a line has to be replaced by the RM when this patch will be pushed (see
> installer/instal.pl l.323 to set $koha39 with your last (+1) version number.

I'll test this out today.

> > - Tested install of updates by clicking "Update All." The PL file applied
> > fine while the SQL file failed due to some SQL syntax error.
> > - Text of error as displayed in the interface: "update_sample -- Error :
> > 1064 => You have an error in your SQL syntax; check the manual that
> > corresponds to your MySQL server version for the right syntax to use near
> > 'INSERT INTO `systempreferences` VALUES ('TestSyspref3','2','set the level
> > of err' at line 2;"
> > - BUG: A quick look at the SQL file shows that the SQL for the "complex"
> > example is correct; I'm assuming that there is a bug in the updater code.
> 
> I can't reproduce that. Could you provide your test files please?

I used the included sample files:

installer/data/mysql/versions/update_sample.pl.sample
installer/data/mysql/versions/update_sample.sql.sample

> 
> > - BUG: I was not given the option of marking the aforementioned update as
> > applied and consequently could not navigate away from the updater interface.
> > This was unexpected behavior to me, as I would have expected to be able to
> > return to normal operation of the staff interface in spite of the update
> > failing. This probably needs to be added. If we do not want to permit
> > forcing of this sort of thing, we should offer the user the option of
> > completeing the updates at a later time perhaps setting a highly visible
> > notification on the staff homepage to that effect. In any case, a more
> > graceful recovery should be possible since we don't know the circumstances
> > surrounding the user when such failure occurs.
> 
> If a .pl file raises an error (here a syntax error), the queries are not
> executed (inevitably). So there are no entry in the sql tables and we don't
> know the "history" of this file. On master, the RM will not push an
> unexecutable file; on a dev install the developper will know what happen and
> will have to fix that (a simple way is to remove or move the file out of the
> versions directory).

Ok
Comment 180 Chris Nighswonger 2012-11-16 17:10:43 UTC
(In reply to comment #179)
> (In reply to comment #178)
> > > - BUG: Koha fails to pick up on the version change, and I am taken to
> > > mainpage.pl
> > 
> > Yes, a line has to be replaced by the RM when this patch will be pushed (see
> > installer/instal.pl l.323 to set $koha39 with your last (+1) version number.
> 
> I'll test this out today.

This still does not work for me.

1. Kohaversion shown in the staff interface is "3.9.0.65 with the following database revisions applied: bug_1234_2 update_sample update_sample_2"

2. kohaversion.pl is "our $VERSION = '3.09.00.066';"

3. version in database is "3.0900065"

4. installer/install.pl line 323 is "my $koha39 = "3.0900066";"

I am still not redirected to the webinstaller to run updatedatabase.pl.

Am I missing something?
Comment 181 Paul Poulain 2012-11-16 17:13:56 UTC
(In reply to comment #180)
> 4. installer/install.pl line 323 is "my $koha39 = "3.0900066";"
> 
> I am still not redirected to the webinstaller to run updatedatabase.pl.

Checking if you need to updatedatabase cost much more than previously, so it's done only on mainpage.pl
Could it be because you're not looking at mainpage.pl ?
Comment 182 Chris Nighswonger 2012-11-16 17:28:49 UTC
(In reply to comment #181)
> (In reply to comment #180)
> > 4. installer/install.pl line 323 is "my $koha39 = "3.0900066";"
> > 
> > I am still not redirected to the webinstaller to run updatedatabase.pl.
> 
> Checking if you need to updatedatabase cost much more than previously, so
> it's done only on mainpage.pl
> Could it be because you're not looking at mainpage.pl ?

Navigating directly to cgi-bin/koha/mainpage.pl does not redirect me to the webinstaller.

Interestingly enough: adding a 'die' at 322 in installer/install.pl has no effect, so it appears that we're never getting to that block of code.

I'll look somemore.
Comment 183 Chris Nighswonger 2012-11-16 17:30:49 UTC
(In reply to comment #182)
> (In reply to comment #181)
> > (In reply to comment #180)
> > > 4. installer/install.pl line 323 is "my $koha39 = "3.0900066";"
> > > 
> > > I am still not redirected to the webinstaller to run updatedatabase.pl.
> > 
> > Checking if you need to updatedatabase cost much more than previously, so
> > it's done only on mainpage.pl
> > Could it be because you're not looking at mainpage.pl ?
> 
> Navigating directly to cgi-bin/koha/mainpage.pl does not redirect me to the
> webinstaller.
> 
> Interestingly enough: adding a 'die' at 322 in installer/install.pl has no
> effect, so it appears that we're never getting to that block of code.
> 
> I'll look somemore.

Mystery solved!

C4/Auth.pm line 588 must also have the correct version set. After I corrected this line, I was correctly redirected to the webinstaller for the dummy update using the old system.

That leaves only the issue with the "complext" SQL file example to resolve.
Comment 184 Jonathan Druart 2012-11-19 11:07:16 UTC Comment hidden (obsolete)
Comment 185 Chris Nighswonger 2012-11-19 15:34:24 UTC Comment hidden (obsolete)
Comment 186 Chris Nighswonger 2012-11-19 15:36:10 UTC Comment hidden (obsolete)
Comment 187 Chris Nighswonger 2012-11-19 15:37:50 UTC Comment hidden (obsolete)
Comment 188 Chris Nighswonger 2012-11-19 15:45:12 UTC Comment hidden (obsolete)
Comment 189 Chris Nighswonger 2012-11-19 15:45:25 UTC Comment hidden (obsolete)
Comment 190 Chris Nighswonger 2012-11-19 15:45:40 UTC Comment hidden (obsolete)
Comment 191 Chris Nighswonger 2012-11-19 15:45:54 UTC Comment hidden (obsolete)
Comment 192 Chris Nighswonger 2012-11-19 15:46:06 UTC Comment hidden (obsolete)
Comment 193 Chris Nighswonger 2012-11-19 15:46:22 UTC Comment hidden (obsolete)
Comment 194 Chris Nighswonger 2012-11-19 15:46:34 UTC Comment hidden (obsolete)
Comment 195 Chris Nighswonger 2012-11-19 15:46:54 UTC Comment hidden (obsolete)
Comment 196 Chris Nighswonger 2012-11-19 15:47:09 UTC Comment hidden (obsolete)
Comment 197 Chris Nighswonger 2012-11-19 15:47:18 UTC Comment hidden (obsolete)
Comment 198 Chris Nighswonger 2012-11-19 15:47:49 UTC Comment hidden (obsolete)
Comment 199 Chris Nighswonger 2012-11-19 15:51:14 UTC
Be carefult: I'm not sure these will apply in the order attached. Here is a snippet of git log to help, just in case.


468de5f Bug 7167: Add .pl and .sql examples
f631e06 Bug 7167: FIX merge
7e6223f Bug 7167: Don't raise an error in routine TableExists
d846366 Bug 7167 reindentation & removing dead code
53a9273 Bug 7167: We want to execute non-numeric version with the -all option
41ae4bf Bug 7167: Add dependency File::Find::Rule
0011843 Bug 7167: FIX typos & moving a script to a proper place
64bd592 Bug 7167: Improve display + factorize get_queries
07812be Bug 7167: Add .pl and .sql examples
6793ea2 Bug 7167: Changing in .sql parsing
7efbd7b Bug 7167: Remove check_coherency
fe42062 Bug 7167: Reimplementing Marcel's suggestions & fixes
3742947 Bug 7167: Improve the update.pl script
132404d Bug 7167: Now, we check versions on mainpage.pl and after login
b38a390 Bug 7167: New version for updatedatabase
Comment 200 Jonathan Druart 2012-11-19 15:52:44 UTC
Thanks Chris ! \o/
Comment 201 Paul Poulain 2012-11-28 13:50:38 UTC Comment hidden (obsolete)
Comment 202 Paul Poulain 2012-11-28 13:50:56 UTC Comment hidden (obsolete)
Comment 203 Paul Poulain 2012-11-28 13:52:37 UTC
QA comment:
 * I've squashed the 15 patches in one large
 * I've fixed 2 tiny POD errors
 * passes koha-qa.pl
 * attached is the main patch, with a tiny follow-up that could need to be adjusted before pushing the patch.

passed QA !
Comment 204 Paul Poulain 2012-11-28 13:53:19 UTC
(forgot to say that I tested the patch(es) once again, could not detect anything wrong)
Comment 205 Katrin Fischer 2012-11-28 13:59:16 UTC
Hi Paul, I know this is an important development, but because it is so important and compley I would really like to see a second QA sign off here.

I will leave final decision to our RM.
Comment 206 Jared Camins-Esakov 2012-12-03 12:39:06 UTC
If someone wanted to update the database from the command line, they could do it by running misc/bin/updatedb.pl? Is that right?

I don't see any unit tests for C4::Update::Database. This patch predates the 3.12 release cycle so I'll be somewhat flexible about that, but I'd really like to see a follow-up with unit tests.

I will be posting a follow-up that allows us to disable updates in the current updatedatabase.pl, so that I can safely copy updates to the current updatedatabase.pl script, and one that changes the name of the misnamed $koha39 variable.
Comment 207 Jonathan Druart 2012-12-03 14:53:12 UTC
(In reply to comment #206)
> If someone wanted to update the database from the command line, they could
> do it by running misc/bin/updatedb.pl? Is that right?

Yes

> I don't see any unit tests for C4::Update::Database. This patch predates the
> 3.12 release cycle so I'll be somewhat flexible about that, but I'd really
> like to see a follow-up with unit tests.

I just tried to add tests for this new module but I am not happy with the result.
Moreover I do not know if it will work on our build server. Indeed my test script only works if there is no data in the updatedb_* tables.

Jared, could you say me if you have a better idea to test it please?
Comment 208 Jonathan Druart 2012-12-03 14:53:29 UTC Comment hidden (obsolete)
Comment 209 Jared Camins-Esakov 2012-12-03 15:20:41 UTC
(In reply to comment #207)
> (In reply to comment #206)
> > If someone wanted to update the database from the command line, they could
> > do it by running misc/bin/updatedb.pl? Is that right?
> 
> Yes
> 
> > I don't see any unit tests for C4::Update::Database. This patch predates the
> > 3.12 release cycle so I'll be somewhat flexible about that, but I'd really
> > like to see a follow-up with unit tests.
> 
> I just tried to add tests for this new module but I am not happy with the
> result.
> Moreover I do not know if it will work on our build server. Indeed my test
> script only works if there is no data in the updatedb_* tables.
> 
> Jared, could you say me if you have a better idea to test it please?

Jonathan, what I would probably do is mock C4::Context->config to set intranetdir so that C4::Update::Database will find a versions directory under t/db_dependent/data. Then I'd make the tests relative, so that the system checks not that there is one update applied, but that there is one more update applied than there was when the test started.

Also, there is one more thing which I did not notice until just now: there are several package-level variables in C4::Update::Database, which will not work with Plack. $version and $list seem not to be used (but please confirm this), $VERSIONS_PATH could be moved safely into the return statement for get_versions_path, and $dbh should be retrieved in each sub that needs it. Do you think you could fix this in your follow-up?
Comment 210 Jonathan Druart 2012-12-03 15:39:59 UTC Comment hidden (obsolete)
Comment 211 Jonathan Druart 2012-12-03 16:14:30 UTC
(In reply to comment #209)

> Jonathan, what I would probably do is mock C4::Context->config to set
> intranetdir so that C4::Update::Database will find a versions directory
> under t/db_dependent/data. Then I'd make the tests relative, so that the
> system checks not that there is one update applied, but that there is one
> more update applied than there was when the test started.

I mock C4::U::D::get_versions_path which is quite similar to mock C4::Context->config.
The idea to make the tests relative seems better, thank you.
I let you (or someone else!) have a look at the following patch. All remarks are welcomed :)
Comment 212 Jonathan Druart 2012-12-03 16:15:22 UTC Comment hidden (obsolete)
Comment 213 Chris Nighswonger 2012-12-03 17:07:27 UTC Comment hidden (obsolete)
Comment 214 Chris Nighswonger 2012-12-03 17:08:01 UTC Comment hidden (obsolete)
Comment 215 Chris Nighswonger 2012-12-03 17:08:13 UTC Comment hidden (obsolete)
Comment 216 Chris Nighswonger 2012-12-03 17:08:24 UTC Comment hidden (obsolete)
Comment 217 Chris Nighswonger 2012-12-03 17:09:22 UTC
Additions apply cleanly and work as expected.
Comment 218 Jared Camins-Esakov 2012-12-03 21:28:09 UTC
I have encountered a problem, and I don't have a solution for it: right now we have the generator tag in the OPAC set to the Koha version. With non-linear updates, the Koha version becomes meaningless. Is there something we can use instead?

I also have a question about the $koha39 variable. Why is it needed? As far as I can understand, $koha39 should be equal to the version in kohaversion.pl. Couldn't we just use that and eliminate the variable in C4::Auth?
Comment 219 Jonathan Druart 2012-12-04 08:52:15 UTC
(In reply to comment #218)
> I have encountered a problem, and I don't have a solution for it: right now
> we have the generator tag in the OPAC set to the Koha version. With
> non-linear updates, the Koha version becomes meaningless. Is there something
> we can use instead?
> 
> I also have a question about the $koha39 variable. Why is it needed? As far
> as I can understand, $koha39 should be equal to the version in
> kohaversion.pl. Couldn't we just use that and eliminate the variable in
> C4::Auth?

I had this discussion with Paul a while ago but I don't remember exactly how we concluded.
What do you call the "tag generator in the OPAC"?
Is it the same number as we have in the intranet file about.tt? If yes take a look at the Comment 57 I think the proposed patch answers you.
The kohaversion.pl is still used. So we cannot remove the '$koha39' variable and we have to continue to increment the kohaversion.pl number.
Comment 220 Jared Camins-Esakov 2012-12-04 13:25:48 UTC
(In reply to comment #219)
> (In reply to comment #218)
> > I have encountered a problem, and I don't have a solution for it: right now
> > we have the generator tag in the OPAC set to the Koha version. With
> > non-linear updates, the Koha version becomes meaningless. Is there something
> > we can use instead?
> > 
> > I also have a question about the $koha39 variable. Why is it needed? As far
> > as I can understand, $koha39 should be equal to the version in
> > kohaversion.pl. Couldn't we just use that and eliminate the variable in
> > C4::Auth?
> 
> I had this discussion with Paul a while ago but I don't remember exactly how
> we concluded.
> What do you call the "tag generator in the OPAC"?

<meta name="generator" content="Koha 3.1100004" /> <!-- leave this for stats -->

> Is it the same number as we have in the intranet file about.tt? If yes take
> a look at the Comment 57 I think the proposed patch answers you.

In that

> The kohaversion.pl is still used. So we cannot remove the '$koha39' variable
> and we have to continue to increment the kohaversion.pl number.

Hm, in that case comment 10 is wrong? I'll update my patch on bug 9191 to add and use a C4::Context->final_linear_version routine, which we can then use in place of $koha39.
Comment 221 Jared Camins-Esakov 2012-12-22 21:50:18 UTC
(In reply to comment #220)
> > The kohaversion.pl is still used. So we cannot remove the '$koha39' variable
> > and we have to continue to increment the kohaversion.pl number.
> 
> Hm, in that case comment 10 is wrong? I'll update my patch on bug 9191 to
> add and use a C4::Context->final_linear_version routine, which we can then
> use in place of $koha39.

Could you confirm that this is correct, and kohaversion.pl will continue to be incremented?
Comment 222 Jonathan Druart 2012-12-23 13:08:54 UTC
(In reply to comment #221)
> (In reply to comment #220)
> > > The kohaversion.pl is still used. So we cannot remove the '$koha39' variable
> > > and we have to continue to increment the kohaversion.pl number.
> > 
> > Hm, in that case comment 10 is wrong? I'll update my patch on bug 9191 to
> > add and use a C4::Context->final_linear_version routine, which we can then
> > use in place of $koha39.
> 
> Could you confirm that this is correct, and kohaversion.pl will continue to
> be incremented?

Which point of comment 10 seems wrong to you? Here are some points addressed in the comment:

* version is checked on mainpage.pl (and here only). If syspref Version differ from kohaversion.pl, the old updatedatabase is launched. If there are updates missing from new mechanism, the updatedatabase page is reached
=> I think that it is not really what we want : "If syspref Version differ from kohaversion.pl, the old updatedatabase is launched" is wrong.

* kohaversion check on each page is now useless in Auth.pm, removed dead code
=> It is correct

* Updated installer: at the end of the process, retrieve all updates and automatically mark them "OK", as they're included in installer
=> It is not correct.

In fact, to resume what I understand:

The Bug 9191 introduces the final_linear_version routine. When you will push the new feature, the routine will return a constant 3.11.00.XXX version.
The C4::Auth::version_check routine must then be rewritten as:

    # Old updatedatabase method
    if (C4::Context->preference('Version') <= C4::Context->final_linear_version) {
        print $query->redirect("/cgi-bin/koha/installer/install.pl?step=3");
        safe_exit;
    }

    # New updatedatabase method
    unless ( C4::Update::Database::is_uptodate() ) {
        # not up-to-date, redirect to updatedatabase page
        warn "redirect to updatedatabase";
        print $query->redirect(-location => "/cgi-bin/koha/admin/updatedatabase.pl", -cookie => $cookie);
       safe_exit;
    }


(I just change the first conditionnal test)

So the kohaversion.pl could be incremented depending on the utility we have of it.

Right now I don't have the code to check if this version is just used for the about page or somewhere else.

I hope I answered to your question.
Comment 223 Jared Camins-Esakov 2012-12-23 13:20:18 UTC
(In reply to comment #222)
> (In reply to comment #221)
> > (In reply to comment #220)
> > > > The kohaversion.pl is still used. So we cannot remove the '$koha39' variable
> > > > and we have to continue to increment the kohaversion.pl number.
> > > 
> > > Hm, in that case comment 10 is wrong? I'll update my patch on bug 9191 to
> > > add and use a C4::Context->final_linear_version routine, which we can then
> > > use in place of $koha39.
> > 
> > Could you confirm that this is correct, and kohaversion.pl will continue to
> > be incremented?
> 
> Which point of comment 10 seems wrong to you? Here are some points addressed
> in the comment:
> 
> * version is checked on mainpage.pl (and here only). If syspref Version
> differ from kohaversion.pl, the old updatedatabase is launched. If there are
> updates missing from new mechanism, the updatedatabase page is reached
> => I think that it is not really what we want : "If syspref Version differ
> from kohaversion.pl, the old updatedatabase is launched" is wrong.

This is the part that I was confused by. final_linear_version replaces the $koha39 variable, not kohaversion.pl. Your analysis (that we don't want to check kohaversion.pl) agrees with mine.

> * kohaversion check on each page is now useless in Auth.pm, removed dead code
> => It is correct
> 
> * Updated installer: at the end of the process, retrieve all updates and
> automatically mark them "OK", as they're included in installer
> => It is not correct.
> 
> In fact, to resume what I understand:
> 
> The Bug 9191 introduces the final_linear_version routine. When you will push
> the new feature, the routine will return a constant 3.11.00.XXX version.
> The C4::Auth::version_check routine must then be rewritten as:
> 
>     # Old updatedatabase method
>     if (C4::Context->preference('Version') <=
> C4::Context->final_linear_version) {
>         print $query->redirect("/cgi-bin/koha/installer/install.pl?step=3");
>         safe_exit;
>     }
> 
>     # New updatedatabase method
>     unless ( C4::Update::Database::is_uptodate() ) {
>         # not up-to-date, redirect to updatedatabase page
>         warn "redirect to updatedatabase";
>         print $query->redirect(-location =>
> "/cgi-bin/koha/admin/updatedatabase.pl", -cookie => $cookie);
>        safe_exit;
>     }
> 
> 
> (I just change the first conditionnal test)
> 
> So the kohaversion.pl could be incremented depending on the utility we have
> of it.
> 
> Right now I don't have the code to check if this version is just used for
> the about page or somewhere else.
> 
> I hope I answered to your question.

Yes, Thank you.
Comment 224 Jared Camins-Esakov 2012-12-27 17:01:46 UTC
This patch has been pushed to master.
Comment 225 Jared Camins-Esakov 2012-12-27 17:04:23 UTC
I am closing this bug so that no one is tempted to use it for any follow-up comments on this feature. Any issues encountered with the new non-linear database updates need to be addressed on separate bugs.
Comment 226 Chris Cormack 2012-12-27 17:50:33 UTC
Reopening because this breaks Koha, you can't currently install

perl Makefile.PL fails with this change, I don't think that is separate bug, just this one needs to be reverted of fixed.
Comment 227 Galen Charlton 2012-12-27 18:17:44 UTC
(In reply to comment #226)
> Reopening because this breaks Koha, you can't currently install
> 
> perl Makefile.PL fails with this change, I don't think that is separate bug,
> just this one needs to be reverted of fixed.

Specifically, one way that failure manifests is:

$ perl Makefile.PL 
unable to locate Koha configuration file koha-conf.xml at /home/esi/koha/C4/Context.pm line 372.
unable to locate Koha configuration file koha-conf.xml at /home/esi/koha/C4/Context.pm line 372.
Can't call method "config" on unblessed reference at /home/esi/koha/C4/Context.pm line 790.
Compilation failed in require at /home/esi/koha/C4/Installer.pm line 26.
BEGIN failed--compilation aborted at /home/esi/koha/C4/Installer.pm line 26.
Compilation failed in require at Makefile.PL line 31.
BEGIN failed--compilation aborted at Makefile.PL line 31.

The following commit fixes the immediate problem and allows the perl Makefile.PL; make; make test sequence to work, although the patch is otherwise untested.

http://git.koha-community.org/gitweb/?p=wip/koha-equinox.git;a=commitdiff;h=aa0052119467f84a7afe33d64714e52ed90acbe4

As the TODO in the patch notes, the double "unable to locate Koha configuration file" bit when running Makefile.PL is noise.
Comment 228 Jonathan Druart 2012-12-28 09:01:45 UTC
Could you describe please, I cannot reproduce:

% git remote update
% git checkout master
% git reset --hard c34017336c3cd03f471b1c5100b9f6b4dbe589db
HEAD is now at c340173 Bug 7167: Set final updatedatabase.pl version
% perl Makefile.PL

By default, Koha can be installed in one of three ways:

standard: Install files in conformance with the Filesystem
          Hierarchy Standard (FHS).  This is the default mode
          and should be used when installing a production
          Koha system.  On Unix systems, root access is
          needed to complete a standard installation.

single:   Install files under a single directory.  This option
          is useful for installing Koha without root access, e.g.,
          on a web host that allows CGI scripts and MySQL databases
          but requires the user to keep all files under the user's
          HOME directory.

dev:      Create a set of symbolic links and configuration files to
          allow Koha to run directly from the source distribution.
          This mode is useful for developers who want to run
          Koha from a git clone.

Installation mode (dev, single, standard) [standard] ^C
Comment 229 Chris Cormack 2012-12-28 09:15:58 UTC
Do it on a fresh machine with no Koha installed (like you would when installing), or make sure your KOHA_CONF variable is not set (like it would be when installing).

eg

$ git checkout c340173
HEAD is now at c340173... Bug 7167: Set final updatedatabase.pl version

$ echo $KOHA_CONF
/home/chrisc/koha-dev/etc/koha-conf.xml

perl Makefile.PL 

Works fine

$ KOHA_CONF='' perl Makefile.PL                             
unable to locate Koha configuration file koha-conf.xml at /home/chrisc/git/catalyst-koha/C4/Context.pm line 372.
unable to locate Koha configuration file koha-conf.xml at /home/chrisc/git/catalyst-koha/C4/Context.pm line 372.
Can't call method "config" on unblessed reference at /home/chrisc/git/catalyst-koha/C4/Context.pm line 790.

$ git checkout kc/master
HEAD is now at 5a61a3b... Revert "Bug 8378 - <fine> syntax not working on overdues anymore"

$ KOHA_CONF='' perl Makefile.PL

Works fine
Comment 230 Jonathan Druart 2012-12-28 11:18:02 UTC Comment hidden (obsolete)
Comment 231 MJ Ray (software.coop) 2013-03-01 13:31:08 UTC
I am upgrading this to blocker because it blocks bug 9326 which is a blocker.
Comment 232 Jared Camins-Esakov 2013-03-01 13:38:10 UTC
Bug 9326 is a blocker only if this feature is pushed.
Comment 233 MJ Ray (software.coop) 2013-03-01 13:53:07 UTC
Could we downgrade Bug 9326, then? I don't think 9326 should block release. Also, it appears unnecessarily high in the needs-signoff list and ahead of this bug which it depends upon.
Comment 234 Jonathan Druart 2013-03-14 10:24:10 UTC
Created attachment 16113 [details] [review]
Bug 7167: New version for updatedatabase

This patch use DataTable, see BUG|BZ 6836
      - css/datatables.css
      - lib/jquery/plugins/jquery.dataTables.min.js
      - js/datatables.js

http://bugs.koha-community.org/show_bug.cgi?id=7167

Bug 7167 follow-up

Major changes:
* creating database tables for update on the fly, the  1st time the update script is called
* version is checked on mainpage.pl (and here only). If syspref Version differ from kohaversion.pl, the old updatedatabase is launched. If there are updates missing from new mechanism, the updatedatabase page is reached
* kohaversion check on each page is now useless in Auth.pm, removed dead code
* Updated installer: at the end of the process, retrieve all updates and automatically mark them "OK", as they're included in installer

Minor changes:
* adding copyright
* adding poddoc
* updating a warning, for better clarity
* switching from $$var to $var->
* small TT glitch fixed in updatedatabase.tt
* about.pl now returns the Version systempreference PLUS all the patches that have been applied

Bug 7167 follow-up perlcritic & numbers display & partial apply depending on DEBUG

* add use strict to updatedatabase, that is now perlcritic compliant
* partial apply of DB revs is now managed by DEBUG env variable = if DEBUG=0, the user can just apply every DBrev. If DEBUG=1, we're in a dev env, the user know has the option to apply DBrevs one by one
Display:
* in updatedatabase, small spelling changes
* in about.pl, remove 0 just after . (3.06.01 is displayed as 3.6.1)
* improve the display of applied numbers on about.pl
 - before this patch, if you have N, N+1, N+2, N+3 and N+10 DB rev applied, about was displaying : , N+1 / N+2 / N+3 / N+10
 - after this patch you have N......N+3 / N+10
* add ORDER BY into list_versions_already_knows to have number retrieved in the same order whatever the order they are applied

http://bugs.koha-community.org/show_bug.cgi?id=6679
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Improve the update.pl script

 * Added CLI options to update.pl
 * Call update.pl from the installer.

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Now, we check versions on mainpage.pl and after login

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Reimplementing Marcel's suggestions & fixes

 * Fixing the bugguy old version check (that was made against 3.0900000 instead of 3.0900027 -the last current kohaversion number
 * in the CLI script, if there is nothing to report, just say it

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>

Bug 7167: Remove check_coherency

As suggested by Katrin, we've removed the call to check_coherency. It intended to provide readable comments when some SQL was wrong. Removing this sub result in the SQL error being displayed. That's OK because the sysadmin or the developer can google the error, understand it, then fix it.

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Changing in .sql parsing

We first split on delimiter and then extract comments. You can now put
\n for delimiter comments.
ex:

DELIMITER ;
-- this is a comment
SELECT * FROM  my_table;
-- another comment

Before this patch, we had to write:
DELIMITER ;
-- this is a comment;
SELECT * FROM  my_table;
-- another comment;

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Add .pl and .sql examples

Those files are in version directory, so will never be executed by the updater
If you want to provide an update, do it in a 3.09/ directory (if your update is expected for 3.10 version)

Note that the updater use a md5sum checker. So, if the same update is in 2 different places, it will be detected. That will be handy for changes made on both stable and master: a library running stable will get the update when updating. When upgrading to the next major release, Koha will detect the patch has already been applied, and no error will be thrown. With the previous mechanism, a DBRev ported to stable was re-executed when upgrading to master, resulting in a nasty (but usually harmless) error message

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Improve display + factorize get_queries

Despite it's size, this patch is dealing with display questions only:
 * The text "comments" and "queries" was hardcoded in ajax-updatedb-getinfo.pl script. It has been replaced by a JSON call, returning 2 separate values, "comments:" and "queries:" is now in the template, making it translatable
 * Some minor tweak in the display (like putting things in bold, displaying OK in green, warnings in yellow and KO in red)
 * Reordering the column headers for more readability:
    * Status column is merged with availability, column is after status
    * Status/availability terms more clear: "Not applied" instead of "unknown", "Applied and OK", "Applied and failed", "Applied and forced" are the 3 other statuses
    * Removed one click to display comments on DBREv not yet applied: before the patch, one had to click "Show details", then "Get comments", now, "Get comments" is enough

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: FIX typos & moving a script to a proper place

* renamed availables to available
* renamed already_knows to already_applied
* fixed FSF & copyright headers
* removing a "use strict" because we already had use Modern::Perl
* fixed a tiny typo in about.tt

* moving update.pl to misc/bin because it's a CLI script

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Add dependency File::Find::Rule

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: We want to execute non-numeric version with the -all option

Dealing with Marcel comment 100:
> Note that the current code around line 52/53 does not
> handle that correctly:
> Argument "\x{74}\x{65}..." isn't numeric in numeric ge (>=) at
> installer/data/mysql/update.pl line 52.

Now, a non-numeric DBRev will be applied if you provide the --all parameter, without throwing the error

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167 reindentation & removing dead code

* The if (! defined $ENV{PERL5LIB}... block was wrongly intented
* The 3 lines running update.pl are useless: the update (new mechanism) is run from admin/updatedatabase.pl script. This part of install.pl is run only when you have "old style" DB revisions.

Summary:
 * old mechanism = it's run as previously, by reaching the installer/install.pl?step=3 page, that applies all revisions
 * new mechanism = when you log-in or reach mainpage.pl, you reach admin/updatedatabase.pl, where you can see what will be run, and run it

Tiny side effect = the check for old mechanism is now done *after* authentification (thus it's not done on each page call). It means that the user will have to enter login/password twice :
 * first to log-in to Koha
 * second to run installer/updatedatabase.pl?step=3
As the old mechanism is deprecated, we can expect this will happend only a few time in the history of a setup, it's not a big deal.

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Don't raise an error in routine TableExists

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: FIX merge

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167: Add .pl and .sql examples

Those files are in version directory, so will never be executed by the updater
If you want to provide an update, do it in a 3.09/ directory (if your update is expected for 3.10 version)

Note that the updater use a md5sum checker. So, if the same update is in 2 different places, it will be detected. That will be handy for changes made on both stable and master: a library running stable will get the update when updating. When upgrading to the next major release, Koha will detect the patch has already been applied, and no error will be thrown. With the previous mechanism, a DBRev ported to stable was re-executed when upgrading to master, resulting in a nasty (but usually harmless) error message

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>

Bug 7167 follow-up fix POD syntax to please koha-qa.pl
Comment 235 Jonathan Druart 2013-03-14 10:24:36 UTC
Created attachment 16114 [details] [review]
Bug 7167 follow-up setting DBrevision, before pushing

The 2 files C4/Auth.pm and install.pl *must* have the $version variable
set to what is the last old-mechanism for updatedatabase

This patch set to 3.11.00001 that is the last number when I QA this patch

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Comment 236 Jonathan Druart 2013-03-14 10:24:42 UTC
Created attachment 16115 [details] [review]
Bug 7167: Followup: Removes useless global variables

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Comment 237 Jonathan Druart 2013-03-14 10:24:49 UTC
Created attachment 16116 [details] [review]
Bug 7167: Adds Unit tests for C4::Update::Database

Signed-off-by: Chris Nighswonger <cnighswonger@foundations.edu>
Comment 238 Jonathan Druart 2013-03-14 10:24:56 UTC Comment hidden (obsolete)
Comment 239 Jonathan Druart 2013-03-14 10:26:08 UTC
(In reply to comment #238)
> Created attachment 16117 [details] [review] [review]
> Bug 7167 follow-up: allow Makefile.PL to work

Please, the last patch is really easy to QA and is waiting for 2 months.
Comment 240 Paul Poulain 2013-03-21 10:40:20 UTC
QA comment:
I tried make Makefile.PL, and it worked fine. koha-qa.pl is OK

so, passed QA (once again ;-) )
Comment 241 Paul Poulain 2013-03-21 10:41:13 UTC
Created attachment 16596 [details] [review]
[PASSED QA] Bug 7167 follow-up: allow Makefile.PL to work

$dbh should not be declared at the package level unless

one is *sure* that there will be a valid context.
TODO: fix the following noise when running perl Makefile.PL:
unable to locate Koha configuration file koha-conf.xml at /home/esi/koha/C4/Context.pm line 372.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Paul Poulain <paul.poulain@biblibre.com>
Comment 242 Jared Camins-Esakov 2013-03-22 03:00:30 UTC
I had hoped to push this early in the 3.12 release cycle (and, in fact, I did, to rather disastrous results). At this point I think it's too late for 3.12, so I am incrementing it to version 3.14 but leaving it as Passed QA so that it can be reviewed speedily by the 3.14 RM.
Comment 243 Marcel de Rooy 2013-05-23 08:29:16 UTC
Version back to master.
Comment 244 Paul Poulain 2013-07-02 09:48:13 UTC
Galen, did you forget this patch ? Any other comment ?
Comment 245 Galen Charlton 2013-07-02 14:40:08 UTC
(In reply to Paul Poulain from comment #244)
> Galen, did you forget this patch ? Any other comment ?

I'll be direct: this patch will remain low on my priority list as compared to other patches (like DBIx::Class support and functionality improvements) that offer greater benefit for less risk.

The linear application of schema updates has made Koha's upgrade process one of the smoothest that I know of for the users, and I am very wary about making it more complicated for them.

While I realize that merge conflicts can be annoying for the developers, I can at least promise as RM that I'll deal with that myself when I apply patches that update the schema.
Comment 246 Galen Charlton 2014-04-29 14:59:43 UTC
(In reply to Galen Charlton from comment #245)
> While I realize that merge conflicts can be annoying for the developers, I
> can at least promise as RM that I'll deal with that myself when I apply
> patches that update the schema.

Setting this to in discussion for future RMs to deal with.  However, I strongly feel that the patch series here presents far too much risk for little gain.
Comment 247 Paul Poulain 2014-10-10 14:27:18 UTC
abandoning this one, will try something else, more simpler, that fixes the sandbox cannot apply most patches with DB update problem