Bug 19605

Summary: ILL backends should be pluggable through regular Koha plugins
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: ILLAssignee: Pedro Amorim <pedro.amorim>
Status: Pushed to main --- QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: black23, dcook, josef.moravec, magnus, martin.renvoize, oleonard, pedro.amorim, tomascohen
Version: Main   
Hardware: All   
OS: All   
URL: https://github.com/PTFS-Europe/koha-ill-backend-plugin
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7317
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19822
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00
Bug Depends on:    
Bug Blocks: 23578, 35581, 36650    
Attachments: Bug 19605: Add 'ill_backend' plugin category
Bug 19605: Update expandTemplate to expand_template method name
Bug 19605: Add support for ILL backends as plugins
Bug 19605: Add 'ill_backend' plugin category
Bug 19605: Update expandTemplate to expand_template method name
Bug 19605: Add support for ILL backends as plugins
Bug 19605: Add 'ill_backend' plugin category
Bug 19605: Update expandTemplate to expand_template method name
Bug 19605: Add support for ILL backends as plugins
Bug 19605: Tidy
Bug 19605: Rename method and use the instantiated plugin in the call
Bug 19605: Clarify method name
Bug 19605: Add 'ill_backend' plugin category
Bug 19605: Update expandTemplate to expand_template method name
Bug 19605: Add support for ILL backends as plugins
Bug 19605: Tidy
Bug 19605: Rename method and use the instantiated plugin in the call
Bug 19605: Clarify method name
Bug 19605: Fix after bug 35536
Bug 19605: Add 'ill_backend' plugin category
Bug 19605: Update expandTemplate to expand_template method name
Bug 19605: Add support for ILL backends as plugins
Bug 19605: Tidy
Bug 19605: Rename method and use the instantiated plugin in the call
Bug 19605: Clarify method name
Bug 19605: Fix after bug 35536
Bug 19605: Another fix after bug 35536
Bug 19605: (QA follow-up): available_backends fix reduce

Description Tomás Cohen Arazi 2017-11-09 16:54:04 UTC
Right now, the ILL module requires a configuration entry for a path in which to look for the backend implementations. This should be implemented in a more simple way for end-users.
Comment 1 Katrin Fischer 2023-01-13 14:29:26 UTC
+100 :)
Comment 2 Michal Denar 2023-01-13 18:38:22 UTC
+100 too :-)
Comment 3 Martin Renvoize 2023-06-06 07:15:51 UTC
Lets make this happen for 23.11 :)
Comment 4 Katrin Fischer 2023-06-06 11:37:47 UTC
(In reply to Martin Renvoize from comment #3)
> Lets make this happen for 23.11 :)

