Bug 38761

Summary: Backend plugins that are disabled remain visible
Product: Koha Reporter: Pedro Amorim <pedro.amorim>
Component: ILLAssignee: Pedro Amorim <pedro.amorim>
Status: Passed QA --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: david, martin.renvoize, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 19605, 35604    
Bug Blocks:    
Attachments: Bug 38761: Add tests
Bug 38761: Don't pass the 'all' param when getting plugins
Bug 38761: Add tests
Bug 38761: Don't pass the 'all' param when getting plugins
Bug 38761: Add tests
Bug 38761: Don't pass the 'all' param when getting plugins
Bug 38761: Add tests
Bug 38761: Don't pass the 'all' param when getting plugins

Description Pedro Amorim 2024-12-20 11:58:42 UTC

    
Comment 1 Pedro Amorim 2024-12-20 12:05:02 UTC Comment hidden (obsolete)
Comment 2 Pedro Amorim 2024-12-20 12:05:04 UTC Comment hidden (obsolete)
Comment 3 Pedro Amorim 2024-12-20 12:07:14 UTC
Created attachment 175826 [details] [review]
Bug 38761: Add tests

prove t/db_dependent/Koha/ILL/Request/Config.t
Comment 4 Pedro Amorim 2024-12-20 12:07:17 UTC
Created attachment 175827 [details] [review]
Bug 38761: Don't pass the 'all' param when getting plugins

Test plan:
1) Apply tests patch. Run. Notice it fails
2) Apply this patch. Run tests patch. Notice if passes.

UI:
1) Enable ILLModule and install a backend plugin e.g.:
https://github.com/PTFS-Europe/koha-ill-backend-plugin/releases/tag/v2.0.5
2) Create an ILL request:
http://localhost:8081/cgi-bin/koha/ill/ill-requests.pl
3) Notice when you click 'New ILL request', 'PluginBackend' is listed even if the plugin is disabled. This patch fixes that.

If there are existing requests and then their respective backend is disabled, no error occurs as plugins are loaded in the background even if disabled
Comment 5 David Nind 2024-12-22 22:00:46 UTC
Tests after both patches applied:
- if no ILL backend plugins installed and enabled, the tests fail
- if an ILL backend is installed, the tests pass

Before the patches were applied (with no ILL backend plugins installed), the tests passed.

prove t/db_dependent/Koha/ILL/Request/Config.t
t/db_dependent/Koha/ILL/Request/Config.t .. 1/9 
    #   Failed test 'get_backend_plugin_names returns list of backend plugin names'
    #   at t/db_dependent/Koha/ILL/Request/Config.t line 530.
    #          got: undef
    #     expected: 'Test Plugin'
    # Looks like you failed 1 test of 3.

#   Failed test 'get_backend_plugin_names() tests'
#   at t/db_dependent/Koha/ILL/Request/Config.t line 545.
# Looks like you failed 1 test of 9.
t/db_dependent/Koha/ILL/Request/Config.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/9 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/ILL/Request/Config.t (Wstat: 256 (exited 1) Tests: 9 Failed: 1)
  Failed test:  9
  Non-zero exit status: 1
Files=1, Tests=9,  1 wallclock secs ( 0.01 usr  0.01 sys +  1.01 cusr  0.17 csys =  1.20 CPU)
Result: FAIL
Comment 6 Pedro Amorim 2024-12-24 11:31:59 UTC
Hi David, thank you. I had the dependencies wrong. Bug 35604 adds the required 'ill_backend' method to Koha/Plugin/Test.pm.

Tests should pass now.
Comment 7 David Nind 2024-12-24 12:55:09 UTC
Created attachment 175931 [details] [review]
Bug 38761: Add tests

prove t/db_dependent/Koha/ILL/Request/Config.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2024-12-24 12:55:11 UTC
Created attachment 175932 [details] [review]
Bug 38761: Don't pass the 'all' param when getting plugins

Test plan:
1) Apply tests patch. Run. Notice it fails
2) Apply this patch. Run tests patch. Notice if passes.

UI:
1) Enable ILLModule and install a backend plugin e.g.:
https://github.com/PTFS-Europe/koha-ill-backend-plugin/releases/tag/v2.0.5
2) Create an ILL request:
http://localhost:8081/cgi-bin/koha/ill/ill-requests.pl
3) Notice when you click 'New ILL request', 'PluginBackend' is listed even if the plugin is disabled. This patch fixes that.

If there are existing requests and then their respective backend is disabled, no error occurs as plugins are loaded in the background even if disabled

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 Martin Renvoize (ashimema) 2025-01-08 17:17:36 UTC
Created attachment 176271 [details] [review]
Bug 38761: Add tests

prove t/db_dependent/Koha/ILL/Request/Config.t

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize (ashimema) 2025-01-08 17:17:38 UTC
Created attachment 176272 [details] [review]
Bug 38761: Don't pass the 'all' param when getting plugins

Test plan:
1) Apply tests patch. Run. Notice it fails
2) Apply this patch. Run tests patch. Notice if passes.

UI:
1) Enable ILLModule and install a backend plugin e.g.:
https://github.com/PTFS-Europe/koha-ill-backend-plugin/releases/tag/v2.0.5
2) Create an ILL request:
http://localhost:8081/cgi-bin/koha/ill/ill-requests.pl
3) Notice when you click 'New ILL request', 'PluginBackend' is listed even if the plugin is disabled. This patch fixes that.

If there are existing requests and then their respective backend is disabled, no error occurs as plugins are loaded in the background even if disabled

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2025-01-08 17:18:09 UTC
Test driven development, QA scripts happy, No regressions, bug fixed.

Passing QA
Comment 12 Katrin Fischer 2025-01-10 18:08:48 UTC
This bugfix depends on a bigger feature patch. Any chance we could untangle?
Comment 13 Pedro Amorim 2025-01-13 10:31:58 UTC
(In reply to Katrin Fischer from comment #12)
> This bugfix depends on a bigger feature patch. Any chance we could untangle?

I think we can untangle, if we must, but the feature blocking this has sat as SO for 8 months and only recently PQA because we (PTFS-E) have QAd it ourselves.
It's its own standalone feature and should not have any impact on current functionality for those upgrading that choose not to use it.
Furthermore, we (PTFS-E) have proven time and time again that we are quick to react and fix any eventual issues introduced by our patches (and often times introduced by patches from others).
I suppose what I am saying is, we can untangle, but at the same time, can bug 35604 be considered to be reviewed by RM? For push or, ultimately if that's the case, FQA.
Comment 14 Katrin Fischer 2025-01-13 10:40:35 UTC
I was working through the bug fixes with the stable releases in mind. Of course the other bug will be reviewed, I was only wondering if we could get this one into stable releases independently.