Bug 32426: Patron::generate_userid - add plugin hook
authorMarcel de Rooy <m.de.rooy@rijksmuseum.nl>
Mon, 5 Dec 2022 14:17:37 +0000 (15:17 +0100)
committerTomas Cohen Arazi <tomascohen@theke.io>
Mon, 27 Mar 2023 10:49:50 +0000 (12:49 +0200)
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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Koha/Patron.pm
t/db_dependent/Koha/Patron_generate_userid.t [new file with mode: 0755]
t/db_dependent/Koha/Patrons.t

index d3ec915..48aec43 100644 (file)
@@ -1791,16 +1791,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 (executable)
index 0000000..d44753d
--- /dev/null
@@ -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;
index 12f00d0..585d3e2 100755 (executable)
@@ -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' );