+ 1 :)
Comment 5 Pedro Amorim 2023-10-30 14:46:38 UTC Comment hidden (obsolete)
Comment 6 Pedro Amorim 2023-10-30 14:46:42 UTC Comment hidden (obsolete)
Comment 7 Pedro Amorim 2023-10-30 14:46:44 UTC Comment hidden (obsolete)
Comment 8 Pedro Amorim 2023-10-30 14:48:05 UTC
Instructions on converting old ILL backend into a plugin can be found at the PluginBackend sample plugin https://github.com/ammopt/koha-ill-backend-plugin:
- Create your plugin, copy the contents from your old ILL backend into the plugin folder.
- Copy the code from your old Base.pm file into the plugin file named after the plugin.
- Rename the old backend 'metadata' method to 'backend_metadata'.
- Rename the old backend 'new' method to 'new_backend'.
Comment 9 Pedro Amorim 2023-10-30 14:49:58 UTC
My suggestions is that we support both ways of loading ILL backends (old backend_dir way and this new plugin way) for a while (1 year? 2 years?) and after that period we deprecate the old backend_dir way of doing things.
Comment 10 Katrin Fischer 2023-10-30 15:00:45 UTC
(In reply to Pedro Amorim from comment #9)
> My suggestions is that we support both ways of loading ILL backends (old
> backend_dir way and this new plugin way) for a while (1 year? 2 years?) and
> after that period we deprecate the old backend_dir way of doing things.

That sounds like a good plan. We should make sure the deprecation warning is well visible, in the Relase notes and on the wiki page and maybe the manual?
Comment 11 Magnus Enger 2023-10-31 07:16:02 UTC
(In reply to Pedro Amorim from comment #9)
> My suggestions is that we support both ways of loading ILL backends (old
> backend_dir way and this new plugin way) for a while (1 year? 2 years?) and
> after that period we deprecate the old backend_dir way of doing things.

That sounds like a most excellent plan.
Comment 12 David Nind 2024-01-02 19:46:38 UTC Comment hidden (obsolete)
Comment 13 David Nind 2024-01-02 19:46:41 UTC Comment hidden (obsolete)
Comment 14 David Nind 2024-01-02 19:46:44 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2024-02-12 17:47:23 UTC
I been looking at the PluginBackend implementation, and got a bit confused about this:

sub new_backend {
    my ( $class, $params ) = @_;

    my $self = {};

    $self->{_logger} = $params->{logger} if ( $params->{logger} );
    $self->{_config} = $params->{config} if ( $params->{config} );

    bless( $self, $class );

    return $self;
}


It seems to me that in this particular implementation, as we already have a plugin instance in this context:

+    my $backend_plugin = $self->get_backend_plugin($backend_name);
+    if ($backend_plugin) {
+
+        # New way of loading backends: Through plugins
+        my $backend_plugin_class = $backend_plugin->{class};
+
+        $self->{_my_backend} = $backend_plugin_class->new_backend(

it doesn't make sense to instantiate the plugin again. Thus, the PluginBackend implementation could just be:

sub new_backend {
    my ($self,$params) = @_;

    $self->{_logger} = $params->{logger} if ( $params->{logger} );
    $self->{_config} = $params->{config} if ( $params->{config} );

    return $self;
}

I would rather rename the method to `get_ill_backend`, and leave the plugin authors the decision on what to return (the name change is a matter of taste, but I think `ill` needs to be part of the method name).

In my case, I would love to be able to (say) have my own class for the backend, so  my implementation would be written like this:

sub get_ill_backend {
    my ($self,$params) = @_;

    require INNReach::ILL::Backend;

    return INNReach::ILL::Backend->new({
        plugin => $self,
        logger => $params->{logger}
    });
}
Comment 16 Tomás Cohen Arazi 2024-03-09 13:07:07 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2024-03-09 13:07:10 UTC Comment hidden (obsolete)
Comment 18 Tomás Cohen Arazi 2024-03-09 13:07:13 UTC Comment hidden (obsolete)
Comment 19 Tomás Cohen Arazi 2024-03-09 13:07:16 UTC Comment hidden (obsolete)
Comment 20 Tomás Cohen Arazi 2024-03-09 13:07:19 UTC Comment hidden (obsolete)
Comment 21 Tomás Cohen Arazi 2024-03-09 13:07:23 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2024-03-09 13:08:07 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2024-03-09 13:08:10 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2024-03-09 13:08:13 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2024-03-09 13:08:16 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2024-03-09 13:08:20 UTC Comment hidden (obsolete)
Comment 27 Tomás Cohen Arazi 2024-03-09 13:08:23 UTC Comment hidden (obsolete)
Comment 28 Tomás Cohen Arazi 2024-03-09 13:13:15 UTC
Sorry for the noise, I forgot to stamp my signature.

Remarks:

* I will submit on a follow-up bug an idea for some error checking we are lacking here. No rush though. Will do on monday for sure.
* I didn't understand why you changed the expandTemplate method name here. No problem for me but we usually do it on a separate report.
* I'm not sure we should tag 'this is the new way' in the code, as it might be confusing for readers a couple months later. I fixed this in one place, making the code and comments assume 'plugins is the way' and the 'old one' is a 'fallback' mechanism. I prefer such terminology.

This all looks great. Awesome job! Sorry for the delay!
Comment 29 Tomás Cohen Arazi 2024-03-09 13:24:50 UTC
I've just sent a PR for adjusting the sample plugin to the proposed changes.
Comment 30 Tomás Cohen Arazi 2024-03-25 17:21:02 UTC Comment hidden (obsolete)
Comment 31 Pedro Amorim 2024-03-28 16:57:18 UTC
Created attachment 164084 [details] [review]
Bug 19605: Add 'ill_backend' plugin category

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 32 Pedro Amorim 2024-03-28 16:57:20 UTC
Created attachment 164085 [details] [review]
Bug 19605: Update expandTemplate to expand_template method name

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 33 Pedro Amorim 2024-03-28 16:57:23 UTC
Created attachment 164086 [details] [review]
Bug 19605: Add support for ILL backends as plugins

This is designed to maintain existing functionality of loading ILL
backends through the backend_dir config (default Koha/Illbackends/).

A check for a plugin of a given backend name takes precedence over a
backend of the same name loaded the old way through backend_dir, this
means that if this happens, the backend plugin is used and NOT the
backend present in backend_dir.

Old backend_dir backends AND new backend plugins coexist.

Test plan, k-t-d:
1) Enable ILLModule and install FreeForm, run:
bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh)
2) Install the plugin ILL backend example .kpz located at:
https://github.com/ammopt/koha-ill-backend-plugin/releases/tag/1.0.0
3) koha-plack --restart kohadev
4) Visit ILL requests:
/cgi-bin/koha/ill/ill-requests.pl
5) Click "+ New ILL request". Notice it lists 'FreeForm' and
   'PluginBackend'
