As suggested by Frido in some other context, I file this bug to introduce the mentioned sub. I resurrected the idea after tracking down some warnings in SIP, and identifying bug 36575 introduced them by calling `->find` without passing a primary key. This kind of errors/warns: ``` [2025/06/30 13:03:23] [24253] [ERROR] [undef]@[undef]: DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): Unable to satisfy requested constraint 'primary', missing values for column(s): 'borrowernumber' at /kohadevbox/koha/Koha/Objects.pm line 98 C4::SIP::Trapper::__ANON__ /kohadevbox/koha/C4/SIP/Trapper.pm (70) ``` Instead of solving it locally, I propose to introduce a sane and tested method, and reuse it anywhere this pattern is being used.
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.