[Bug 31894] New: Extend Hold hooks with more actions
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Bug ID: 31894 Summary: Extend Hold hooks with more actions Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Plugin architecture Assignee: koha-bugs@lists.koha-community.org Reporter: stefan.berndtsson@ub.gu.se QA Contact: testopia@bugs.koha-community.org 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) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #1 from Stefan Berndtsson <stefan.berndtsson@ub.gu.se> --- Created attachment 142265 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142265&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Stefan Berndtsson <stefan.berndtsson@ub.gu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |stefan.berndtsson@ub.gu.se |ity.org | Status|NEW |Needs Signoff -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142265|0 |1 is obsolete| | --- Comment #2 from David Nind <david@davidnind.com> --- Created attachment 142278 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142278&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |m.de.rooy@rijksmuseum.nl --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #4 from Stefan Berndtsson <stefan.berndtsson@ub.gu.se> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Stefan Berndtsson <stefan.berndtsson@ub.gu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #5 from Stefan Berndtsson <stefan.berndtsson@ub.gu.se> --- Setting it back to Signed off, since nothing has changed in the patch, and clarification was provided. :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 142612 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142612&action=edit Bug 31894: (QA follow-up) Move rollbacks to the end Obvious fix. This test removed my method records. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |BLOCKED QA Contact|testopia@bugs.koha-communit |m.de.rooy@rijksmuseum.nl |y.org | --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Looking here -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 142613 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142613&action=edit Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls Test plan: Run test again. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #11)
Test plan: Run test again.
It does not really help :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Failed QA --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Two other actions have the same thing: resume and suspend. But they are outside the scope here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #15 from Stefan Berndtsson <stefan.berndtsson@ub.gu.se> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142613|0 |1 is obsolete| | --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 142747 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142747&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |31734 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31734 [Bug 31734] Hooks for account, recall, indexing and more actions for holds. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Keywords| |rel_22_11_candidate --- Comment #17 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Nice :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (In reply to Marcel de Rooy from comment #16)
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>
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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #142747|0 |1 is obsolete| | --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Comment on attachment 142747 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=142747 Bug 31894: (QA follow-up) Remove unneeded get_from_storage calls Moving to 32107 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |22.11.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 22.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 --- Comment #21 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Please update documentation on https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31894 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This patch adds three more release notes| |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) Summary|Extend Hold hooks with more |Extend hold hooks with more |actions |actions Keywords|rel_22_11_candidate | -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org