6) Click 'PluginBackend'. Enter a '123' in pubmedid, '42' in cardnumber
   and pick a library. Click 'Marke request'
7) Notice the request is created successfully.
8) Visit plugins:
/cgi-bin/koha/plugins/plugins-home.pl
9) Click "View plugin by class". Pick "ill backend plugins". Notice the
   correct plugin is listed.
10) Click "configure" on the ILL backend plugin. Notice it's a normal
    plugin configuration page

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 34 Pedro Amorim 2024-03-28 16:57:26 UTC
Created attachment 164087 [details] [review]
Bug 19605: Tidy

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 35 Pedro Amorim 2024-03-28 16:57:29 UTC
Created attachment 164088 [details] [review]
Bug 19605: Rename method and use the instantiated plugin in the call

This patch makes the backend instantiation happen through a plugin
object method instead of a static one. This way, the (already)
instantiated plugin can be reused (if needed) in the plugin workflow
like this:

sub new_il_backend {
    my ($self, $params) = @_;
    return Custom::Backend->new(
        {
            config => $params->{config},
            logger => $params->{logger},
            plugin => $self,
        }
    );
}

The passed plugin object would then be used to retrieve whatever
plugin-defined configurations, template paths or even helper methods. On
INN-Reach, it gives the backend access to API user agents, task queue
injection, etc.

This patch also renames the method so it is more straight-forward this is
related to ILL.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 36 Pedro Amorim 2024-03-28 16:57:32 UTC
Created attachment 164089 [details] [review]
Bug 19605: Clarify method name

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 37 Pedro Amorim 2024-03-28 16:57:35 UTC
Created attachment 164090 [details] [review]
Bug 19605: Fix after bug 35536

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 38 Pedro Amorim 2024-03-28 16:57:37 UTC
Created attachment 164091 [details] [review]
Bug 19605: Another fix after bug 35536

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 39 Pedro Amorim 2024-03-28 16:59:05 UTC
(In reply to Tomás Cohen Arazi from comment #29)
> I've just sent a PR for adjusting the sample plugin to the proposed changes.

I got some errors testing it and followed up, please take a look when possible:
https://github.com/PTFS-Europe/koha-ill-backend-plugin
Comment 40 Tomás Cohen Arazi 2024-04-01 11:39:44 UTC
(In reply to Pedro Amorim from comment #39)
> (In reply to Tomás Cohen Arazi from comment #29)
> > I've just sent a PR for adjusting the sample plugin to the proposed changes.
> 
> I got some errors testing it and followed up, please take a look when
> possible:
> https://github.com/PTFS-Europe/koha-ill-backend-plugin

You're rigth with the fix. Sorry for that!
Comment 41 Pedro Amorim 2024-04-08 14:32:40 UTC
Update to the test plan:
When installing the pluginBackend, please install version 2.0.5 here:
https://github.com/PTFS-Europe/koha-ill-backend-plugin/releases/tag/v2.0.5

It contains the latest CSRF compatibility changes required to create an ILL request on master for this backend.
Comment 42 Pedro Amorim 2024-04-19 13:50:18 UTC
Created attachment 165201 [details] [review]
Bug 19605: (QA follow-up): available_backends fix reduce

available_backends was only considering dir backends for reduce, it should consider all backends, including plugin backends.

This relates to the ILLOpacbackends syspref, where if you add a backend name that happens to be a plugin, it was not disabling it for the OPAC.
This change considers all backends for Illopacbackends, regardless of if its a plugin or a directory backend.

This was made evident while writing tests for bug 36197.
Regression tests for this particular use case, and others, are part of bug 36197.
Comment 43 Katrin Fischer 2024-04-19 17:55:37 UTC
Pushed for 24.05!

Well done everyone, thank you!