Bug 40275 - Add Koha::Patrons->find_by_identifier()
Summary: Add Koha::Patrons->find_by_identifier()
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 36575
Blocks:
  Show dependency treegraph
 
Reported: 2025-06-30 13:46 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-06-30 21:42 UTC (History)
4 users (show)

See Also:
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:


Attachments
Bug 40275: Unit tests (4.86 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` (2.41 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase (5.08 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Unit tests (4.91 KB, patch)
2025-06-30 21:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` (2.46 KB, patch)
2025-06-30 21:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase (5.11 KB, patch)
2025-06-30 21:42 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.