Bug 25131

Summary: Web installer broken if enable_plugin is set
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: blocker    
Priority: P5 - low CC: 1joynelson, katrin.fischer, kyle, martin.renvoize, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Bug Depends on: 20415    
Bug Blocks:    
Attachments: Bug 25131: Add C4::Context->needs_install
Bug 25131: Regression tests
Bug 25131: Skip loading API plugins if Koha is not installed
Bug 25131: Add C4::Context->needs_install
Bug 25131: Regression tests
Bug 25131: Skip loading API plugins if Koha is not installed
Bug 25131: Add C4::Context->needs_install
Bug 25131: Regression tests
Bug 25131: Skip loading API plugins if Koha is not installed
Bug 25131: (QA follow-up) Move test for needs_install outside the plugin
Bug 25131: Make sure plugin data is fresh

Description Jonathan Druart 2020-04-13 15:01:59 UTC
Since bug 20415, we only have the config flag to turn on/off the plugins, enable_plugins
Before, we had the syspref.

It highlights a quite big issue, if the plugins are enabled and the DB structure is not created yet, plack does not start correctly and has the following lines in logs:

DBD::mysql::st execute failed: Table 'koha_kohadev.plugin_methods' doesn't exist [for Statement "SELECT `me`.`plugin_class` FROM `plugin_methods` `me` WHERE ( `plugin_method` = ? ) GROUP BY `me`.`plugin_class`" 
with ParamValues: 0='api_namespace'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.                                                                                                                      
Error while loading /etc/koha/sites/kohadev/plack.psgi: Can't load application from file "/kohadevbox/koha/api/v1/app.pl": DBIx::Class::Storage::DBI::_dbh_execute(): Table 'koha_kohadev.plugin_methods' doesn't e
xist at /kohadevbox/koha/Koha/Plugins.pm line 89
Comment 1 Jonathan Druart 2020-04-14 10:43:00 UTC
It's coming from the GetPlugins call in Koha/REST/Plugin/PluginRoutes.pm

I have no idea how to fix that correctly.
Comment 2 Tomás Cohen Arazi 2020-04-14 15:22:00 UTC
Will take care.
Comment 3 Tomás Cohen Arazi 2020-04-14 19:33:00 UTC
Created attachment 102941 [details] [review]
Bug 25131: Add C4::Context->needs_install

This trivial patch adds a new convenient way to ask if Koha is
installed. It uses the same approach as C4::Auth:730

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/Context.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 4 Tomás Cohen Arazi 2020-04-14 19:33:05 UTC
Created attachment 102942 [details] [review]
Bug 25131: Regression tests
Comment 5 Tomás Cohen Arazi 2020-04-14 19:33:09 UTC
Created attachment 102943 [details] [review]
Bug 25131: Skip loading API plugins if Koha is not installed

This patch adds a check on Koha being actually installed to the
PluginRoutes Mojolicious plugin.

If Koha is not installed, plugin routes won't be tried to get installed.
This has the effect of making the webinstaller functional again (when
enable_plugins is set to 1).

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Reset your working branch
6. Delete your database (e.g. in koha-testing-docker):
   $ mysql -hdb -ppassword
   > DROP DATABASE koha_kohadev;
   > CREATE DATABASE koha_kohadev; \q
7. Set enable_plugins to 1 in koha-conf.xml
8. Restart all:
   $ service memcached restart
   $ koha-plack --restart kohadev
9. Open the staff interface
=> FAIL: Hangs, the logs show nasty errors (koha-plack-err)
10. Apply this patches
11. Restart all
12. Repeat 10
=> SUCCESS: The web installer shows up :-D
13. Sign off :-D
Comment 6 Jonathan Druart 2020-04-15 08:54:50 UTC
Tomas, from where is this method called?

When I looked yesterday I had the feeling that it should be fixed at higher level (ie. ->register should not be called at all if Koha is not installed yet).
Comment 7 David Nind 2020-04-15 11:22:31 UTC
Created attachment 102989 [details] [review]
Bug 25131: Add C4::Context->needs_install

This trivial patch adds a new convenient way to ask if Koha is
installed. It uses the same approach as C4::Auth:730

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/Context.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2020-04-15 11:22:35 UTC
Created attachment 102990 [details] [review]
Bug 25131: Regression tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2020-04-15 11:22:38 UTC
Created attachment 102991 [details] [review]
Bug 25131: Skip loading API plugins if Koha is not installed

This patch adds a check on Koha being actually installed to the
PluginRoutes Mojolicious plugin.

If Koha is not installed, plugin routes won't be tried to get installed.
This has the effect of making the webinstaller functional again (when
enable_plugins is set to 1).

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Reset your working branch
6. Delete your database (e.g. in koha-testing-docker):
   $ mysql -hdb -ppassword
   > DROP DATABASE koha_kohadev;
   > CREATE DATABASE koha_kohadev; \q
7. Set enable_plugins to 1 in koha-conf.xml
8. Restart all:
   $ service memcached restart
   $ koha-plack --restart kohadev
