Summary: | Add Koha::Patrons->find_by_identifier() | ||
---|---|---|---|
Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Component: | Architecture, internals, and plumbing | Assignee: | Tomás Cohen Arazi (tcohen) <tomascohen> |
Status: | Signed Off --- | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | david, fridolin.somers, kyle, martin.renvoize, nick, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 36575 | ||
Bug Blocks: | 40286 | ||
Attachments: |
Bug 40275: Unit tests
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Bug 40275: Unit tests Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase |
Description
Tomás Cohen Arazi (tcohen)
2025-06-30 13:46:04 UTC
Created attachment 183647 [details] [review] Bug 40275: Unit tests Created attachment 183648 [details] [review] Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` In our codebase, there's a pattern we tend to use for searching patrons by userid and falling back to find them by cardnumber: * C4::Auth * C4::SIP::ILS::Patron * (slightly different) Koha::REST::V1::Auth::Password * (slightly different) Koha::REST::V1::Auth * C4::Circulation * C4::Auth_with_cas It generally uses the following pattern: ```perl my $patron = Koha::Patrons->find( { userid => $identifier } ); $patron //= Koha::Patrons->find( { cardnumber => $identifier } ); ``` The problem with this is that `find` actually produces a DBIX::Class warn because `find` is being called without primary key parameters. I haven't seen it in the wild, until the latest change in `checkpw_internal` which made it throw warnings in SIP. My interpretation is that SIP's special approach with the Trapper.pm class made it show where other places just get it hidden. That said, I implemented this using `search()` to overcome this. To test: 1. Apply the patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: Tests pass! 3. Sign off :-D Created attachment 183649 [details] [review] Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase # Testing notes All seems to work. I ran the following related tests successfuly: ``` prove -r t/db_dependent/SIP \ t/db_dependent/Circ* \ t/db_dependent/Auth* ``` # Caveats As I understand it, `checkpw_internal()` should be adapted to use this as well. But it implies a behavior change and I need more feedback in that. The method seems to validate password for a patron found by `userid` and if that validation fails, it tries with the `cardnumber`. I understand we don't allow a cardnumber to match an existing userid, and the other way around. But I want to hear from other devs about it. If that was the case, we could just use `find_by_identifier()` and remove one of the blocks. I think this makes sense, and I see the problem I introduced in bug 36575 The limiting of patrons to sharing cardnumber/userid hasn't been finished (bug 14323 and bug 33905) so we should not let those block this cleanup (In reply to Tomás Cohen Arazi (tcohen) from comment #4) > # Testing notes > > All seems to work. I ran the following related tests successfuly: > > ``` > prove -r t/db_dependent/SIP \ > t/db_dependent/Circ* \ > t/db_dependent/Auth* > ``` > > # Caveats > > As I understand it, `checkpw_internal()` should be adapted to use this as > well. But it implies a behavior change and I need more feedback in that. > > The method seems to validate password for a patron found by `userid` and if > that validation fails, it tries with the `cardnumber`. I understand we don't > allow a cardnumber to match an existing userid, and the other way around. > But I want to hear from other devs about it. If that was the case, we could > just use `find_by_identifier()` and remove one of the blocks. Created attachment 183659 [details] [review] Bug 40275: Unit tests Signed-off-by: David Nind <david@davidnind.com> Created attachment 183660 [details] [review] Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` In our codebase, there's a pattern we tend to use for searching patrons by userid and falling back to find them by cardnumber: * C4::Auth * C4::SIP::ILS::Patron * (slightly different) Koha::REST::V1::Auth::Password * (slightly different) Koha::REST::V1::Auth * C4::Circulation * C4::Auth_with_cas It generally uses the following pattern: ```perl my $patron = Koha::Patrons->find( { userid => $identifier } ); $patron //= Koha::Patrons->find( { cardnumber => $identifier } ); ``` The problem with this is that `find` actually produces a DBIX::Class warn because `find` is being called without primary key parameters. I haven't seen it in the wild, until the latest change in `checkpw_internal` which made it throw warnings in SIP. My interpretation is that SIP's special approach with the Trapper.pm class made it show where other places just get it hidden. That said, I implemented this using `search()` to overcome this. To test: 1. Apply the patches 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Created attachment 183661 [details] [review] Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD): 1. Tests pass before and after the patches. I wanted to mention I didn't use the new method in `C4::Auth::checkpw_internal` (yet) because the tests explicitly check the use case of matching cardnumber-userid between different users, which we really want to forbid (see bug 33905). I think making `checkpw_internal` use this method should depend on what we do on that bug report, and as a follow-up. This report stands on its own. |