Bug 31894 - Extend hold hooks with more actions
Summary: Extend hold hooks with more actions
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Stefan Berndtsson
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 31734
  Show dependency treegraph
 
Reported: 2022-10-20 16:38 UTC by Stefan Berndtsson
Modified: 2023-06-08 22:32 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch adds three more hooks to the existing "after_hold_action", extending it to handle the different found values. * "transfer" when a hold calls "set_transfer()" (found=T) * "waiting" when a hold calls "set_waiting()" (found=W) * "processing" when a hold calls "set_processing()" (found=P)
Version(s) released in:
22.11.00


Attachments
Bug 31894: Extend after_hold_action hook (2.80 KB, patch)
2022-10-20 16:39 UTC, Stefan Berndtsson
Details | Diff | Splinter Review
Bug 31894: Extend after_hold_action hook (2.84 KB, patch)
2022-10-20 18:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 31894: (QA follow-up) Move rollbacks to the end (1.75 KB, patch)
2022-10-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls (1.20 KB, patch)
2022-10-25 14:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls (1.26 KB, patch)
2022-10-28 07:43 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 Stefan Berndtsson 2022-10-20 16:38:49 UTC
This patch adds three more hooks to the existing "after_hold_action", extending it to handle the different found values.

* "transfer" when a Hold calls "set_transfer()" (found=T)
* "waiting" when a Hold calls "set_waiting()" (found=W)
* "processing" when a Hold calls "set_processing()" (found=P)
Comment 1 Stefan Berndtsson 2022-10-20 16:39:30 UTC
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
Comment 2 David Nind 2022-10-20 18:51:56 UTC
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>
Comment 3 Marcel de Rooy 2022-10-21 06:54:58 UTC
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.
Comment 4 Stefan Berndtsson 2022-10-21 08:47:56 UTC
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.
Comment 5 Stefan Berndtsson 2022-10-21 08:49:24 UTC
Setting it back to Signed off, since nothing has changed in the patch, and clarification was provided. :)
Comment 6 Marcel de Rooy 2022-10-25 14:30:29 UTC
Created attachment 142612 [details] [review]
Bug 31894: (QA follow-up) Move rollbacks to the end

Obvious fix. This test removed my method records.
Comment 7 Marcel de Rooy 2022-10-25 14:30:54 UTC
Looking here
Comment 8 Marcel de Rooy 2022-10-25 14:40:16 UTC
(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.
Comment 9 Marcel de Rooy 2022-10-25 14:45:04 UTC
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 }
Comment 10 Marcel de Rooy 2022-10-25 14:49:03 UTC
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..
Comment 11 Marcel de Rooy 2022-10-25 14:52:25 UTC
Created attachment 142613 [details] [review]
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls

Test plan:
Run test again.
Comment 12 Marcel de Rooy 2022-10-25 14:53:00 UTC
(In reply to Marcel de Rooy from comment #11)
> Test plan:
> Run test again.

It does not really help :)
Comment 13 Marcel de Rooy 2022-10-25 14:54:39 UTC
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
Comment 14 Marcel de Rooy 2022-10-25 14:55:29 UTC
Two other actions have the same thing: resume and suspend. But they are outside the scope here.
Comment 15 Stefan Berndtsson 2022-10-26 19:15:12 UTC
(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.
Comment 16 Marcel de Rooy 2022-10-28 07:43:18 UTC
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>
Comment 17 Tomás Cohen Arazi 2022-10-28 19:39:47 UTC
Nice :-D
Comment 18 Marcel de Rooy 2022-11-04 09:14:33 UTC
(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 19 Marcel de Rooy 2022-11-04 09:15:10 UTC
Comment on attachment 142747 [details] [review]
Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls

Moving to 32107
Comment 20 Tomás Cohen Arazi 2022-11-09 13:07:07 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 21 Katrin Fischer 2022-11-09 13:10:53 UTC
Please update documentation on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks.