[Koha-devel] Bug 7167 New updatedatabase

[Jared Camins-Esakov]

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 at cpbibliography.com
(web) http://www.cpbibliography.com/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: </pipermail/koha-devel/attachments/20120613/76ba1db5/attachment-0001.htm>