9. Open the staff interface
=> FAIL: Hangs, the logs show nasty errors (koha-plack-err)
10. Apply this patches
11. Restart all
12. Repeat 10
=> SUCCESS: The web installer shows up :-D
13. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Tomás Cohen Arazi 2020-04-15 11:45:44 UTC
(In reply to Jonathan Druart from comment #6)
> Tomas, from where is this method called?
> 
> When I looked yesterday I had the feeling that it should be fixed at higher
> level (ie. ->register should not be called at all if Koha is not installed
> yet).

That's a valid point. I had the same doubt but decided to do it there because of the repeated code. I can fix it to skip loading the plugin in V1.pm if you guys prefer.
Comment 11 Kyle M Hall 2020-04-15 12:33:30 UTC
Created attachment 102995 [details] [review]
Bug 25131: Add C4::Context->needs_install

This trivial patch adds a new convenient way to ask if Koha is
installed. It uses the same approach as C4::Auth:730

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ t/Context.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Kyle M Hall 2020-04-15 12:33:40 UTC
Created attachment 102996 [details] [review]
Bug 25131: Regression tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2020-04-15 12:33:43 UTC
Created attachment 102997 [details] [review]
Bug 25131: Skip loading API plugins if Koha is not installed

This patch adds a check on Koha being actually installed to the
PluginRoutes Mojolicious plugin.

If Koha is not installed, plugin routes won't be tried to get installed.
This has the effect of making the webinstaller functional again (when
enable_plugins is set to 1).

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/REST/Plugin/PluginRoutes.t
=> FAIL: Tests fail!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Reset your working branch
6. Delete your database (e.g. in koha-testing-docker):
   $ mysql -hdb -ppassword
   > DROP DATABASE koha_kohadev;
   > CREATE DATABASE koha_kohadev; \q
7. Set enable_plugins to 1 in koha-conf.xml
8. Restart all:
   $ service memcached restart
   $ koha-plack --restart kohadev
9. Open the staff interface
=> FAIL: Hangs, the logs show nasty errors (koha-plack-err)
10. Apply this patches
11. Restart all
12. Repeat 10
=> SUCCESS: The web installer shows up :-D
13. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2020-04-15 12:34:54 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Jonathan Druart from comment #6)
> > Tomas, from where is this method called?
> > 
> > When I looked yesterday I had the feeling that it should be fixed at higher
> > level (ie. ->register should not be called at all if Koha is not installed
> > yet).
> 
> That's a valid point. I had the same doubt but decided to do it there
> because of the repeated code. I can fix it to skip loading the plugin in
> V1.pm if you guys prefer.

Leaving this call to the RM.
Comment 15 Tomás Cohen Arazi 2020-04-15 14:50:37 UTC
Created attachment 103015 [details] [review]
Bug 25131: (QA follow-up) Move test for needs_install outside the plugin

This follow-up patch moves the check outside the register routine. The
Mojo plugin won't be loaded unless there's no need to install.

As the original tests rely on V1.pm to be able to load the plugin, they
are still valid, and they should still pass as there's no behaviour
change.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Jonathan Druart 2020-04-15 14:54:29 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> Created attachment 103015 [details] [review] [review]
> Bug 25131: (QA follow-up) Move test for needs_install outside the plugin
> 
> This follow-up patch moves the check outside the register routine. The
> Mojo plugin won't be loaded unless there's no need to install.
> 
> As the original tests rely on V1.pm to be able to load the plugin, they
> are still valid, and they should still pass as there's no behaviour
> change.
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

The startup method seems to be a better place, thanks Tomás!
Comment 17 Martin Renvoize 2020-04-16 18:21:46 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 18 Jonathan Druart 2020-04-20 12:26:20 UTC
Test is failing:

t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 2/3
    #   Failed test 'Plugin enabled, route defined as C4::Context->needs_install is false'
    #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 164.
    # Looks like you failed 1 test of 2.
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 3/3 
#   Failed test 'needs_install use case tests'
#   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 169.
Comment 19 Tomás Cohen Arazi 2020-04-22 14:42:29 UTC
(In reply to Jonathan Druart from comment #18)
> Test is failing:
> 
> t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 2/3
>     #   Failed test 'Plugin enabled, route defined as
> C4::Context->needs_install is false'
>     #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 164.
>     # Looks like you failed 1 test of 2.
> t/db_dependent/Koha/REST/Plugin/PluginRoutes.t .. 3/3 
> #   Failed test 'needs_install use case tests'
> #   at t/db_dependent/Koha/REST/Plugin/PluginRoutes.t line 169.

It passes for me, can you give me more details on your env? I'm using plain koha-testing-docker (i.e. stretch and MySQL 5.5)
Comment 20 Martin Renvoize 2020-04-22 15:13:03 UTC
It's failing pretty consistently on Jekins
Comment 21 Jonathan Druart 2020-04-24 09:05:31 UTC
Run t/db_dependent/Koha/Plugins/Plugins.t first
Comment 22 Tomás Cohen Arazi 2020-04-29 22:12:33 UTC
Created attachment 104003 [details] [review]
Bug 25131: Make sure plugin data is fresh

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-04-29 22:20:38 UTC
@RM: there you have a fix. I still think we should revamp the plugins system. The difficulty to properly write clean tests for this is evidence some changes are required in our architecture. Anyway, I love our plugins :-D
Comment 24 Martin Renvoize 2020-04-30 07:30:56 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 25 Martin Renvoize 2020-04-30 07:31:27 UTC
Thanks Tomas :)
Comment 26 Martin Renvoize 2020-04-30 07:33:40 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> @RM: there you have a fix. I still think we should revamp the plugins
> system. The difficulty to properly write clean tests for this is evidence
> some changes are required in our architecture. Anyway, I love our plugins :-D

Agreed, it's high on my priority list to really polish and improve our plugins architecture next cycle.. I had hoped to get a little further this cycle but I think we've all been tied up with other things.  Let's make a concerted effort to get the install process, signatures and metadata verification workflows clear, working and well documented next cycle.
Comment 27 Joy Nelson 2020-05-05 22:43:09 UTC
missing dependencies - not backported to 19.11.x
Comment 28 Jonathan Druart 2020-05-22 14:22:14 UTC
This is still "valid".
If you have enable_plugins and an empty DB you get this error at the OPAC.