Bug 20348 - SIP2 patron identification fails to use userid
Summary: SIP2 patron identification fails to use userid
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jose Martin
QA Contact: Josef Moravec
URL:
Keywords:
: 10372 (view as bug list)
Depends on: 17829
Blocks:
  Show dependency treegraph
 
Reported: 2018-03-07 08:19 UTC by Jose Martin
Modified: 2018-12-03 20:03 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Replaces "or" with "||" in variable assignment (950 bytes, patch)
2018-03-07 12:27 UTC, Jose Martin
Details | Diff | Splinter Review
Patch with signoff (1.08 KB, patch)
2018-03-08 15:04 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 20348: SIP2 patron identification fails to use userid (1.12 KB, patch)
2018-03-09 08:02 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jose Martin 2018-03-07 08:19:46 UTC
The following code in C4/SIP/ILS/Patron.pm:

    $kp = Koha::Patrons->find( { cardnumber => $patron_id } )
        or Koha::Patrons->find( { userid => $patron_id } );

should be (using || instead of or):

    $kp = Koha::Patrons->find( { cardnumber => $patron_id } )
        || Koha::Patrons->find( { userid => $patron_id } );

It will never try to match a patron by userid.
This is a bug according to Perl documentation here: https://perldoc.perl.org/perlop.html#Logical-or-and-Exclusive-Or
Comment 1 Jose Martin 2018-03-07 12:27:10 UTC
Created attachment 72502 [details] [review]
Replaces "or" with "||" in variable assignment
Comment 2 Colin Campbell 2018-03-08 15:04:26 UTC
Created attachment 72539 [details] [review]
Patch with signoff

Signed off - added CR to commit message to avoid auto wrapping
added a line to clarify what was being fixed
Comment 3 Colin Campbell 2018-03-08 15:10:26 UTC
Confirmed that lookup by userid was not being executed and that reverting the or to || does correct that and restores the code to a working lookup.

People should beware of prettifying the code by replacing operators with operators that are not equivalent. Specially as in this case when the buggy change is hidden in a large commit which is not explicit about making the change
Comment 4 Josef Moravec 2018-03-09 08:02:52 UTC
Created attachment 72556 [details] [review]
Bug 20348: SIP2 patron identification fails to use userid

Replaces "or" with "||" in variable assignment

second attempt to retrieve borrower was not being executed due
to changed precedence

Signed-off-by: Colin Campbell <colin.campbell@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 5 Jonathan Druart 2018-03-19 16:29:31 UTC
Caused by
  commit 2b90ea2cb0e5e976de7ddef0151ae83d8ac578e6
    Bug 17829: Move GetMember to Koha::Patron
Comment 6 Jonathan Druart 2018-03-19 18:29:27 UTC
Patch pushed to master for 18.05

Thanks Jose for your first patch in!
Comment 7 Magnus Enger 2018-03-23 10:31:03 UTC
Hooray for Jose! This also needs to go into the stable branch, please.
Comment 8 Magnus Enger 2018-03-23 10:31:47 UTC
*** Bug 10372 has been marked as a duplicate of this bug. ***
Comment 9 Nick Clemens 2018-03-23 12:53:13 UTC
Awesome work all! Pushed to stable for 17.11.04
Comment 10 Fridolin Somers 2018-03-27 11:45:22 UTC
Not in 17.05.x since Bug 17829 not in it