Marcel, et. al., Patch 7167 has been signed off. I have pasted my QA comment here below to
get some feedback from you. Note that this patch will impact your future updates !
My comments are not strictly technical, but also on a functional level. Due to the nature of this patch, I think that I should.
Before Paul or Jonathan moves on, you may have some comments. Do you agree with me, or why not? Etc.
Thanks a lot..
Marcel
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.
I think this is a fantastic idea. I think this would greatly simplify the development and testing process.
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).
It's easier, but also a lot more dangerous in my opinion. The consensus on the caching configuration was that it was too dangerous to allow access to it in sysprefs. Based on that logic, I think using an environment variable would be the way to go.
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.)
I disagree here, too. I think unnumbered dbrevs should be shown in normal mode, so that if a developer switches to normal mode, they're able to spot "oh, oops, I forgot to reset my database."
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.
Regards, Jared -- Jared Camins-Esakov Bibliographer, C & P Bibliography Services, LLC (phone) +1 (917) 727-3445 (e-mail) jcamins@cpbibliography.com (web) http://www.cpbibliography.com/