[Bug 40275] New: Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Bug ID: 40275 Summary: Add Koha::Patrons->find_by_identifier() Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m, tomascohen@gmail.com Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Depends on| |36575 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36575 [Bug 36575] Wrong patron can be returned for API validation route -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 183647 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183647&action=edit Bug 40275: Unit tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 183648 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183648&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #3 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 183649 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183649&action=edit Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- # 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com --- Comment #5 from Nick Clemens (kidclamp) <nick@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183647|0 |1 is obsolete| | --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 183659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183659&action=edit Bug 40275: Unit tests Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183648|0 |1 is obsolete| | --- Comment #7 from David Nind <david@davidnind.com> --- Created attachment 183660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183660&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183649|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 183661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183661&action=edit Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #9 from David Nind <david@davidnind.com> --- Testing notes (using KTD): 1. Tests pass before and after the patches. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@openfifth.c | |o.uk --- Comment #10 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40286 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 [Bug 40286] Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aleisha@catalyst.net.nz Status|Signed Off |Failed QA --- Comment #11 from Aleisha Amohia <aleisha@catalyst.net.nz> --- This is a great fix. There's still quite a few more places in the codebase that use Koha::Patrons->find and pass in the userid or cardnumber. I started listing them all and then there were heaps and heaps ... and from what you've said here, they'll all cause those errors/warns. I feel like they should be addressed within this patchset even though I realise that creates a lot more work - otherwise it ends up being one of those situations where we have the old bad code in place at the same time as the new good code. I'll set this to Failed QA because of the other instances where I think this needs to be implemented all at once, but happy to hear your thoughts. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Aleisha Amohia <aleisha@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |aleisha@catalyst.net.nz |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #12 from David Cook <dcook@prosentient.com.au> --- (In reply to Aleisha Amohia from comment #11)
This is a great fix. There's still quite a few more places in the codebase that use Koha::Patrons->find and pass in the userid or cardnumber. I started listing them all and then there were heaps and heaps ... and from what you've said here, they'll all cause those errors/warns. I feel like they should be addressed within this patchset even though I realise that creates a lot more work - otherwise it ends up being one of those situations where we have the old bad code in place at the same time as the new good code.
I'll set this to Failed QA because of the other instances where I think this needs to be implemented all at once, but happy to hear your thoughts.
Do you have some examples of some other places? There are a number of places where I think we should actually be moving to using borrowernumber instead of cardnumber/userid, so some places might need that fix instead of this one? (For example bug 34018 or bug 39535) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #13 from Aleisha Amohia <aleisha@catalyst.net.nz> --- (In reply to David Cook from comment #12)
Do you have some examples of some other places?
There are a number of places where I think we should actually be moving to using borrowernumber instead of cardnumber/userid, so some places might need that fix instead of this one? (For example bug 34018 or bug 39535)
I started making that list, and then stopped because I had the same thought about using borrowernumber instead, and being out of scope. But as an example, there is this instance in C4::Auth->check_cookie_auth() that I'm not sure about? 1943 } elsif ($userid) { 1944 $session->param( 'lasttime', time() ); 1945 my $patron = Koha::Patrons->find( { userid => $userid } ); 1946 1947 # If the user modify their own userid 1948 # Better than 500 but we could do better 1949 unless ($patron) { 1950 $session->delete(); 1951 $session->flush; 1952 return ( "expired", undef ); 1953 } 1954 1955 $patron = Koha::Patrons->find( { cardnumber => $userid } ) 1956 unless $patron; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183659|0 |1 is obsolete| | --- Comment #14 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 183761 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183761&action=edit Bug 40275: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183660|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 183762 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183762&action=edit 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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183661|0 |1 is obsolete| | --- Comment #16 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 183763 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183763&action=edit Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #17 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 183764 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183764&action=edit Bug 40275: Implement find_by_identifier for check_cookie_auth -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183764|0 |1 is obsolete| | --- Comment #18 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 183765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183765&action=edit Bug 40275: Implement find_by_identifier for check_cookie_auth Test Plan: 1) prove t/db_dependent/Auth.t -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #19 from David Cook <dcook@prosentient.com.au> --- (In reply to Aleisha Amohia from comment #13)
(In reply to David Cook from comment #12)
Do you have some examples of some other places?
There are a number of places where I think we should actually be moving to using borrowernumber instead of cardnumber/userid, so some places might need that fix instead of this one? (For example bug 34018 or bug 39535)
I started making that list, and then stopped because I had the same thought about using borrowernumber instead, and being out of scope.
But as an example, there is this instance in C4::Auth->check_cookie_auth() that I'm not sure about?
1943 } elsif ($userid) { 1944 $session->param( 'lasttime', time() ); 1945 my $patron = Koha::Patrons->find( { userid => $userid } ); 1946 1947 # If the user modify their own userid 1948 # Better than 500 but we could do better 1949 unless ($patron) { 1950 $session->delete(); 1951 $session->flush; 1952 return ( "expired", undef ); 1953 } 1954 1955 $patron = Koha::Patrons->find( { cardnumber => $userid } ) 1956 unless $patron;
Yeah, I'm pretty sure this is one of those places we should be using borrowernumber (ie $session->param('number') rather than the userid in $session->param('id')) instead. The current lookup by userid and cardnumber there is... not good. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |In Discussion --- Comment #20 from David Cook <dcook@prosentient.com.au> --- Ohhh I just saw that Kyle added the patch for Koha::Patrons->find_by_identifier to check_cookie_auth in response to Aleisha's comment. Yeah, rather than doing that let's do something more like this: my $patron = Koha::Patrons->find({ borrowernumber => $session->param('number') }); That's going to be way better. -- I'm just going to quickly pop this to "In Discussion". Do we want to drop the patch "Implement find_by_identifier for check_cookie_auth" and handle the move to borrowernumber on a separate bug? Alternatively, we leave the patch in, but knowing that it really should be replaced with a borrowernumber lookup? I think Aleisha raises a really interesting point here. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #20)
I'm just going to quickly pop this to "In Discussion".
Do we want to drop the patch "Implement find_by_identifier for check_cookie_auth" and handle the move to borrowernumber on a separate bug?
Alternatively, we leave the patch in, but knowing that it really should be replaced with a borrowernumber lookup?
I think Aleisha raises a really interesting point here.
FYI I'm just looking for something like "Yeah, let's handle it on a separate bug", or "No, maybe we should handle it here". Not trying to derail the progress here. Might be worthwhile just opening an Omnibus bug for the "Use borrowernumber instead of userid/cardnumber where possible", and then we raise a child report for "check_cookie_auth" (and all the other bugs I've reported ont his topic). I think I'm probably inclined to go that direction myself, but curious what other people think. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #22 from Aleisha Amohia <aleisha@catalyst.net.nz> --- (In reply to David Cook from comment #21)
FYI I'm just looking for something like "Yeah, let's handle it on a separate bug", or "No, maybe we should handle it here". Not trying to derail the progress here.
Might be worthwhile just opening an Omnibus bug for the "Use borrowernumber instead of userid/cardnumber where possible", and then we raise a child report for "check_cookie_auth" (and all the other bugs I've reported ont his topic). I think I'm probably inclined to go that direction myself, but curious what other people think.
I agree with this - I don't want to block progress here but would like to see intention for the other cases to get handled somehow -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183765|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off --- Comment #23 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Hi all. This patch set addresses a pattern that we use in the codebase and creates a fully tested method for reusing it. As with many places in Koha, old code that's been patched for years hides bugs, and small refactoring intentions end up being a pain because 'why not fix it all'. I agree and have always tried to fix all the things, but let's make it a separate bug report. I would like to work on that, but it is better to have a well summarized description of the problem to be solved. And some places are so tricky we should probably have an individual bug for them. In the case of `check_cookie_auth`, I'd say that fallback mechanism is hiding another bug somewhere else. Because it was probably added as a workaround. As we should really just have the borrowernumber there... so a longer conversation that should happen in a separate report. Thanks Kyle anyway! Setting this back to SO so we can move forward. Can someone file a follow-up bug in which we can discuss the other issues? Happy to work on them! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #24 from David Cook <dcook@prosentient.com.au> --- (In reply to Tomás Cohen Arazi (tcohen) from comment #23)
This patch set addresses a pattern that we use in the codebase and creates a fully tested method for reusing it.
Definitely a good move forward.
As with many places in Koha, old code that's been patched for years hides bugs, and small refactoring intentions end up being a pain because 'why not fix it all'.
I agree and have always tried to fix all the things, but let's make it a separate bug report. I would like to work on that, but it is better to have a well summarized description of the problem to be solved. And some places are so tricky we should probably have an individual bug for them.
Agreed about making separate bug reports. Trying to fix all the things at once rarely works. Just want to make sure all the key people know there are other bug reports to consider.
In the case of `check_cookie_auth`, I'd say that fallback mechanism is hiding another bug somewhere else. Because it was probably added as a workaround. As we should really just have the borrowernumber there... so a longer conversation that should happen in a separate report. Thanks Kyle anyway!
+1
Setting this back to SO so we can move forward.
Can someone file a follow-up bug in which we can discuss the other issues? Happy to work on them!
I've raised bug 40457 and I'll try to attach all related reports I know about -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=40457 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |23771 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23771 [Bug 23771] CAS/Shib Authentication can fail when multiple users with no userid/cardnumber defined and one of them is locked -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |40463 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40463 [Bug 40463] Use Koha::Patrons->find_by_identifier() in C4/Auth_with_ldap.pm -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183761|0 |1 is obsolete| | --- Comment #25 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 185914 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185914&action=edit Bug 40275: Unit tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183762|0 |1 is obsolete| | --- Comment #26 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 185915 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185915&action=edit 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> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #183763|0 |1 is obsolete| | --- Comment #27 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 185916 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185916&action=edit Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|aleisha@catalyst.net.nz |martin.renvoize@openfifth.c | |o.uk Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #28 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Paul Derscheid <paul.derscheid@lmscloud.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|25.11.00 |25.11.00,25.05.04 released in| | Status|Pushed to main |Pushed to stable -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #29 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to stable |Needs documenting --- Comment #30 from Fridolin Somers <fridolin.somers@biblibre.com> --- Enhancement not pushed to 24.11.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #31 from David Nind <david@davidnind.com> --- Architecture related, no changes to the manual required. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Nick Clemens (kidclamp) <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=42043 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org