Attachment 147704 Details for Bug 32426 – Bug 32426: Patron::generate_userid - add plugin hook

[patch] Bug 32426: Patron::generate_userid - add plugin hook

Bug-32426-Patrongenerateuserid---add-plugin-hook.patch (text/plain), 12.40 KB, created by Kyle M Hall (khall) on 2023-03-03 16:08:35 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2023-03-03 16:08:35 UTC

Size: 12.40 KB

patch

obsolete

>From 13498c321015114f8c65eb53e9bdabe808adb002 Mon Sep 17 00:00:00 2001
>From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Date: Mon, 5 Dec 2022 15:17:37 +0100
>Subject: [PATCH] Bug 32426: Patron::generate_userid - add plugin hook
>
>The hook is called patron_generate_userid. How suitable.
>The existing generate_userid tests and new tests with mocked
>plugins are merged into a new test script.
>
>Test plan:
>Run t/db_dependent/Koha/Patron_generate_userid.t
>Run t/db_dependent/Koha/Patrons.t
>
>Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
>Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
>---
> Koha/Patron.pm                               |  35 +++-
> t/db_dependent/Koha/Patron_generate_userid.t | 158 +++++++++++++++++++
> t/db_dependent/Koha/Patrons.t                |  55 +------
> 3 files changed, 190 insertions(+), 58 deletions(-)
> create mode 100755 t/db_dependent/Koha/Patron_generate_userid.t
>
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index 9282b38307..9309ed10c7 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -1815,16 +1815,43 @@ sub has_valid_userid {
> 
> =head3 generate_userid
> 
>-my $patron = Koha::Patron->new( $params );
>-$patron->generate_userid
>+    $patron->generate_userid;
> 
>-Generate a userid using the $surname and the $firstname (if there is a value in $firstname).
>+    If you do not have a plugin for generating a userid, we will call
>+    the legacy method here that returns firstname.surname[.number],
>+    where number is an optional suffix to make the userid unique.
>+    (Its behavior has not been changed on bug 32426.)
> 
>-Set a generated userid ($firstname.$surname if there is a $firstname, or $surname if there is no value in $firstname) plus offset (0 if the $userid is unique, or a higher numeric value if not unique).
>+    If you have plugin(s), the first valid response will be used.
>+    A plugin is assumed to return a valid userid as suggestion, but not
>+    assumed to save it already.
>+    Does not fallback to legacy (you could arrange for that in your plugin).
>+    Clears userid when there are no valid plugin responses.
> 
> =cut
> 
> sub generate_userid {
>+    my ( $self ) = @_;
>+    my @responses = Koha::Plugins->call(
>+        'patron_generate_userid', { patron => $self },
>+    );
>+    unless( @responses ) {
>+        # Empty list only possible when there are NO enabled plugins for this method.
>+        # In that case we provide legacy response.
>+        return $self->_generate_userid_legacy;
>+    }
>+    # If a plugin returned false value or invalid value, we do however not return
>+    # legacy response. The plugins should deal with that themselves. So we prevent
>+    # unexpected/unwelcome legacy codes for plugin failures.
>+    foreach my $response ( grep { $_ } @responses ) {
>+        $self->userid( $response );
>+        return $self if $self->has_valid_userid;
>+    }
>+    $self->userid(undef);
>+    return $self;
>+}
>+
>+sub _generate_userid_legacy { # as we always did
>     my ($self) = @_;
>     my $offset = 0;
>     my $firstname = $self->firstname // q{};
>diff --git a/t/db_dependent/Koha/Patron_generate_userid.t b/t/db_dependent/Koha/Patron_generate_userid.t
>new file mode 100755
>index 0000000000..d44753d883
>--- /dev/null
>+++ b/t/db_dependent/Koha/Patron_generate_userid.t
>@@ -0,0 +1,158 @@
>+#!/usr/bin/perl
>+
>+# Copyright 2022 Rijksmuseum, Koha development team
>+#
>+# This file is part of Koha
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+use utf8;
>+#use Data::Dumper;
>+use Test::More tests => 2;
>+use Test::Warn;
>+use Test::MockModule;
>+use Test::MockObject;
>+
>+use t::lib::Mocks;
>+use t::lib::TestBuilder;
>+
>+use Koha::Database;
>+use Koha::Patrons;
>+use Koha::Plugins;
>+
>+our $schema = Koha::Database->new->schema;
>+$schema->storage->txn_begin;
>+our $builder = t::lib::TestBuilder->new;
>+our $expected_plugins = [];
>+
>+sub mockedGetPlugins {
>+    my @plugins;
>+    foreach my $p ( @$expected_plugins ) {
>+        my $object = Test::MockObject->new;
>+        my $method;
>+        if( $p eq 'email' ) {
>+            $method = sub { return $_[1]->{patron}->email; };
>+        } elsif( $p eq 'firstname' ) {
>+            $method = sub { return $_[1]->{patron}->firstname. ($_[1]->{patron}->id // 0); };
>+        } elsif( $p eq 'baduserid' ) {
>+            $method = sub { return ''; }; # bad return
>+        } elsif( $p eq 'die' ) {
>+            $method = sub { die; };
>+        } elsif( $p eq 'undef' ) {
>+            $method = sub { return; };
>+        } else { # borrowernumber
>+            $method = sub { return $_[1]->{patron}->id // 0; };
>+        }
>+        $object->mock('patron_generate_userid', $method);
>+        $object->mock('get_metadata', sub { return { name => $p }}); # called when warning from ->call
>+        push @plugins, $object;
>+    }
>+    return @plugins;
>+}
>+
>+subtest 'generate_userid (legacy, without plugins)' => sub {
>+    plan tests => 7;
>+
>+    t::lib::Mocks::mock_config('enable_plugins', 0);
>+
>+    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
>+    my $patron_category = $builder->build_object(
>+        {
>+            class => 'Koha::Patron::Categories',
>+            value => { category_type => 'P', enrolmentfee => 0 }
>+        }
>+    );
>+    my %data = (
>+        cardnumber   => "123456789",
>+        firstname    => "TÃ´mÃ sÃtÃ³",
>+        surname      => "ÃonÃ©",
>+        categorycode => $patron_category->categorycode,
>+        branchcode   => $library->branchcode,
>+    );
>+
>+    my $expected_userid_patron_1 = 'tomasito.none';
>+    my $new_patron = Koha::Patron->new({ firstname => $data{firstname}, surname => $data{surname} } );
>+    $new_patron->generate_userid;
>+    my $userid = $new_patron->userid;
>+    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
>+    my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
>+    my $patron_1 = Koha::Patrons->find($borrowernumber);
>+    is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' );
>+
>+    $new_patron->generate_userid;
>+    $userid = $new_patron->userid;
>+    is( $userid, $expected_userid_patron_1 . '1', 'generate_userid should generate the userid we expect' );
>+    $data{cardnumber} = '987654321';
>+    my $new_borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
>+    my $patron_2 = Koha::Patrons->find($new_borrowernumber);
>+    isnt( $patron_2->userid, 'tomasito',
>+        "Patron with duplicate userid has new userid generated" );
>+    is( $patron_2->userid, $expected_userid_patron_1 . '1', # TODO we could make that configurable
>+        "Patron with duplicate userid has new userid generated (1 is appended" );
>+
>+    $new_patron->generate_userid;
>+    $userid = $new_patron->userid;
>+    is( $userid, $expected_userid_patron_1 . '2', 'generate_userid should generate the userid we expect' );
>+
>+    $patron_1 = Koha::Patrons->find($borrowernumber);
>+    $patron_1->userid(undef);
>+    $patron_1->generate_userid;
>+    $userid = $patron_1->userid;
>+    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
>+
>+    # Cleanup
>+    $patron_1->delete;
>+    $patron_2->delete;
>+};
>+
>+subtest 'Plugins for generate_userid' => sub {
>+    plan tests => 6;
>+    t::lib::Mocks::mock_config('enable_plugins', 1);
>+
>+    my $auth = Test::MockModule->new( 'Koha::Plugins' );
>+    $auth->mock( 'GetPlugins', \&mockedGetPlugins );
>+    $auth->mock( 'get_enabled_plugins', \&mockedGetPlugins );
>+
>+    # Check the email plugin
>+    $expected_plugins = [ 'email' ];
>+    my $patron1 = $builder->build_object({ class => 'Koha::Patrons', value => { email => 'test@domain.com' } });
>+    $patron1->generate_userid;
>+    is( $patron1->userid, 'test@domain.com', 'generated userid from email plugin' );
>+
>+    # Expect second response from firstname, because empty string from baduserid is not valid
>+    $expected_plugins = [ 'baduserid', 'firstname', 'email' ];
>+    $patron1->generate_userid;
>+    my $reg = $patron1->firstname. '\d+';
>+    like( $patron1->userid, qr/$reg/, 'ignored baduserid, generated userid from firstname plugin' );
>+
>+    # Expect third response from fallback for wrong_method, catch warning from die plugin
>+    $expected_plugins = [ 'die', 'baduserid', 'wrong_method', 'firstname', 'email' ];
>+    warning_like { $patron1->generate_userid; } qr/Plugin error $die$: Died/, 'Caught warn for die plugin';
>+    like( $patron1->userid, qr/^\d+$/, 'generated borrowernumber userid from plugin when given wrong_method' );
>+    $patron1->delete;
>+
>+    # Testing with an object not in storage; unknown should return id 0, the plugin undef returns undef :)
>+    $expected_plugins = [ 'unknown', 'undef' ];
>+    $patron1= Koha::Patron->new({ firstname => 'A', surname => 'B', email => 'test2@domain.com', userid => 'user999' });
>+    $patron1->generate_userid;
>+    is( $patron1->userid, undef, 'No valid plugin responses' );
>+
>+    # Finally pass no plugins, so we would expect legacy response
>+    $expected_plugins = [];
>+    $patron1->generate_userid;
>+    is( $patron1->userid, 'a.b', 'No plugins: legacy response' );
>+};
>+
>+$schema->storage->txn_rollback;
>diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
>index 8684c8aa15..25d2fbd6c5 100755
>--- a/t/db_dependent/Koha/Patrons.t
>+++ b/t/db_dependent/Koha/Patrons.t
>@@ -19,7 +19,7 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 45;
>+use Test::More tests => 44;
> use Test::Warn;
> use Test::Exception;
> use Test::MockModule;
>@@ -1599,59 +1599,6 @@ subtest 'userid_is_valid' => sub {
>     $patron_3->delete;
> };
> 
>-subtest 'generate_userid' => sub {
>-    plan tests => 7;
>-
>-    my $library = $builder->build_object( { class => 'Koha::Libraries' } );
>-    my $patron_category = $builder->build_object(
>-        {
>-            class => 'Koha::Patron::Categories',
>-            value => { category_type => 'P', enrolmentfee => 0 }
>-        }
>-    );
>-    my %data = (
>-        cardnumber   => "123456789",
>-        firstname    => "TÃ´mÃ sÃtÃ³",
>-        surname      => "ÃonÃ©",
>-        categorycode => $patron_category->categorycode,
>-        branchcode   => $library->branchcode,
>-    );
>-
>-    my $expected_userid_patron_1 = 'tomasito.none';
>-    my $new_patron = Koha::Patron->new({ firstname => $data{firstname}, surname => $data{surname} } );
>-    $new_patron->generate_userid;
>-    my $userid = $new_patron->userid;
>-    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
>-    my $borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
>-    my $patron_1 = Koha::Patrons->find($borrowernumber);
>-    is ( $patron_1->userid, $expected_userid_patron_1, 'The userid generated should be the one we expect' );
>-
>-    $new_patron->generate_userid;
>-    $userid = $new_patron->userid;
>-    is( $userid, $expected_userid_patron_1 . '1', 'generate_userid should generate the userid we expect' );
>-    $data{cardnumber} = '987654321';
>-    my $new_borrowernumber = Koha::Patron->new(\%data)->store->borrowernumber;
>-    my $patron_2 = Koha::Patrons->find($new_borrowernumber);
>-    isnt( $patron_2->userid, 'tomasito',
>-        "Patron with duplicate userid has new userid generated" );
>-    is( $patron_2->userid, $expected_userid_patron_1 . '1', # TODO we could make that configurable
>-        "Patron with duplicate userid has new userid generated (1 is appened" );
>-
>-    $new_patron->generate_userid;
>-    $userid = $new_patron->userid;
>-    is( $userid, $expected_userid_patron_1 . '2', 'generate_userid should generate the userid we expect' );
>-
>-    $patron_1 = Koha::Patrons->find($borrowernumber);
>-    $patron_1->userid(undef);
>-    $patron_1->generate_userid;
>-    $userid = $patron_1->userid;
>-    is( $userid, $expected_userid_patron_1, 'generate_userid should generate the userid we expect' );
>-
>-    # Cleanup
>-    $patron_1->delete;
>-    $patron_2->delete;
>-};
>-
> $nb_of_patrons = Koha::Patrons->search->count;
> $retrieved_patron_1->delete;
> is( Koha::Patrons->search->count, $nb_of_patrons - 1, 'Delete should have deleted the patron' );
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 32426: 146947 | 146948 | 146949 | 146950 | 146951 | 146952 | 146953 | 146954 | 147252 | 147253 | 147254 | 147255 | 147704 | 147705 | 147706 | 147707 | 147708 | 147709 | 147710 | 147711 | 147712 | 148203 | 148204 | 148205 | 148206 | 148207 | 148208 | 148209 | 148210 | 148211 | 148782 | 148793