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.
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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Bug number in 5th patch corrected.
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>
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>
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>
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>
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>
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>
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>
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>
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>
(In reply to Kyle M Hall from comment #23) > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Great, Kyle. Thx
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>
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>
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>
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>
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>
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>
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>
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>
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>
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
This is great, added to : https://wiki.koha-community.org/wiki/Koha_Plugin_Hooks#Under_development_hooks
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?
(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.
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.
Pushed to master for 23.05. Nice work everyone, thanks!
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)
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>
API patch forthcoming
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>
Tomas: Should be okay now.
(In reply to Marcel de Rooy from comment #44) > Tomas: Should be okay now. Thanks for taking care, Marcel. Good job!
Enhancement - not backporting to 22.11.x Nice work everyone!
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.