Bug 35536

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>

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>

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>

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>

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>

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>

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.

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

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.

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>

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>

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>

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>

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>

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.

Needs rebase for changes on 35507

(In reply to Marcel de Rooy from comment #17)
> Needs rebase for changes on 35507

Still working here. Add some parameters to RemovePlugins.

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>

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>

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>

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>

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>

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.

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.

Created attachment 160031 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

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

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.

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>

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>

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>

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>

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>

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.

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.

Created attachment 160808 [details] [review]
Bug 35536: Remove last two references to resultset PluginData

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

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.

Rebased. Please test.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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. :)

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>

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>

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>

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>

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>

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>

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>

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>

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>

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.

(In reply to Kyle M Hall from comment #56)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks!

I love this. Thank Marcel

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

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..

Pushed for 24.05!

Well done everyone, thank you!

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>

Thx Tomas.
Does this need an additional keyword to attract RM attention?
Trying: additional_work_needed :)

(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.

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>

Second follow-up is on my branch, will push with the next batch.

Not backported to 23.11.x

No changes required to the manual.

If changes are required to the plugin development pages in the Wiki, please update them.

(In reply to Fridolin Somers from comment #68)
> Not backported to 23.11.x

In the end backported to backport Bug 36503

Not backporting to 23.05.x unless requested