Bug 32426 - Make userid generation pluggable
Summary: Make userid generation pluggable
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 36170
  Show dependency treegraph
 
Reported: 2022-12-08 08:16 UTC by Marcel de Rooy
Modified: 2024-02-27 08:51 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This adds a new plugin hook `patron_generate_userid` that allows to have a custom method for generating the userid on a patron record.
Version(s) released in:
23.05.00


Attachments
Bug 32426: Patron::generate_userid - add plugin hook (12.38 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Throw InvalidUserid exception in Patron->store (4.87 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Changes for opac-memberentry (4.71 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Changes for opac-registration-verify (5.52 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32436: Changes for members/memberentry.pl (7.52 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Adjust error message of ERROR_login_exist (1.80 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Fix qatools issues (4.06 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: [DO NOT PUSH] Introduce example plugin (3.01 KB, patch)
2023-02-20 11:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Changes for members/memberentry.pl (7.52 KB, patch)
2023-02-23 15:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Adjust error message of ERROR_login_exist (1.80 KB, patch)
2023-02-23 15:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Fix qatools issues (4.06 KB, patch)
2023-02-23 15:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: [DO NOT PUSH] Introduce example plugin (3.01 KB, patch)
2023-02-23 15:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: Patron::generate_userid - add plugin hook (12.40 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Throw InvalidUserid exception in Patron->store (4.88 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Changes for opac-memberentry (4.72 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Changes for opac-registration-verify (5.54 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Changes for members/memberentry.pl (7.53 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Adjust error message of ERROR_login_exist (1.82 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Fix qatools issues (4.07 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: (follow-up) Use more acurate term 'internal' instead of 'legacy' (2.44 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: [DO NOT PUSH] Introduce example plugin (3.03 KB, patch)
2023-03-03 16:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32426: Patron::generate_userid - add plugin hook (12.46 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Throw InvalidUserid exception in Patron->store (4.94 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Changes for opac-memberentry (4.79 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Changes for opac-registration-verify (5.60 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Changes for members/memberentry.pl (7.60 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Adjust error message of ERROR_login_exist (1.88 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: Fix qatools issues (4.14 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: (follow-up) Use more acurate term 'internal' instead of 'legacy' (2.51 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: [DO NOT PUSH] Introduce example plugin (3.09 KB, patch)
2023-03-15 13:43 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32426: (follow-up) Fix for Members.t / Patrons.t (3.40 KB, patch)
2023-03-27 14:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32426: (follow-up) Fix api/v1/patrons.t (4.44 KB, patch)
2023-03-28 06:44 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 Marcel de Rooy 2022-12-08 08:16:03 UTC
See also bug 29480 and its old predecessor 2237 for an email based userid.
I will give a try here in doing this via Koha plugins and hence providing more flexibility.
Comment 1 Marcel de Rooy 2023-01-23 13:17:19 UTC
FAKE_PATRON CODE (memberentry.pl)
    my $fake_patron = Koha::Patron->new;
    $fake_patron->userid($patron->userid) if $patron; # editing
    if ( ( defined $newdata{'firstname'} || $category_type eq 'I' ) && ( defined $newdata{'surname'} ) ) {
        # Full page edit, firstname and surname input zones are present
        $fake_patron->firstname($newdata{firstname});
        $fake_patron->surname($newdata{surname});
        $fake_patron->generate_userid;
        $newdata{'userid'} = $fake_patron->userid;
=> We should remove this code and leave userid to patron->store now imo.
Comment 2 Marcel de Rooy 2023-02-20 11:50:25 UTC
Created attachment 146947 [details] [review]
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>
Comment 3 Marcel de Rooy 2023-02-20 11:50:27 UTC
Created attachment 146948 [details] [review]
Bug 32426: Throw InvalidUserid exception in Patron->store

When creating we still call generate_userid and verify the result.
When modifying we do not accept an invalid userid. When needed,
we recreate the userid; this should be very exceptional.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Go to staff interface. Try changing userid of a patron to an
existing one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2023-02-20 11:50:29 UTC
Created attachment 146949 [details] [review]
Bug 32426: Changes for opac-memberentry

Test plan:
Enable self registration, pick a default category too for it.
The changes will be tested later with a plugin, but now change
Koha/Patron.pm as follows:
 sub _generate_userid_legacy { # as we always did
     my ($self) = @_;
+return $self;
So, add the return $self line only.
Restart all. Try to register an account on OPAC.
You should see an alert about problems processing your registration.
Undo the change in Patron.pm and restart all.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-02-20 11:50:32 UTC
Created attachment 146950 [details] [review]
Bug 32426: Changes for opac-registration-verify

Similar to changes in opac-memberentry.

Test plan:
Now also enable  PatronSelfRegistrationVerifyByEmail.
Make the same change in Patron again with return $self.
Restart all. Self register. Check your email.
Follow the link. Verify that you have a similar alert.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2023-02-20 11:50:34 UTC
Created attachment 146951 [details] [review]
Bug 32436: Changes for members/memberentry.pl

Test plan:
Note: We will address this again when installing a plugin, but
first we test with the legacy userid code.

Add a new user with members/memberentry in staff.
Edit this user, change userid in staff. Try full form and partial
one.
If you remove userid or replace by a space (when mandatory), Koha
should regenerate a legacy userid.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2023-02-20 11:50:36 UTC
Created attachment 146952 [details] [review]
Bug 32426: Adjust error message of ERROR_login_exist

The reference to password might be confusing. You could have the
same password as another user.
The addition of 'could not create unique one' especially applies to
plugins that return an existing userid, like the very simple plugin
example on the Bugzilla report that just returns borrowers.email.

Test plan:
Covered by previous patch. Just a string change here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-02-20 11:50:39 UTC
Created attachment 146953 [details] [review]
Bug 32426: Fix qatools issues

[1] opac-registration-verify.pl forbidden pattern: Script permissions is authnotrequired => 1, it is correct for an OPAC script, not intranet (bug 24663)
    => Do not go back to authrequired => 1. Use a check on OpacPublic.
[2] opac-registration-invalid.tt missing_filter at line 41 (<p>Error [% error_type %]: [% error_info %]</p>)
[3] opac-memberentry.tt missing_filter at line 131 (<p>Error [% error_type %]: [% error_info %]</p>)

Test plan:
Verify that qatools passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-02-20 11:50:41 UTC
Created attachment 146954 [details] [review]
Bug 32426: [DO NOT PUSH] Introduce example plugin

RM: Do not push this patch!

A very simple plugin; it just returns the borrowers.email field, assuming
that you made this field mandatory.
It should check uniqueness, provide an alternative; for simplicity
that is not included here.

Test plan:
NOTE: Some unit tests may fail when applying this patch/installing this
plugin. That's no problem; this example plugin just serves as POC.

Install the plugin. You could just run:
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->install"

Run t/db_dependent/Koha/Patron_generate_userid.t. This should pass.

Enter a new patron via OPAC self registration; try existing and new email address.
Toggle PatronSelfRegistrationVerifyByEmail. Try again (existing, new).
Go to staff memberentry page.
Add new patron with new email address. (Leave userid empty.)
Verify the newly generated userid after saving.
Try to add another patron with the same email. You should see an alert.
Edit an existing patron. Try new and existing email address.
When editing, clear userid and save. Check result.

Uninstall the plugin.
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->uninstall"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2023-02-23 15:18:10 UTC
Created attachment 147252 [details] [review]
Bug 32426: Changes for members/memberentry.pl

Test plan:
Note: We will address this again when installing a plugin, but
first we test with the legacy userid code.

Add a new user with members/memberentry in staff.
Edit this user, change userid in staff. Try full form and partial
one.
If you remove userid or replace by a space (when mandatory), Koha
should regenerate a legacy userid.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2023-02-23 15:18:12 UTC
Created attachment 147253 [details] [review]
Bug 32426: Adjust error message of ERROR_login_exist

The reference to password might be confusing. You could have the
same password as another user.
The addition of 'could not create unique one' especially applies to
plugins that return an existing userid, like the very simple plugin
example on the Bugzilla report that just returns borrowers.email.

Test plan:
Covered by previous patch. Just a string change here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2023-02-23 15:18:14 UTC
Created attachment 147254 [details] [review]
Bug 32426: Fix qatools issues

[1] opac-registration-verify.pl forbidden pattern: Script permissions is authnotrequired => 1, it is correct for an OPAC script, not intranet (bug 24663)
    => Do not go back to authrequired => 1. Use a check on OpacPublic.
[2] opac-registration-invalid.tt missing_filter at line 41 (<p>Error [% error_type %]: [% error_info %]</p>)
[3] opac-memberentry.tt missing_filter at line 131 (<p>Error [% error_type %]: [% error_info %]</p>)

Test plan:
Verify that qatools passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2023-02-23 15:18:16 UTC
Created attachment 147255 [details] [review]
Bug 32426: [DO NOT PUSH] Introduce example plugin

RM: Do not push this patch!

A very simple plugin; it just returns the borrowers.email field, assuming
that you made this field mandatory.
It should check uniqueness, provide an alternative; for simplicity
that is not included here.

Test plan:
NOTE: Some unit tests may fail when applying this patch/installing this
plugin. That's no problem; this example plugin just serves as POC.

Install the plugin. You could just run:
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->install"

Run t/db_dependent/Koha/Patron_generate_userid.t. This should pass.

Enter a new patron via OPAC self registration; try existing and new email address.
Toggle PatronSelfRegistrationVerifyByEmail. Try again (existing, new).
Go to staff memberentry page.
Add new patron with new email address. (Leave userid empty.)
Verify the newly generated userid after saving.
Try to add another patron with the same email. You should see an alert.
Edit an existing patron. Try new and existing email address.
When editing, clear userid and save. Check result.

Uninstall the plugin.
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->uninstall"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2023-02-23 15:18:40 UTC
Bug number in 5th patch corrected.
Comment 15 Kyle M Hall 2023-03-03 16:08:35 UTC
Created attachment 147704 [details] [review]
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>
Comment 16 Kyle M Hall 2023-03-03 16:08:42 UTC
Created attachment 147705 [details] [review]
Bug 32426: Throw InvalidUserid exception in Patron->store

When creating we still call generate_userid and verify the result.
When modifying we do not accept an invalid userid. When needed,
we recreate the userid; this should be very exceptional.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Go to staff interface. Try changing userid of a patron to an
existing one.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2023-03-03 16:08:44 UTC
Created attachment 147706 [details] [review]
Bug 32426: Changes for opac-memberentry

Test plan:
Enable self registration, pick a default category too for it.
The changes will be tested later with a plugin, but now change
Koha/Patron.pm as follows:
 sub _generate_userid_legacy { # as we always did
     my ($self) = @_;
+return $self;
So, add the return $self line only.
Restart all. Try to register an account on OPAC.
You should see an alert about problems processing your registration.
Undo the change in Patron.pm and restart all.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2023-03-03 16:08:46 UTC
Created attachment 147707 [details] [review]
Bug 32426: Changes for opac-registration-verify

Similar to changes in opac-memberentry.

Test plan:
Now also enable  PatronSelfRegistrationVerifyByEmail.
Make the same change in Patron again with return $self.
Restart all. Self register. Check your email.
Follow the link. Verify that you have a similar alert.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall 2023-03-03 16:08:48 UTC
Created attachment 147708 [details] [review]
Bug 32426: Changes for members/memberentry.pl

Test plan:
Note: We will address this again when installing a plugin, but
first we test with the legacy userid code.

Add a new user with members/memberentry in staff.
Edit this user, change userid in staff. Try full form and partial
one.
If you remove userid or replace by a space (when mandatory), Koha
should regenerate a legacy userid.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2023-03-03 16:08:50 UTC
Created attachment 147709 [details] [review]
Bug 32426: Adjust error message of ERROR_login_exist

The reference to password might be confusing. You could have the
same password as another user.
The addition of 'could not create unique one' especially applies to
plugins that return an existing userid, like the very simple plugin
example on the Bugzilla report that just returns borrowers.email.

Test plan:
Covered by previous patch. Just a string change here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2023-03-03 16:08:52 UTC
Created attachment 147710 [details] [review]
Bug 32426: Fix qatools issues

[1] opac-registration-verify.pl forbidden pattern: Script permissions is authnotrequired => 1, it is correct for an OPAC script, not intranet (bug 24663)
    => Do not go back to authrequired => 1. Use a check on OpacPublic.
[2] opac-registration-invalid.tt missing_filter at line 41 (<p>Error [% error_type %]: [% error_info %]</p>)
[3] opac-memberentry.tt missing_filter at line 131 (<p>Error [% error_type %]: [% error_info %]</p>)

Test plan:
Verify that qatools passes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2023-03-03 16:08:53 UTC
Created attachment 147711 [details] [review]
Bug 32426: (follow-up) Use more acurate term 'internal' instead of 'legacy'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2023-03-03 16:08:56 UTC
Created attachment 147712 [details] [review]
Bug 32426: [DO NOT PUSH] Introduce example plugin

RM: Do not push this patch!

A very simple plugin; it just returns the borrowers.email field, assuming
that you made this field mandatory.
It should check uniqueness, provide an alternative; for simplicity
that is not included here.

Test plan:
NOTE: Some unit tests may fail when applying this patch/installing this
plugin. That's no problem; this example plugin just serves as POC.

Install the plugin. You could just run:
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->install"

Run t/db_dependent/Koha/Patron_generate_userid.t. This should pass.

Enter a new patron via OPAC self registration; try existing and new email address.
Toggle PatronSelfRegistrationVerifyByEmail. Try again (existing, new).
Go to staff memberentry page.
Add new patron with new email address. (Leave userid empty.)
Verify the newly generated userid after saving.
Try to add another patron with the same email. You should see an alert.
Edit an existing patron. Try new and existing email address.
When editing, clear userid and save. Check result.

Uninstall the plugin.
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->uninstall"

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Marcel de Rooy 2023-03-04 07:30:35 UTC
(In reply to Kyle M Hall from comment #23)
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Great, Kyle. Thx
Comment 25 Martin Renvoize 2023-03-15 13:43:10 UTC
Created attachment 148203 [details] [review]
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>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2023-03-15 13:43:13 UTC
Created attachment 148204 [details] [review]
Bug 32426: Throw InvalidUserid exception in Patron->store

When creating we still call generate_userid and verify the result.
When modifying we do not accept an invalid userid. When needed,
we recreate the userid; this should be very exceptional.

Test plan:
Run t/db_dependent/Koha/Patrons.t
Go to staff interface. Try changing userid of a patron to an
existing one.

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>
Comment 27 Martin Renvoize 2023-03-15 13:43:15 UTC
Created attachment 148205 [details] [review]
Bug 32426: Changes for opac-memberentry

Test plan:
Enable self registration, pick a default category too for it.
The changes will be tested later with a plugin, but now change
Koha/Patron.pm as follows:
 sub _generate_userid_legacy { # as we always did
     my ($self) = @_;
+return $self;
So, add the return $self line only.
Restart all. Try to register an account on OPAC.
You should see an alert about problems processing your registration.
Undo the change in Patron.pm and restart all.

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>
Comment 28 Martin Renvoize 2023-03-15 13:43:18 UTC
Created attachment 148206 [details] [review]
Bug 32426: Changes for opac-registration-verify

Similar to changes in opac-memberentry.

Test plan:
Now also enable  PatronSelfRegistrationVerifyByEmail.
Make the same change in Patron again with return $self.
Restart all. Self register. Check your email.
Follow the link. Verify that you have a similar alert.

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>
Comment 29 Martin Renvoize 2023-03-15 13:43:21 UTC
Created attachment 148207 [details] [review]
Bug 32426: Changes for members/memberentry.pl

Test plan:
Note: We will address this again when installing a plugin, but
first we test with the legacy userid code.

Add a new user with members/memberentry in staff.
Edit this user, change userid in staff. Try full form and partial
one.
If you remove userid or replace by a space (when mandatory), Koha
should regenerate a legacy userid.

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>
Comment 30 Martin Renvoize 2023-03-15 13:43:24 UTC
Created attachment 148208 [details] [review]
Bug 32426: Adjust error message of ERROR_login_exist

The reference to password might be confusing. You could have the
same password as another user.
The addition of 'could not create unique one' especially applies to
plugins that return an existing userid, like the very simple plugin
example on the Bugzilla report that just returns borrowers.email.

Test plan:
Covered by previous patch. Just a string change here.

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>
Comment 31 Martin Renvoize 2023-03-15 13:43:26 UTC
Created attachment 148209 [details] [review]
Bug 32426: Fix qatools issues

[1] opac-registration-verify.pl forbidden pattern: Script permissions is authnotrequired => 1, it is correct for an OPAC script, not intranet (bug 24663)
    => Do not go back to authrequired => 1. Use a check on OpacPublic.
[2] opac-registration-invalid.tt missing_filter at line 41 (<p>Error [% error_type %]: [% error_info %]</p>)
[3] opac-memberentry.tt missing_filter at line 131 (<p>Error [% error_type %]: [% error_info %]</p>)

Test plan:
Verify that qatools passes.

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>
Comment 32 Martin Renvoize 2023-03-15 13:43:29 UTC
Created attachment 148210 [details] [review]
Bug 32426: (follow-up) Use more acurate term 'internal' instead of 'legacy'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2023-03-15 13:43:31 UTC
Created attachment 148211 [details] [review]
Bug 32426: [DO NOT PUSH] Introduce example plugin

RM: Do not push this patch!

A very simple plugin; it just returns the borrowers.email field, assuming
that you made this field mandatory.
It should check uniqueness, provide an alternative; for simplicity
that is not included here.

Test plan:
NOTE: Some unit tests may fail when applying this patch/installing this
plugin. That's no problem; this example plugin just serves as POC.

Install the plugin. You could just run:
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->install"

Run t/db_dependent/Koha/Patron_generate_userid.t. This should pass.

Enter a new patron via OPAC self registration; try existing and new email address.
Toggle PatronSelfRegistrationVerifyByEmail. Try again (existing, new).
Go to staff memberentry page.
Add new patron with new email address. (Leave userid empty.)
Verify the newly generated userid after saving.
Try to add another patron with the same email. You should see an alert.
Edit an existing patron. Try new and existing email address.
When editing, clear userid and save. Check result.

Uninstall the plugin.
* perl -MKoha::Plugin::Example::Userid_email -e"Koha::Plugin::Example::Userid_email->new->uninstall"

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>
Comment 34 Martin Renvoize 2023-03-15 13:44:31 UTC
All works well and as designed.. no regressions found and unit tests all present and correct.

I'm also happy to see some area's cleaned up a little in this patchset.

Passing QA
Comment 35 Fridolin Somers 2023-03-18 02:18:14 UTC
This is great, added to :
https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks#Under_development_hooks
Comment 36 Tomás Cohen Arazi 2023-03-21 18:49:37 UTC
I like this. And have a question I've been munging about this kind of hooks for a while:

What if there are two plugins that implement the hook? Not asking for this bug to solve it but, should we have a way to define some precedence between them, and make it work in a way that is not just alphabetically?
Comment 37 Marcel de Rooy 2023-03-21 20:09:05 UTC
(In reply to Tomás Cohen Arazi from comment #36)
> I like this. And have a question I've been munging about this kind of hooks
> for a while:
> 
> What if there are two plugins that implement the hook? Not asking for this
> bug to solve it but, should we have a way to define some precedence between
> them, and make it work in a way that is not just alphabetically?

Yes, I thought about that too. I think that it is not even alphabetical but database order.
Bug 32449.
Comment 38 Martin Renvoize 2023-03-21 20:17:47 UTC
Marcel and I discussed exactly that on IRC when I was QAing this as it happens.

We could do with a few refinements around the plugin system as a whole. Setting a precedence order to apply transforms like this is exactly one of the cases we were thinking about.
Comment 39 Tomás Cohen Arazi 2023-03-27 10:47:29 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 40 Marcel de Rooy 2023-03-27 14:25:21 UTC
Tomas:
I will check those 3 failures now:

t_db_dependent_Members_t.Koha::Patron->store should not update userid if not true (78)
t_db_dependent_api_v1_patrons_t.add() tests (1689)
t_db_dependent_api_v1_patrons_t.update() tests (1689)
Comment 41 Marcel de Rooy 2023-03-27 14:36:37 UTC
Created attachment 148782 [details] [review]
Bug 32426: (follow-up) Fix for Members.t / Patrons.t

Moving and adjusting test in Member.t.

Test plan:
Run both tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2023-03-27 14:53:25 UTC
API patch forthcoming
Comment 43 Marcel de Rooy 2023-03-28 06:44:55 UTC
Created attachment 148793 [details] [review]
Bug 32426: (follow-up) Fix api/v1/patrons.t

We use another exception now.

Test plan:
Run t/db_dependent/api/v1/patrons.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2023-03-28 06:48:10 UTC
Tomas: Should be okay now.
Comment 45 Tomás Cohen Arazi 2023-03-28 12:54:42 UTC
(In reply to Marcel de Rooy from comment #44)
> Tomas: Should be okay now.

Thanks for taking care, Marcel. Good job!
Comment 46 Matt Blenkinsop 2023-04-11 10:40:35 UTC
Enhancement - not backporting to 22.11.x

Nice work everyone!
Comment 47 Caroline Cyr La Rose 2023-12-20 21:33:45 UTC
Plugin hooks are not documented in the manual. I see that this one is already mentioned here https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks
Unless there is somewhere else this should be documented, I think we can close it. Feel free to reopen with more details, if necessary.