[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Nick Clemens (kidclamp) 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind changed: What|Removed |Added Status|Needs documenting |RESOLVED Resolution|--- |FIXED --- Comment #31 from David Nind --- Architecture related, no changes to the manual required. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Fridolin Somers changed: What|Removed |Added Status|Pushed to stable|Needs documenting --- Comment #30 from Fridolin Somers --- Enhancement not pushed to 24.11.x -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #29 from Paul Derscheid --- Nice work everyone! Pushed to 25.05.x -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Paul Derscheid 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #28 from Lucas Gass (lukeg) --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Lucas Gass (lukeg) 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) changed: What|Removed |Added QA Contact|[email protected] |[email protected] ||o.uk Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) changed: What|Removed |Added Attachment #183763|0 |1 is obsolete|| --- Comment #27 from Martin Renvoize (ashimema) --- 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 Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
Martin Renvoize (ashimema) changed:
What|Removed |Added
Attachment #183762|0 |1
is obsolete||
--- Comment #26 from Martin Renvoize (ashimema)
---
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
Signed-off-by: Kyle M Hall
Signed-off-by: Martin Renvoize
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Martin Renvoize (ashimema) changed: What|Removed |Added Attachment #183761|0 |1 is obsolete|| --- Comment #25 from Martin Renvoize (ashimema) --- Created attachment 185914 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185914&action=edit Bug 40275: Unit tests Signed-off-by: David Nind Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #24 from David Cook --- (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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) changed: What|Removed |Added Status|In Discussion |Signed Off --- Comment #23 from Tomás Cohen Arazi (tcohen) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) changed: What|Removed |Added Attachment #183765|0 |1 is obsolete|| -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #22 from Aleisha Amohia --- (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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Jonathan Druart changed: What|Removed |Added CC||[email protected] -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #21 from David Cook --- (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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
David Cook changed:
What|Removed |Added
Status|Signed Off |In Discussion
--- Comment #20 from David Cook ---
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.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
--- Comment #19 from David Cook ---
(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.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) changed: What|Removed |Added Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) changed: What|Removed |Added Attachment #183764|0 |1 is obsolete|| --- Comment #18 from Kyle M Hall (khall) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #17 from Kyle M Hall (khall) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) changed: What|Removed |Added Attachment #183661|0 |1 is obsolete|| --- Comment #16 from Kyle M Hall (khall) --- 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 Signed-off-by: Kyle M Hall -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
Kyle M Hall (khall) changed:
What|Removed |Added
Attachment #183660|0 |1
is obsolete||
--- Comment #15 from Kyle M Hall (khall) ---
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
Signed-off-by: Kyle M Hall
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Kyle M Hall (khall) changed: What|Removed |Added Attachment #183659|0 |1 is obsolete|| --- Comment #14 from Kyle M Hall (khall) --- Created attachment 183761 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183761&action=edit Bug 40275: Unit tests Signed-off-by: David Nind Signed-off-by: Kyle M Hall -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
--- Comment #13 from Aleisha Amohia ---
(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.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Cook changed: What|Removed |Added CC||[email protected] --- Comment #12 from David Cook --- (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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Aleisha Amohia changed: What|Removed |Added QA Contact|[email protected] |[email protected] |y.org | -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Aleisha Amohia changed: What|Removed |Added CC||[email protected] Status|Signed Off |Failed QA --- Comment #11 from Aleisha Amohia --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) changed: What|Removed |Added CC||[email protected], ||[email protected] ||o.uk --- Comment #10 from Tomás Cohen Arazi (tcohen) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind changed: What|Removed |Added CC||[email protected] --- Comment #9 from David Nind --- Testing notes (using KTD): 1. Tests pass before and after the patches. -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind changed: What|Removed |Added Attachment #183649|0 |1 is obsolete|| --- Comment #8 from David Nind --- 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 -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
David Nind changed:
What|Removed |Added
Attachment #183648|0 |1
is obsolete||
--- Comment #7 from David Nind ---
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
--
You are receiving this mail because:
You are watching all bug changes.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind changed: What|Removed |Added Attachment #183647|0 |1 is obsolete|| --- Comment #6 from David Nind --- Created attachment 183659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=183659&action=edit Bug 40275: Unit tests Signed-off-by: David Nind -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 David Nind changed: What|Removed |Added Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Nick Clemens (kidclamp) changed: What|Removed |Added CC||[email protected] --- Comment #5 from Nick Clemens (kidclamp) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #4 from Tomás Cohen Arazi (tcohen) --- # 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #3 from Tomás Cohen Arazi (tcohen) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275
--- Comment #2 from Tomás Cohen Arazi (tcohen) ---
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.
___
Koha-bugs mailing list
[email protected]
https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 --- Comment #1 from Tomás Cohen Arazi (tcohen) --- 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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) changed: What|Removed |Added Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
[Koha-bugs] [Bug 40275] Add Koha::Patrons->find_by_identifier()
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 Tomás Cohen Arazi (tcohen) changed: What|Removed |Added CC||[email protected] ||m, [email protected] Assignee|[email protected] |[email protected] |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. ___ Koha-bugs mailing list [email protected] https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
