Hi all,
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.
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.
_______________________________________________
Koha-devel mailing list
[email protected]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
