|
Description
Jonathan Druart
2020-04-13 15:01:59 UTC
It's coming from the GetPlugins call in Koha/REST/Plugin/PluginRoutes.pm I have no idea how to fix that correctly. Will take care. 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 Created attachment 102942 [details] [review] Bug 25131: Regression tests 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 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). 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> Created attachment 102990 [details] [review] Bug 25131: Regression tests Signed-off-by: David Nind <david@davidnind.com> 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> (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. 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> 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> 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> (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. 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> (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! Nice work everyone! Pushed to master for 20.05 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.
(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) It's failing pretty consistently on Jekins Run t/db_dependent/Koha/Plugins/Plugins.t first Created attachment 104003 [details] [review] Bug 25131: Make sure plugin data is fresh Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> @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 Nice work everyone! Pushed to master for 20.05 Thanks Tomas :) (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. missing dependencies - not backported to 19.11.x This is still "valid". If you have enable_plugins and an empty DB you get this error at the OPAC. |