Created attachment 142265 [details] [review]
Bug 31894: Extend after_hold_action hook

Hook actions added:
after_hold_action adds new actions transfer, waiting and processing

How to test:
Run tests in t/db_dependent/Koha/Plugins/Holds_hooks.t

Sponsored by: Gothenburg University Library

Created attachment 142278 [details] [review]
Bug 31894: Extend after_hold_action hook

Hook actions added:
after_hold_action adds new actions transfer, waiting and processing

How to test:
Run tests in t/db_dependent/Koha/Plugins/Holds_hooks.t

Sponsored by: Gothenburg University Library

Signed-off-by: David Nind <david@davidnind.com>

I am seeing this pattern a few times:

     $self->store();

+    Koha::Plugins->call(

+            payload => { hold => $self->get_from_storage }

You store and get back from storage right away? Please clarify why that would be needed.

I can't say why all the others did it this way. My hooks were just copies of the model used everywhere else.

I can reason about a situation where it will likely matter. If the plugin forks/threads before doing its work, there is a chance that the originally stored object has been changed after the hook is called, but before the plugin has actually done its work. By fetching a new object, this is not the case.

I also do remember having seen a situation where the state of $self after store was not the same as the newly fetched object, but I can't recall how or where this was the case.

Setting it back to Signed off, since nothing has changed in the patch, and clarification was provided. :)

Created attachment 142612 [details] [review]
Bug 31894: (QA follow-up) Move rollbacks to the end

Obvious fix. This test removed my method records.

Looking here

(In reply to Stefan Berndtsson from comment #4)
> I can't say why all the others did it this way. My hooks were just copies of
> the model used everywhere else.
Ha. Do not assume that copy/paste creates correct code :)

> I can reason about a situation where it will likely matter. If the plugin
> forks/threads before doing its work, there is a chance that the originally
> stored object has been changed after the hook is called, but before the
> plugin has actually done its work. By fetching a new object, this is not the
> case.

This cannot be correct. The interval between the store and get_from_storage is before the first plugin is called. The result of get_from_storage is passed.

> I also do remember having seen a situation where the state of $self after
> store was not the same as the newly fetched object, but I can't recall how
> or where this was the case.
Thats correct. When you pass $self to the plugin, the plugin can change it since it is a reference. That is in line with the intention:
    @responses = Koha::Plugins->call($method, @args)
Note: Pass your arguments as refs, when you want subsequent plugins to use the value updated by preceding plugins, provided that these plugins support that.

This might be a different scenario:

            my $old_me = $self->_move_to_old;

            Koha::Plugins->call(
                'after_hold_action',
                {
                    action  => 'cancel',
                    payload => { hold => $old_me->get_from_storage }

Since we only test this sub:

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

    my $action = $params->{action};
    my $hold   = $params->{payload}->{hold};

    Koha::Exception->throw(
        "after_hold_action called with action: $action, ref: " . ref($hold) );
}

We wont really know what happens in the plugin in connection to different versions of $self..

Created attachment 142613 [details] [review]
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls

Test plan:
Run test again.

(In reply to Marcel de Rooy from comment #11)
> Test plan:
> Run test again.

It does not really help :)

Stefan: Since I imagine that you have some real life plugin to connect to this hook, could you let me know the results of that plugin with the follow-ups here?
The test only does not prove much.

Changing status to reflect need for feedback

Two other actions have the same thing: resume and suspend. But they are outside the scope here.

(In reply to Marcel de Rooy from comment #13)
> Stefan: Since I imagine that you have some real life plugin to connect to
> this hook, could you let me know the results of that plugin with the
> follow-ups here?
> The test only does not prove much.

Yes. Testing plugins properly is a bit messy, but it works just fine with these follow-ups.

The plugin in question is: https://github.com/ub-digit/koha-plugin-stats-other-table

This essentially records customised statistics into its own table (specified in the plugin configuration) on multiple of the "after_*_action" hooks (hence the reason for two of the three other hook addition bug-entries).

It should be possible to use on a master as is, but be aware that it automatically creates the table in the database (if checkbox to do this is set).

With this plugin installed I did:

1. Place hold on next available item
2. Checkin an item on other library than pickup (found=T)
3. Checkin the item on the pickup library (found=W)
4. Checkout item to patron (found=F + issue)

Without this patch, this sequence records hold_place, hold_fill, issue.
With the patch it records hold_place, hold_transfer, hold_waiting, hold_fill, issue as expected.

hold_processing would be included if SIP checkin was used with that processing syspref.

Created attachment 142747 [details] [review]
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls

Test plan:
Run test again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Nice :-D

(In reply to Marcel de Rooy from comment #16)
> Created attachment 142747 [details] [review] [review]
> Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls
> 
> Test plan:
> Run test again.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

We had an interesting discussion on IRC and within the QA team now about this. I opened bug 32107 to get a consistent approach. For now I will remove the latest follow-up here.

Comment on attachment 142747 [details] [review]
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls

Moving to 32107

Pushed to master for 22.11.

Nice work everyone, thanks!

Please update documentation on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks.