Bug 40275

Summary: Add Koha::Patrons->find_by_identifier()
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: 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
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.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:08 UTC
Created attachment 183647 [details] [review]
Bug 40275: Unit tests
Comment 2 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:11 UTC
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
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:14 UTC
Created attachment 183649 [details] [review]
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase
Comment 4 Tomás Cohen Arazi (tcohen) 2025-06-30 18:47:10 UTC
# 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.
Comment 5 Nick Clemens (kidclamp) 2025-06-30 18:59:37 UTC
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.
Comment 6 David Nind 2025-06-30 21:41:56 UTC
Created attachment 183659 [details] [review]
Bug 40275: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2025-06-30 21:41:59 UTC
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>
Comment 8 David Nind 2025-06-30 21:42:02 UTC
Created attachment 183661 [details] [review]
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2025-06-30 21:42:31 UTC
Testing notes (using KTD):
1. Tests pass before and after the patches.
Comment 10 Tomás Cohen Arazi (tcohen) 2025-07-01 14:55:20 UTC
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.