Bug 35536 - Improve removal of Koha plugins in unit tests
Summary: Improve removal of Koha plugins in unit tests
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 35507
Blocks: 35548 36419
  Show dependency treegraph
 
Reported: 2023-12-11 10:30 UTC by Marcel de Rooy
Modified: 2024-04-26 20:42 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 35536: Tiny fix in PluginRoutes.t (1.20 KB, patch)
2023-12-13 14:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.85 KB, patch)
2023-12-14 10:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (2.99 KB, patch)
2023-12-14 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (5.26 KB, patch)
2023-12-14 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (10.10 KB, patch)
2023-12-14 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Remove Method->delete and reorder rollbacks (9.46 KB, patch)
2023-12-14 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: DONT PUSH !!! - Example plugin (1008 bytes, patch)
2023-12-14 10:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: DO NOT PUSH !!! - Example plugin (1010 bytes, patch)
2023-12-14 10:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.85 KB, patch)
2023-12-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (2.99 KB, patch)
2023-12-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (5.26 KB, patch)
2023-12-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (10.97 KB, patch)
2023-12-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Remove Method->delete and reorder rollbacks (9.46 KB, patch)
2023-12-14 14:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: DONT PUSH !!! - Example plugin (1008 bytes, patch)
2023-12-14 14:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.68 KB, patch)
2023-12-19 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (2.99 KB, patch)
2023-12-19 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (7.62 KB, patch)
2023-12-19 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (11.28 KB, patch)
2023-12-19 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorder rollbacks (6.35 KB, patch)
2023-12-19 15:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Refine verbose handling in some Koha::Plugins calls (10.84 KB, patch)
2023-12-19 15:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Silence tests when run from koha-qa.pl (875 bytes, patch)
2023-12-19 15:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Remove last two references to resultset PluginData (2.13 KB, patch)
2023-12-19 15:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: DO NOT PUSH !!! - Example plugin (1010 bytes, patch)
2023-12-19 15:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.68 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (2.99 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (7.62 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (11.28 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorder rollbacks (6.35 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Refine verbose handling in some Koha::Plugins calls (10.84 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Silence tests when run from koha-qa.pl (875 bytes, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Remove last two references to resultset PluginData (1.95 KB, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: DO NOT PUSH !!! - Example plugin (1010 bytes, patch)
2024-01-11 08:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.71 KB, patch)
2024-01-11 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (3.02 KB, patch)
2024-01-11 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (7.65 KB, patch)
2024-01-11 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (11.33 KB, patch)
2024-01-11 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Reorder rollbacks (6.39 KB, patch)
2024-01-11 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Refine verbose handling in some Koha::Plugins calls (10.87 KB, patch)
2024-01-11 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Silence tests when run from koha-qa.pl (903 bytes, patch)
2024-01-11 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Remove last two references to resultset PluginData (1.98 KB, patch)
2024-01-11 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: DO NOT PUSH !!! - Example plugin (1.01 KB, patch)
2024-01-11 09:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35536: Reorganize Plugins.t (10.76 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Add Koha object classes for plugin_data (3.08 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Add Koha::Plugins->RemovePlugins class method (7.70 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Add RemovePlugins calls in plugin unit tests (11.38 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Reorder rollbacks (6.45 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Refine verbose handling in some Koha::Plugins calls (10.93 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Silence tests when run from koha-qa.pl (958 bytes, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: Remove last two references to resultset PluginData (2.04 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: DO NOT PUSH !!! - Example plugin (1.07 KB, patch)
2024-01-11 16:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35536: (follow-up) Add missing koha_object(s)_class definitions (969 bytes, patch)
2024-03-22 13:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 35536: (follow-up) Fix Plugins.t for D10 (1.55 KB, patch)
2024-03-25 11:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-12-11 10:30:25 UTC
See bug 35507 comment10 a.o.
Comment 1 Marcel de Rooy 2023-12-13 14:55:21 UTC
Created attachment 159804 [details] [review]
Bug 35536: Tiny fix in PluginRoutes.t

There was a change in record counts in plugin_data and methods between
first part and last part of subtest 'needs_install use case tests'.
This fix makes the situation at the start of the last part more
similar.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2023-12-14 10:08:57 UTC
Created attachment 159820 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2023-12-14 10:09:00 UTC
Created attachment 159821 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-12-14 10:09:02 UTC
Created attachment 159822 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Note: In a subsequent patch we will remove the cleanup action
from Koha::Plugins::Method.

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-12-14 10:09:04 UTC
Created attachment 159823 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Remove superfluous $schema->resultset('PluginData')->delete;
[3] Remove superfluous use Koha::Plugins::Methods
[4] Add the line $plugins = Koha::Plugins->new in PluginRoutes.t.
    As originally done in a separate patch. This makes both parts
    of subtest 'needs_install use case tests' more consistent.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2023-12-14 10:09:06 UTC
Created attachment 159824 [details] [review]
Bug 35536: Remove Method->delete and reorder rollbacks

We removed the need for Method->delete, so we can remove
that.

Also we should now move all RemovePlugins calls BEFORE the last
rollback. Nice and clean :)

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Install a plugin before following test plan.
You could use the attached 'DONT PUSH' example patch. Having 3 recs in data
and 45 in methods with that plugin only (before/after tests).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2023-12-14 10:09:09 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2023-12-14 10:12:24 UTC
NOTE FOR QA:
 FAIL   Koha/Plugins/Data.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
 FAIL   Koha/Plugins/Method.pm
   FAIL   pod coverage
                POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666

Please ignore false failures for empty Koha object classes.
Comment 9 Marcel de Rooy 2023-12-14 10:12:45 UTC
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
t/db_dependent/ImportBatch.t ................................. ok
t/db_dependent/Koha/Patron/Consents.t ........................ ok
t/db_dependent/Koha/Patron_generate_userid.t ................. ok
t/db_dependent/Koha/Plugins/Account_hooks.t .................. ok
t/db_dependent/Koha/Plugins/BackgroundJob.t .................. ok
t/db_dependent/Koha/Plugins/Barcode_transform_hooks.t ........ ok
t/db_dependent/Koha/Plugins/Biblio_and_Items_plugin_hooks.t .. ok
t/db_dependent/Koha/Plugins/Broken.t ......................... ok
t/db_dependent/Koha/Plugins/Circulation_hooks.t .............. ok
t/db_dependent/Koha/Plugins/Holds_hooks.t .................... ok
t/db_dependent/Koha/Plugins/Patron.t ......................... 1/5 Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1'
Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1'
Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'patron_barcode_transform called with parameter: test_cn_1'
t/db_dependent/Koha/Plugins/Patron.t ......................... ok
t/db_dependent/Koha/Plugins/Plugins.t ........................ ok
t/db_dependent/Koha/Plugins/Recall_hooks.t ................... ok
t/db_dependent/Koha/Plugins/authority_hooks.t ................ ok
t/db_dependent/Koha/REST/Plugin/PluginRoutes.t ............... ok
t/db_dependent/Koha/Template/Plugin/KohaPlugins.t ............ ok
All tests successful.
Files=16, Tests=102, 130 wallclock secs ( 0.21 usr  0.05 sys + 121.79 cusr  5.89 csys = 127.94 CPU)
Result: PASS
Comment 10 Marcel de Rooy 2023-12-14 10:33:08 UTC
Created attachment 159830 [details] [review]
Bug 35536: DO NOT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.
Comment 11 Marcel de Rooy 2023-12-14 14:31:49 UTC
Created attachment 159839 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2023-12-14 14:31:51 UTC
Created attachment 159840 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-12-14 14:31:53 UTC
Created attachment 159841 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Note: In a subsequent patch we will remove the cleanup action
from Koha::Plugins::Method.

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-12-14 14:31:56 UTC
Created attachment 159842 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins:
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Remove superfluous $schema->resultset('PluginData')->delete.
[3] Remove superfluous use Koha::Plugins::Methods.
[4] Add the line $plugins = Koha::Plugins->new in PluginRoutes.t.
    As originally done in a separate patch. This makes both parts
    of subtest 'needs_install use case tests' more consistent.
[5] Add RemovePlugins now in Handler->delete too.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2023-12-14 14:31:58 UTC
Created attachment 159843 [details] [review]
Bug 35536: Remove Method->delete and reorder rollbacks

We removed the need for Method->delete, so we can remove
that.

Also we should now move all RemovePlugins calls BEFORE the last
rollback. Nice and clean :)

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Install a plugin before following test plan.
You could use the attached 'DONT PUSH' example patch. Having 3 recs in data
and 45 in methods with that plugin only (before/after tests).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2023-12-14 14:32:01 UTC
Created attachment 159844 [details] [review]
Bug 35536: DONT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.
Comment 17 Marcel de Rooy 2023-12-14 15:00:51 UTC
Needs rebase for changes on 35507
Comment 18 Marcel de Rooy 2023-12-15 11:15:06 UTC
(In reply to Marcel de Rooy from comment #17)
> Needs rebase for changes on 35507

Still working here. Add some parameters to RemovePlugins.
Comment 19 Marcel de Rooy 2023-12-19 15:56:50 UTC
Created attachment 160024 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2023-12-19 15:56:52 UTC
Created attachment 160025 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 21 Marcel de Rooy 2023-12-19 15:56:54 UTC
Created attachment 160026 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2023-12-19 15:56:57 UTC
Created attachment 160027 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins.
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Replace $schema->resultset('PluginData')->delete by destructive parameter.
[3] Add RemovePlugins too in Handler->delete too. Note that this call
    might be better off with disable? Added a comment.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2023-12-19 15:56:59 UTC
Created attachment 160028 [details] [review]
Bug 35536: Reorder rollbacks

Time to move all RemovePlugins calls BEFORE rollbacks.
Broken.t did not even include a transaction! Some modules
are removed there as well.

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Apply TestMR plugin patch (marked DO NOT PUSH).
Run perl -MKoha::Plugins -e"Koha::Plugins->new->InstallPlugins".
Check plugin records in database.
Keep those records but remove last patch from git.
Run previous prove and verify no data changes since last check.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2023-12-19 15:57:02 UTC
Created attachment 160029 [details] [review]
Bug 35536: Refine verbose handling in some Koha::Plugins calls

Three routines in Plugins got the verbose parameter on 35507.
We can refine this a bit further.
The idea here is report when you are installing plugins but not
report when just calling plugins (flooding logs).

[1] GetPlugins: Most callers do not expect (or check) results for
    failing plugins. This patch makes GetPlugins only return
    errors when passing the *errors* flag (in 2 cases).
    [a] The misc/devel script prints warnings now using verbose,
        so does not need the errors flag anymore.
    [b] plugins/plugins-home is the only case left. Tiny adjustment
        to keep current behavior. Fixed colspan in template.
        Does not need verbose in favor of 'errors' (passed to
        template).
    [c] For most calls we do not want verbose. New default is 0.

[2] InstallPlugins
    [a] Disabled verbose in plugin-upload. Not really needed.
        Added a FIXME; we need to improve individual install.
    [b] misc/devel: No warnings anymore when calling InstallPlugins
        after GetPlugins.

[3] get_enabled_plugins
    [a] Plugins->call does not need verbose.
    [b] Plugins->feature_enabled does not need it too.

Test plan:

[1] See previous plan. With TestMR data but without patch, run
misc script and go to plugins-home. Do you see load errors on
commandline or form?

[2] Run plugins/plugins-upload (uploading just some file is good
enough); verify that you do not see TestMR lines in logfile.

[3] Run t/db_dependent/Koha/Plugins/Plugins.t for the additional
test on verbose and errors flag.
Comment 25 Marcel de Rooy 2023-12-19 15:57:04 UTC
Created attachment 160030 [details] [review]
Bug 35536: Silence tests when run from koha-qa.pl

Extending the regex in Plugins::_verbose.

Test plan:
Run qa tools on patch set.
Comment 26 Marcel de Rooy 2023-12-19 15:57:07 UTC
Created attachment 160031 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t
Comment 27 Marcel de Rooy 2023-12-19 15:57:09 UTC
Created attachment 160032 [details] [review]
Bug 35536: DO NOT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.
Comment 28 Marcel de Rooy 2024-01-11 08:20:26 UTC
Created attachment 160801 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2024-01-11 08:20:29 UTC
Created attachment 160802 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2024-01-11 08:20:31 UTC
Created attachment 160803 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2024-01-11 08:20:34 UTC
Created attachment 160804 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins.
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Replace $schema->resultset('PluginData')->delete by destructive parameter.
[3] Add RemovePlugins too in Handler->delete too. Note that this call
    might be better off with disable? Added a comment.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2024-01-11 08:20:36 UTC
Created attachment 160805 [details] [review]
Bug 35536: Reorder rollbacks

Time to move all RemovePlugins calls BEFORE rollbacks.
Broken.t did not even include a transaction! Some modules
are removed there as well.

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Apply TestMR plugin patch (marked DO NOT PUSH).
Run perl -MKoha::Plugins -e"Koha::Plugins->new->InstallPlugins".
Check plugin records in database.
Keep those records but remove last patch from git.
Run previous prove and verify no data changes since last check.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2024-01-11 08:20:39 UTC
Created attachment 160806 [details] [review]
Bug 35536: Refine verbose handling in some Koha::Plugins calls

Three routines in Plugins got the verbose parameter on 35507.
We can refine this a bit further.
The idea here is report when you are installing plugins but not
report when just calling plugins (flooding logs).

[1] GetPlugins: Most callers do not expect (or check) results for
    failing plugins. This patch makes GetPlugins only return
    errors when passing the *errors* flag (in 2 cases).
    [a] The misc/devel script prints warnings now using verbose,
        so does not need the errors flag anymore.
    [b] plugins/plugins-home is the only case left. Tiny adjustment
        to keep current behavior. Fixed colspan in template.
        Does not need verbose in favor of 'errors' (passed to
        template).
    [c] For most calls we do not want verbose. New default is 0.

[2] InstallPlugins
    [a] Disabled verbose in plugin-upload. Not really needed.
        Added a FIXME; we need to improve individual install.
    [b] misc/devel: No warnings anymore when calling InstallPlugins
        after GetPlugins.

[3] get_enabled_plugins
    [a] Plugins->call does not need verbose.
    [b] Plugins->feature_enabled does not need it too.

Test plan:

[1] See previous plan. With TestMR data but without patch, run
misc script and go to plugins-home. Do you see load errors on
commandline or form?

[2] Run plugins/plugins-upload (uploading just some file is good
enough); verify that you do not see TestMR lines in logfile.

[3] Run t/db_dependent/Koha/Plugins/Plugins.t for the additional
test on verbose and errors flag.
Comment 34 Marcel de Rooy 2024-01-11 08:20:41 UTC
Created attachment 160807 [details] [review]
Bug 35536: Silence tests when run from koha-qa.pl

Extending the regex in Plugins::_verbose.

Test plan:
Run qa tools on patch set.
Comment 35 Marcel de Rooy 2024-01-11 08:20:44 UTC
Created attachment 160808 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t
Comment 36 Marcel de Rooy 2024-01-11 08:20:46 UTC
Created attachment 160809 [details] [review]
Bug 35536: DO NOT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.
Comment 37 Marcel de Rooy 2024-01-11 08:22:35 UTC
Rebased. Please test.
Comment 38 Martin Renvoize 2024-01-11 09:44:47 UTC
Created attachment 160817 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2024-01-11 09:44:50 UTC
Created attachment 160818 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2024-01-11 09:44:53 UTC
Created attachment 160819 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Martin Renvoize 2024-01-11 09:44:55 UTC
Created attachment 160820 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins.
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Replace $schema->resultset('PluginData')->delete by destructive parameter.
[3] Add RemovePlugins too in Handler->delete too. Note that this call
    might be better off with disable? Added a comment.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Martin Renvoize 2024-01-11 09:44:58 UTC
Created attachment 160821 [details] [review]
Bug 35536: Reorder rollbacks

Time to move all RemovePlugins calls BEFORE rollbacks.
Broken.t did not even include a transaction! Some modules
are removed there as well.

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Apply TestMR plugin patch (marked DO NOT PUSH).
Run perl -MKoha::Plugins -e"Koha::Plugins->new->InstallPlugins".
Check plugin records in database.
Keep those records but remove last patch from git.
Run previous prove and verify no data changes since last check.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 43 Martin Renvoize 2024-01-11 09:45:01 UTC
Created attachment 160822 [details] [review]
Bug 35536: Refine verbose handling in some Koha::Plugins calls

Three routines in Plugins got the verbose parameter on 35507.
We can refine this a bit further.
The idea here is report when you are installing plugins but not
report when just calling plugins (flooding logs).

[1] GetPlugins: Most callers do not expect (or check) results for
    failing plugins. This patch makes GetPlugins only return
    errors when passing the *errors* flag (in 2 cases).
    [a] The misc/devel script prints warnings now using verbose,
        so does not need the errors flag anymore.
    [b] plugins/plugins-home is the only case left. Tiny adjustment
        to keep current behavior. Fixed colspan in template.
        Does not need verbose in favor of 'errors' (passed to
        template).
    [c] For most calls we do not want verbose. New default is 0.

[2] InstallPlugins
    [a] Disabled verbose in plugin-upload. Not really needed.
        Added a FIXME; we need to improve individual install.
    [b] misc/devel: No warnings anymore when calling InstallPlugins
        after GetPlugins.

[3] get_enabled_plugins
    [a] Plugins->call does not need verbose.
    [b] Plugins->feature_enabled does not need it too.

Test plan:

[1] See previous plan. With TestMR data but without patch, run
misc script and go to plugins-home. Do you see load errors on
commandline or form?

[2] Run plugins/plugins-upload (uploading just some file is good
enough); verify that you do not see TestMR lines in logfile.

[3] Run t/db_dependent/Koha/Plugins/Plugins.t for the additional
test on verbose and errors flag.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 44 Martin Renvoize 2024-01-11 09:45:04 UTC
Created attachment 160823 [details] [review]
Bug 35536: Silence tests when run from koha-qa.pl

Extending the regex in Plugins::_verbose.

Test plan:
Run qa tools on patch set.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 45 Martin Renvoize 2024-01-11 09:45:06 UTC
Created attachment 160824 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2024-01-11 09:45:09 UTC
Created attachment 160825 [details] [review]
Bug 35536: DO NOT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2024-01-11 09:49:16 UTC
This is great, and all works nice and consistently so I'm signing off.

With my QA hat on.. I slightly wonder about the new Koha:: class names.. 'Data' and 'Datas' should strictly (according to british english at least) be 'Datum' and 'Data'... but I'm not sure if that's more or less confusing at the end of the day (I couldn't come up with a synonym that had a nicer plural either :( )

This is a real step forward though in my book. :)
Comment 48 Kyle M Hall 2024-01-11 16:53:21 UTC
Created attachment 160881 [details] [review]
Bug 35536: Reorganize Plugins.t

Move stuff in the middle into new subtest.
Add transaction around second set of subtests.

Test plan:
t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Kyle M Hall 2024-01-11 16:53:27 UTC
Created attachment 160882 [details] [review]
Bug 35536: Add Koha object classes for plugin_data

Test plan:
Read the patch.
The objects will be used in subsequent patches, and tested there.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Kyle M Hall 2024-01-11 16:53:29 UTC
Created attachment 160883 [details] [review]
Bug 35536: Add Koha::Plugins->RemovePlugins class method

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 51 Kyle M Hall 2024-01-11 16:53:32 UTC
Created attachment 160884 [details] [review]
Bug 35536: Add RemovePlugins calls in plugin unit tests

[1] Replace Methods->delete by RemovePlugins.
    git grep -l "Plugins::Methods->delete" | xargs sed -i -e's/Plugins::Methods->delete/Plugins->RemovePlugins/g'
[2] Replace $schema->resultset('PluginData')->delete by destructive parameter.
[3] Add RemovePlugins too in Handler->delete too. Note that this call
    might be better off with disable? Added a comment.

Test plan:
prove $(git grep -l Koha::Plugin | grep -P "^t\/db")

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Kyle M Hall 2024-01-11 16:53:34 UTC
Created attachment 160885 [details] [review]
Bug 35536: Reorder rollbacks

Time to move all RemovePlugins calls BEFORE rollbacks.
Broken.t did not even include a transaction! Some modules
are removed there as well.

Test plan:
Search for wrong order with:
  grep -Pzo "txn_rollback;\n.*RemovePlugins" $(git grep -l RemovePlugins)
  No occurrences left? Think of another grep :)
Check number of records in plugin_data/methods.
Repeat: prove $(git grep -l Koha::Plugin | grep -P "^t\/db")
And check number of records again. Same?

Bonus: Apply TestMR plugin patch (marked DO NOT PUSH).
Run perl -MKoha::Plugins -e"Koha::Plugins->new->InstallPlugins".
Check plugin records in database.
Keep those records but remove last patch from git.
Run previous prove and verify no data changes since last check.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Kyle M Hall 2024-01-11 16:53:36 UTC
Created attachment 160886 [details] [review]
Bug 35536: Refine verbose handling in some Koha::Plugins calls

Three routines in Plugins got the verbose parameter on 35507.
We can refine this a bit further.
The idea here is report when you are installing plugins but not
report when just calling plugins (flooding logs).

[1] GetPlugins: Most callers do not expect (or check) results for
    failing plugins. This patch makes GetPlugins only return
    errors when passing the *errors* flag (in 2 cases).
    [a] The misc/devel script prints warnings now using verbose,
        so does not need the errors flag anymore.
    [b] plugins/plugins-home is the only case left. Tiny adjustment
        to keep current behavior. Fixed colspan in template.
        Does not need verbose in favor of 'errors' (passed to
        template).
    [c] For most calls we do not want verbose. New default is 0.

[2] InstallPlugins
    [a] Disabled verbose in plugin-upload. Not really needed.
        Added a FIXME; we need to improve individual install.
    [b] misc/devel: No warnings anymore when calling InstallPlugins
        after GetPlugins.

[3] get_enabled_plugins
    [a] Plugins->call does not need verbose.
    [b] Plugins->feature_enabled does not need it too.

Test plan:

[1] See previous plan. With TestMR data but without patch, run
misc script and go to plugins-home. Do you see load errors on
commandline or form?

[2] Run plugins/plugins-upload (uploading just some file is good
enough); verify that you do not see TestMR lines in logfile.

[3] Run t/db_dependent/Koha/Plugins/Plugins.t for the additional
test on verbose and errors flag.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 54 Kyle M Hall 2024-01-11 16:53:38 UTC
Created attachment 160887 [details] [review]
Bug 35536: Silence tests when run from koha-qa.pl

Extending the regex in Plugins::_verbose.

Test plan:
Run qa tools on patch set.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 55 Kyle M Hall 2024-01-11 16:53:41 UTC
Created attachment 160888 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

Test plan:
Run t/db_dependent/Koha/Plugins/Plugins.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 56 Kyle M Hall 2024-01-11 16:53:43 UTC
Created attachment 160889 [details] [review]
Bug 35536: DO NOT PUSH !!! - Example plugin

After applying patch, run:
perl -MKoha::Plugins -e"Koha::Plugin->new->InstallPlugins"
This installs the TestMR plugin.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 57 Kyle M Hall 2024-01-11 17:09:48 UTC
Only "issue" is the use of Data/Datas instead of Datum/Data or something less confusing than Datum/Data. I don't consider this a blocker.
Comment 58 Marcel de Rooy 2024-01-12 07:03:17 UTC
(In reply to Kyle M Hall from comment #56)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks!
Comment 59 Tomás Cohen Arazi 2024-03-05 18:08:41 UTC
I love this. Thank Marcel
Comment 60 Katrin Fischer 2024-03-22 09:05:14 UTC
I applied the patches minus the last one and ran QA tools, unit tests are not passing:

	* Proving /kohadevbox/koha/t/db_dependent/Koha/Plugins/authority_hooks.t KO!

    #   Failed test 'AddAuthority calls the hook with action=create, id passed'
    #   at /kohadevbox/koha/t/db_dependent/Koha/Plugins/authority_hooks.t line 57.
     # found warning: Cannot determine authority type for record: 1740 at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 573.
     # found warning: Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'after_authority_action called with action: create, id: 1740'
     # expected to find warning: (?^u:after_authority_action called with action: create, id: \d+)
 
    #   Failed test 'ModAuthority calls the hook with action=modify, id passed'
    #   at /kohadevbox/koha/t/db_dependent/Koha/Plugins/authority_hooks.t line 61.
     # found warning: Cannot determine authority type for record: 1740 at /kohadevbox/koha/Koha/SearchEngine/Elasticsearch.pm line 573.
     # found warning: Plugin error (Test Plugin): Exception 'Koha::Exception' thrown 'after_authority_action called with action: modify, id: 1740'
     # expected to find warning: (?^u:after_authority_action called with action: modify, id: 1740)
     # Looks like you failed 2 tests of 3.
 
#   Failed test 'after_authority_action hook'
#   at /kohadevbox/koha/t/db_dependent/Koha/Plugins/authority_hooks.t line 70.
 # Looks like you failed 1 test of 4.
 /kohadevbox/koha/t/db_dependent/Koha/Plugins/authority_hooks.t .. 
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests
Comment 61 Marcel de Rooy 2024-03-22 09:58:15 UTC
Back to PQA. Test seems to fail with Elastic without this patch set too.
Submitted a fix on 36397 btw. Could go along with pushing this set..
Comment 62 Katrin Fischer 2024-03-22 11:28:15 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 63 Tomás Cohen Arazi 2024-03-22 13:07:00 UTC
Created attachment 163701 [details] [review]
Bug 35536: (follow-up) Add missing koha_object(s)_class definitions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 64 Marcel de Rooy 2024-03-25 08:04:39 UTC
Thx Tomas.
Does this need an additional keyword to attract RM attention?
Trying: additional_work_needed :)
Comment 65 Katrin Fischer 2024-03-25 08:59:43 UTC
(In reply to Marcel de Rooy from comment #64)
> Thx Tomas.
> Does this need an additional keyword to attract RM attention?
> Trying: additional_work_needed :)

No need, it was already pushed last week. The automatic script doesn't update the bugs with the wrong status - I forgot to do it manually.
Comment 66 Marcel de Rooy 2024-03-25 11:59:14 UTC
Created attachment 163797 [details] [review]
Bug 35536: (follow-up) Fix Plugins.t for D10

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 67 Katrin Fischer 2024-03-25 13:29:30 UTC
Second follow-up is on my branch, will push with the next batch.