[Bug 36503] New: Add a plugin hook to modify patrons after authentication
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Bug ID: 36503 Summary: Add a plugin hook to modify patrons after authentication Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Authentication Assignee: koha-bugs@lists.koha-community.org Reporter: raphael.straub@kit.edu QA Contact: testopia@bugs.koha-community.org CC: dpavlin@rot13.org This plugin hook allows to change patron data or define the patron based on the authenticated user. We use this hook to map the authenticated user to a patron record we migrated from the former ILS based on data from the identity provider. In Koha we use OIDC/Shibboleth for authentication but we don't have the EPPN in the former ILS and therefore not in the migrated patron record. We have written a plugin that takes the card number from the identity provider, searches a matching patron record in Koha and adds the EPPN to this patron. -- 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=36503 Raphael Straub <raphael.straub@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Change sponsored?|--- |Sponsored Assignee|koha-bugs@lists.koha-commun |raphael.straub@kit.edu |ity.org | CC| |clemens.tubach@kit.edu, | |michaela.sieber@kit.edu -- 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=36503 --- Comment #1 from Raphael Straub <raphael.straub@kit.edu> --- Created attachment 164379 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164379&action=edit Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Raphael Straub <raphael.straub@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Michaela Sieber <michaela.sieber@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=20340 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Mark Hofstetter <koha@trust-box.at> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |koha@trust-box.at --- Comment #2 from Mark Hofstetter <koha@trust-box.at> --- Hi Raphael, you don't need to check if the plugins are enabled ie the if statement should be removed if ( C4::Context->config('enable_plugins') ) { .. } and could you please provide a rudimentary example plugin to test the hook -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Michael Skarupianski <michael.skarupianski@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michael.skarupianski@gmail. | |com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #3 from Clemens Tubach <clemens.tubach@kit.edu> --- Hi Marc, we have added a test plugin, see attached file. This plugins write "Bug 36503: Hook test" into field "OPAC note" from the user currently logging in. To test: 1) Install and configure SSO with OIDC, see https://wiki.koha-community.org/wiki/Testing_SSO#SAML 2) Install the attached plugin AfterLogin-458d2ef.kpz 3) Login to OPAC Using SSO with user kohadev 4) Check to see "Messages for you: Bug 36503: Hook test" after Login -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #4 from Clemens Tubach <clemens.tubach@kit.edu> --- Created attachment 164628 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164628&action=edit Plugin to test the hook -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #5 from Clemens Tubach <clemens.tubach@kit.edu> --- Hi Marc, if plugins are disabled and the if statement does not exist, we get following error message: "There was an error authenticating to external identity provider Can't call method "GetPlugins" on an undefined value at /kohadevbox/koha/Koha/Auth/Client.pm line 70." So the if statement is necessary. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Thomas Klausner <domm@plix.at> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |domm@plix.at --- Comment #6 from Thomas Klausner <domm@plix.at> --- I followed the test plan, but had to add an entry for "sso" into my "/etc/hosts": In docker-compose, keycloak runs as service name "sso" and thus is reachable from eg the OPAC via http://sso:8082. But as OAuth needs to also redirect the browser (running on my host) to the same URL, I needed to add this to the "/etc/hosts" on my host (= dev laptop): 127.0.0.1 sso Depending on your setup, you might need to do different things, just make sure that the "well_known_url" is "http://sso:8082/auth/realms/test/.well-known/openid-configuration" and that you can access this URL from your host and from inside koha testing docker (try "curl http://sso:8082", which should return some raw html like "Resource not found" and NOT "Failed to connect") -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #7 from Thomas Klausner <domm@plix.at> --- Created attachment 164893 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164893&action=edit Screenshot showing that it worked! Just a small screenshot showing that the test plan worked -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #8 from Thomas Klausner <domm@plix.at> --- Also note that the unit test does not work after running the manual test plan and installing the pluging, so run the unit test before the manual test! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Thomas Klausner <domm@plix.at> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164379|0 |1 is obsolete| | --- Comment #9 from Thomas Klausner <domm@plix.at> --- Created attachment 164894 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=164894&action=edit Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Thomas Klausner <domm@plix.at> 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=36503 Michaela Sieber <michaela.sieber@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- Hmm sounds interesting. There's all kinds of interesting/useful things you could do with a plugin hook like this. I'm not sure about the plugin method name. Despite this bug's title, I suppose you wouldn't be required to "modify" the user. This is just a method that is running after the user has been returned by a successful authentication. Hmm... I notice that there isn't a test on $patron, so the plugin author needs to make sure to add their own test to $patron to handle first time logins. I suppose that's OK, since you might want to update the $mapped_data. I wonder if the plugin name should reference OIDC/Oauth2 since at the moment it would only work for those protocols, but the hook is added to the base class Koha::Auth::Client... It seems that you'd need a different plugin hook for SAML though, so I think maybe it would be worth adding a prefix/suffix to the plugin method name... I better run, but overall I like the idea of this. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Hmm... wouldn't it make more sense to run this plugin just before returning? That way you could also have access to the $domain, and you'd have access to the default values coming from the domain in the $mapped_data. Otherwise, if you tried to change the categorycode/branchcode based on IdP data for a new patron, you wouldn't be able to do it. I won't mark as Failed QA yet, but I'm curious. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #12 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to David Cook from comment #10)
I'm not sure about the plugin method name. Despite this bug's title, I suppose you wouldn't be required to "modify" the user. This is just a method that is running after the user has been returned by a successful authentication. Hmm...
Yes, as this hook can do many things, I had the idea to name it "auth_client_get_user" after the namespace and method that calls it.
I wonder if the plugin name should reference OIDC/Oauth2 since at the moment it would only work for those protocols, but the hook is added to the base class Koha::Auth::Client...
It seems that you'd need a different plugin hook for SAML though, so I think maybe it would be worth adding a prefix/suffix to the plugin method name...
I intentionally added the hook to the base class so that all authentication methods based on it use the same hook. You are right, currently, this is only OIDC/OAuth2, but I suppose that there will be additional authentication methods in the future. If we add some prefix/suffix to the plugin method name that indicates the authentication method, all plugins depending on that hook would have to be updated if the plugin method name changes because of additional authentication methods. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #13 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to David Cook from comment #11)
Hmm... wouldn't it make more sense to run this plugin just before returning? Maybe, but I wanted to use the update on auth parameter of the domain, so I had to update the mapped data before the call to $patron->set($mapped_data). In addition, if update on auth is true, the patron is updated in the database before the hook has the opportunity to choose another patron.
That way you could also have access to the $domain, and you'd have access to the default values coming from the domain in the $mapped_data.
Otherwise, if you tried to change the categorycode/branchcode based on IdP data for a new patron, you wouldn't be able to do it. In our use case having access to the domain is not necessary, but calling the hook before $patron->…->store with the domain as an additional argument could be a good idea. Then the plugin might modify $domain->update_on_auth, $domain->default_category_id or $domain->default_library_id.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #14 from Raphael Straub <raphael.straub@kit.edu> --- Created attachment 165466 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=165466&action=edit Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #15 from Michaela Sieber <michaela.sieber@kit.edu> --- (In reply to David Cook from comment #11)
Hmm... wouldn't it make more sense to run this plugin just before returning?
That way you could also have access to the $domain, and you'd have access to the default values coming from the domain in the $mapped_data.
Otherwise, if you tried to change the categorycode/branchcode based on IdP data for a new patron, you wouldn't be able to do it.
I won't mark as Failed QA yet, but I'm curious.
Hi David, thanks for your feedback. As you can see Raphael has added a follow-up patch. We would be very glad if you could do QA and this hook would be in version 24.05 :-) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Raphael Straub from comment #13)
In our use case having access to the domain is not necessary, but calling the hook before $patron->…->store with the domain as an additional argument could be a good idea. Then the plugin might modify $domain->update_on_auth, $domain->default_category_id or $domain->default_library_id.
The more I look at Koha::Auth::Client, the more I think the already existing approach has its flaws (like arbitrarily setting the branchcode and categorycode in the $mapped_data at the end of get_user()... I think that's probably a bug in the current implementation). -- Could you describe your use case more? I know someone who might be interested in this as well, so I'd like to get more feedback... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #17 from Michaela Sieber <michaela.sieber@kit.edu> --- (In reply to David Cook from comment #16)
(In reply to Raphael Straub from comment #13)
In our use case having access to the domain is not necessary, but calling the hook before $patron->…->store with the domain as an additional argument could be a good idea. Then the plugin might modify $domain->update_on_auth, $domain->default_category_id or $domain->default_library_id.
The more I look at Koha::Auth::Client, the more I think the already existing approach has its flaws (like arbitrarily setting the branchcode and categorycode in the $mapped_data at the end of get_user()... I think that's probably a bug in the current implementation).
--
Could you describe your use case more?
I know someone who might be interested in this as well, so I'd like to get more feedback...
Hi David, our use case is described in the Description of this bug. We use this hook to match and afterwards to update the patron record via data we get from the identity provider after the patron has authenticated. All this is done behind the scenes, the patron does not recognize this process, he just uses the normal authentication process he is used to (in the university context) We do not change the branchcode or the categorycode. This is our code : sub auth_client_get_user { my ( $self, $args ) = @_; my $logger = Koha::Logger->get(); my $mapped_data = $args->{'mapped_data'}; if ( !$args->{'patron'} ) { $logger->warn('Patron not found, find patron by card number'); $logger->warn(encode_json($mapped_data)); } if ( defined $mapped_data->{'cardnumber'} ) { # Split data (e. g. kit.edu:123456789) and set the card number. my ( $card_domain, $cardnumber ) = split( /\:/, $mapped_data->{'cardnumber'} ); $mapped_data->{'cardnumber'} = $cardnumber; $logger->warn( 'cardnumber: ' . $cardnumber ); if ( !$args->{'patron'} ) { # Find the patron by card number. my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); if ( $patrons->count == 1 ) { $args->{'patron'} = $patrons->next; $logger->warn('Patron found.'); } } } return; } If you see problems in Koha::Auth::Client this should be filed in a separate bug. We think this new hook has no negative effects on existing koha community code. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_24_05_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com Status|Signed Off |Failed QA --- Comment #18 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Hi Raphael, It looks like you missed Marks comment about removing the 'enable_plugins' section... in fact you can and should just use the ->call method now. my @responses = Koha::Plugins->call($method, @args); The above should suffice for a large part of your code block introduced.. it does the check for enable_plugins, find the right plugins and checked they're enabled/disabled individually and then runs their methods.. it's also allows passing by reference so you should still be able to use the passed arguments to fill the data on return. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #19 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to Martin Renvoize from comment #18) Hi Martin,
It looks like you missed Marks comment about removing the 'enable_plugins' section... no, as Clemens wrote in comment #5 the if statement is necessary.
in fact you can and should just use the ->call method now.
my @responses = Koha::Plugins->call($method, @args);
The above should suffice for a large part of your code block introduced.. it does the check for enable_plugins, find the right plugins and checked they're enabled/disabled individually and then runs their methods.. it's also allows passing by reference so you should still be able to use the passed arguments to fill the data on return. Yes, I considered using Koha::Plugins->call but, as far as I understand, "call" calls the given method of all plugins that implement it in an arbitrary order. Because all such plugins could read/modify the mapped data and/or the patron, the order is important here. This is why I added the possibility to define a priority ($plugin->retrieve_data('priority')) on these plugins.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #20 from Raphael Straub <raphael.straub@kit.edu> --- Created attachment 166050 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166050&action=edit Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #21 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Sorry Raphael, I was distracted by all the release activity happening right now. I'm going to take a pass through this now; I didn't mean to block it. Regards the prioritising I hadn't spotted it.. we should certainly put that into 'call' at some point soon, I've been after such a feature for a while. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=32449 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #164894|0 |1 is obsolete| | --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166111 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166111&action=edit Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #165466|0 |1 is obsolete| | --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166112 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166112&action=edit Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166050|0 |1 is obsolete| | --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166113 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166113&action=edit Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #25 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166114 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166114&action=edit Bug 36503: Fix unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #26 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166115 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166115&action=edit Bug 36503: (follow-up) Simplify code by passing by reference -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #27 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166116 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166116&action=edit Bug 36503: Move and update unit test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com --- Comment #28 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- We could certainly test more thoroughly here and I was tempted to try and use the syntax we're moving towards for passing parameters... but I don't want to hold this up any further for now and don't have the time to work on further refinements. As such, I'm passing QA. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #29 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- For reference, we've been moving to: ->call( 'hook_name', { action => 'action_name', payload => { key => value, . . . } }); See the 'before_x_action' and 'after_x_action' hooks added to Items.pm for example. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #30 from Clemens Tubach <clemens.tubach@kit.edu> --- (In reply to Martin Renvoize from comment #26)
Created attachment 166115 [details] [review] Bug 36503: (follow-up) Simplify code by passing by reference
Hi Martin, thanks for your work and your time! Unfortunately this patch does not work for our plugin. Our plugin sets the patron if it is undefined: if ( !$args->{'patron'} ) { # Find the patron by card number. my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); if ( $patrons->count == 1 ) { $args->{'patron'} = $patrons->next; $logger->warn('Patron found.'); } } So, with your patch, $patron is still undefined after calling the hook. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #31 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Moving to Failed QA to resolve issue pointed out by KIT -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #32 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Clemens Tubach from comment #30)
(In reply to Martin Renvoize from comment #26)
Created attachment 166115 [details] [review] [review] Bug 36503: (follow-up) Simplify code by passing by reference
Hi Martin, thanks for your work and your time! Unfortunately this patch does not work for our plugin. Our plugin sets the patron if it is undefined:
if ( !$args->{'patron'} ) { # Find the patron by card number. my $patrons = Koha::Patrons->search( { 'cardnumber' => $cardnumber } ); if ( $patrons->count == 1 ) { $args->{'patron'} = $patrons->next; $logger->warn('Patron found.'); } }
So, with your patch, $patron is still undefined after calling the hook.
That should still work find.. have you tested? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #33 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'll add to the unit test for you -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #34 from Raphael Straub <raphael.straub@kit.edu> --- (In reply to Martin Renvoize from comment #32)
(In reply to Clemens Tubach from comment #30)
So, with your patch, $patron is still undefined after calling the hook.
That should still work find.. have you tested? Yes, and it did not work.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Not forgotten this.. just iterating on the unit test and restoring the functionality you've highlighted regressed. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166111|0 |1 is obsolete| | --- Comment #36 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166458 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166458&action=edit Bug 36503: Add a plugin hook after user authentication This plugin hook allows to change patron data or define the patron based on the authenticated user. To test: Run prove t/db_dependent/Koha/Auth/Client.t Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Thomas Klausner <domm@plix.at> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166112|0 |1 is obsolete| | --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166459 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166459&action=edit Bug 36503: (follow-up) Move the plugin hook to include the domain This allows the plugin to read/change the domain. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166113|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166460 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166460&action=edit Bug 36503: (follow-up) Use the call method of Koha::Plugins This removes the option to call multiple plugins in a prioritized order, but this is not needed if there is only one plugin that uses this hook. Sponsored-by: Karlsruhe Institute of Technology (KIT) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166114|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166461 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166461&action=edit Bug 36503: Fix unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166116|0 |1 is obsolete| | --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 166462 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166462&action=edit Bug 36503: Move and update unit test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166115|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #41 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I stand corrected.. I've remove my patch and increased the test coverage -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #42 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thank to everyone for their perseverance :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #43 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hm, a little too early - Martin, can you double check? I am having a fail: prove -v /kohadevbox/koha/t/db_dependent/Koha/Auth/Client.t /kohadevbox/koha/t/db_dependent/Koha/Auth/Client.t .. 1..4 # Subtest: get_user() tests 1..5 C4::Context->userenv not defined! at /kohadevbox/koha/Koha/Patron.pm line 1869. C4::Context->userenv not defined! at /kohadevbox/koha/Koha/Patron.pm line 1869. ok 1 - Patron correctly retrieved ok 2 - Data mapped correctly ok 3 - No surname mapped ok 4 - Is the same domain # Looks like you planned 5 tests but ran 4. not ok 1 - get_user() tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #166462|0 |1 is obsolete| | --- Comment #44 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 166531 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166531&action=edit Bug 36503: Move and update unit test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #45 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 166532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=166532&action=edit Bug 36503: (follow-up) Fix tests count Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Thanks for the quick work, Tomas! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |24.05.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #47 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|rel_24_05_candidate |release-notes-needed --- Comment #48 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Please remember to update the Hooks page in the wiki! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Michaela Sieber <michaela.sieber@kit.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | Text to go in the| |This plugin hook allows to release notes| |change patron data or | |define the patron based on | |the authenticated user. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #49 from Michaela Sieber <michaela.sieber@kit.edu> --- Is it possible to backport this hook to 23.11? I have updated the Wiki page and added a text for release notes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m --- Comment #50 from Fridolin Somers <fridolin.somers@biblibre.com> --- additional_work_needed is obsolete ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #51 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Fridolin Somers from comment #50)
additional_work_needed is obsolete ?
It was for the hooks page on the wiki, but I see it's listed there now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #52 from Fridolin Somers <fridolin.somers@biblibre.com> --- (In reply to Michaela Sieber from comment #49)
Is it possible to backport this hook to 23.11?
I have updated the Wiki page and added a text for release notes
Sure, but we are close to release, I will have a look on next month release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00 |24.05.00,23.11.07 released in| | Status|Pushed to main |Pushed to stable --- Comment #53 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.07 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #54 from Michaela Sieber <michaela.sieber@kit.edu> --- (In reply to Fridolin Somers from comment #53)
Pushed to 23.11.x for 23.11.07
Thank You! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |35536 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35536 [Bug 35536] Improve removal of Koha plugins in unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 --- Comment #55 from Fridolin Somers <fridolin.somers@biblibre.com> --- UT depends on Bug 35536 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 wainuiwitikapark@catalyst.net.nz changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |wainuiwitikapark@catalyst.n | |et.nz --- Comment #56 from wainuiwitikapark@catalyst.net.nz --- Not backporting to 23.05.x unless requested -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Pushed to oldstable CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to oldstable |Needs documenting -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36503 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #57 from David Nind <david@davidnind.com> --- Not covered in the manual. No update to the manual required. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org