From 33ee86e2225e710f9c3c65e795ab6e90c733efa9 Mon Sep 17 00:00:00 2001 From: Maryse Simard Date: Tue, 19 Nov 2019 09:12:35 -0500 Subject: [PATCH] Bug 11808: (QA follow-up) Adds support for other patron searches This patch add supports to other pages where patron search is possible: patron search, hold and article requests. Removes the code that tried to support searching for search fields other than cardnumber in circulation.pl Catch exception on broken FK contraint if information on the LDAP server can't be added to the database as to not interrupt execution. The error message is shown on the UI. Revised test plan: 0. You need to have an ldap server containing patron informations configured in Koha. 1. Apply patch and update database. 2. Check that the new system preference SearchCardnumberWithLDAP has been added and set as 'never'. 3. Choose a patron who exists in your ldap server but not in Koha. (you can delete an existing patron from Koha if needed) 4. Search for the patron by it's cardnumber in the following locations. Do not choose from the dropdown list, use the submit button. - circ/circulation.pl - circ/article-request.pl - reserve/request.pl - members/member.pl both at the top and in the filter box - any page using common/patron_search.tt, for example adding a guarantor with the members/guarantor_search.pl popup => Should return cardnumber/patron not found or no results 5. Change SearchCardnumberWithLDAP to 'if not found locally'. 6. Repeat step 4. => The patron should be added to Koha. 7. Edit the patron and change any of the existing information. 8. Repeat step 4. => The patron has not been updated (the change made in step 7 is still there) 9. Change SearchCardnumberWithLDAP to 'always'. 10. Repeat step 4. => The patron has been updated. Change made in step 7 has been rewritten and patron informations is the same as in step 6. 11. Try searching for a patron who doesn't exist in Koha and has a nonexistant branch on the LDAP server. An error message should appear accompanying the absence of results. --- C4/Auth_with_ldap.pm | 38 ++++++++++------- circ/circulation.pl | 47 ++++++++-------------- circ/request-article.pl | 23 +++++++++-- .../en/modules/acqui/tables/members_results.tt | 1 + .../prog/en/modules/circ/circulation.tt | 2 +- .../prog/en/modules/circ/request-article.tt | 6 +++ .../prog/en/modules/common/patron_search.tt | 4 ++ .../prog/en/modules/members/member.tt | 7 ++++ .../en/modules/members/tables/guarantor_search.tt | 1 + .../en/modules/members/tables/members_results.tt | 1 + .../modules/patroncards/tables/members_results.tt | 1 + .../prog/en/modules/reserve/request.tt | 6 +++ .../en/modules/serials/tables/members_results.tt | 1 + members/member.pl | 14 +++++++ reserve/request.pl | 14 +++++++ svc/members/search | 14 +++++++ 16 files changed, 130 insertions(+), 50 deletions(-) diff --git a/C4/Auth_with_ldap.pm b/C4/Auth_with_ldap.pm index 57faab9..a0ef39d 100644 --- a/C4/Auth_with_ldap.pm +++ b/C4/Auth_with_ldap.pm @@ -32,6 +32,7 @@ use Koha::AuthUtils qw(hash_password); use List::MoreUtils qw( any ); use Net::LDAP; use Net::LDAP::Filter; +use Try::Tiny; use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $debug); @@ -401,36 +402,38 @@ sub updateborrower_ldap { my $patron = shift; my $cardnumber = $patron->cardnumber; - return 0 unless ($config{update}); + return unless ($config{update}); my @hosts = split(',', $prefhost); - my $db = Net::LDAP->new(\@hosts, timeout => ($ldap->{timeout} || 30)) or return 0; + my $db = Net::LDAP->new(\@hosts, timeout => ($ldap->{timeout} || 30)) or return; my $cardnumber_field = $mapping{cardnumber}->{is} or die ldapserver_error("mapping for 'cardnumber'"); my $filter = Net::LDAP::Filter->new("$cardnumber_field=$cardnumber") or die "Failed to create new Net::LDAP::Filter"; my $res = ($config{anonymous}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword); if ($res->code) { # connection refused warn "LDAP bind failed as ldapuser " . ($ldapname || '[ANONYMOUS]') . ": " . description($res); - return 0; + return; } my $search = $db->search( base => $base, filter => $filter, ); - # Search error, return 0 - return (0, $search->error) if $search->code; + # Search error, return undef + return (undef, $search->error) if $search->code; my $count = $search->count; if ($count == 1) { # User was found, update! my %borrower = ldap_entry_2_hash($search->shift_entry,$cardnumber); + &update_local($patron->userid, $patron->password, $patron->borrowernumber, \%borrower); - return 1; + my $updated_patron = Koha::Patrons->find( $patron->borrowernumber ); + return $updated_patron; } else { - return 0; + return; } } @@ -440,25 +443,25 @@ sub updateborrower_ldap { sub findcardnumber_ldap { my $cardnumber = shift; - return 0 unless ($config{replicate}); + return unless ($config{replicate}); my @hosts = split(',', $prefhost); - my $db = Net::LDAP->new(\@hosts, timeout => ($ldap->{timeout} || 30)) or return 0; + my $db = Net::LDAP->new(\@hosts, timeout => ($ldap->{timeout} || 30)) or return; my $cardnumber_field = $mapping{cardnumber}->{is} or die ldapserver_error("mapping for 'cardnumber'"); my $filter = Net::LDAP::Filter->new("$cardnumber_field=$cardnumber") or die "Failed to create new Net::LDAP::Filter"; my $res = ($config{anonymous}) ? $db->bind : $db->bind($ldapname, password=>$ldappassword); if ($res->code) { # connection refused warn "LDAP bind failed as ldapuser " . ($ldapname || '[ANONYMOUS]') . ": " . description($res); - return 0; + return; } my $search = $db->search( base => $base, filter => $filter, ); - # Search error, return 0 - return (0, $search->error) if $search->code; + # Search error, return undef + return (undef, $search->error) if $search->code; my $count = $search->count; if ($count == 1) { @@ -469,11 +472,16 @@ sub findcardnumber_ldap { my @chars = ( "A" .. "Z", "a" .. "z", 0 .. 9, qw(! @ $ % ^ & *) ); $borrower{'password'} = join("", @chars[ map { rand @chars } ( 1 .. 30 ) ]) unless $borrower{'password'}; - my $patron = Koha::Patron->new( \%borrower )->store; - return $patron->borrowernumber; + try { + my $patron = Koha::Patron->new( \%borrower )->store; + $patron = Koha::Patrons->find( $patron->borrowernumber ); + return $patron; + } catch { + return (undef, $_->message); + }; } else { - return 0; + return; } } diff --git a/circ/circulation.pl b/circ/circulation.pl index a829f70..aa01035 100755 --- a/circ/circulation.pl +++ b/circ/circulation.pl @@ -250,13 +250,22 @@ if ( $print eq 'yes' && $borrowernumber ne '' ) { my $message; if ($findborrower) { my $patron = Koha::Patrons->find( { cardnumber => $findborrower } ); + + # search cardnumber on LDAP server + if ( C4::Context->config('useldapserver') + and ( my $search_cardnumber = C4::Context->preference("SearchCardnumberWithLDAP") ) ne "never" + ) { + my $ldap_error; + if ( $patron ) { + ($patron, $ldap_error) = C4::Auth_with_ldap::updateborrower_ldap($patron) if $search_cardnumber eq "always"; + } else { + ($patron, $ldap_error) = C4::Auth_with_ldap::findcardnumber_ldap($findborrower); + } + $template->param(ldap_error => $ldap_error) if $ldap_error; + } + if ( $patron ) { $borrowernumber = $patron->borrowernumber; - # Update the user with LDAP if we need to - if(C4::Context->config('useldapserver') and C4::Context->preference("SearchCardnumberWithLDAP") eq "always") { - my ($result, $ldap_error) = C4::Auth_with_ldap::updateborrower_ldap($patron); - $template->param(ldap_error => $ldap_error) if $ldap_error; - } } else { my $dt_params = { iDisplayLength => -1 }; my $results = C4::Utils::DataTables::Members::search( @@ -269,35 +278,13 @@ if ($findborrower) { my $borrowers = $results->{patrons}; if ( scalar @$borrowers == 1 ) { $borrowernumber = $borrowers->[0]->{borrowernumber}; - # Update the user with LDAP if we need to - if(C4::Context->config('useldapserver') and C4::Context->preference("SearchCardnumberWithLDAP") eq "always") { - my $patron_obj = Koha::Patrons->find( $borrowernumber ); - my ($result, $ldap_error) = C4::Auth_with_ldap::updateborrower_ldap($patron_obj); - $template->param(ldap_error => $ldap_error) if $ldap_error; - } $query->param( 'borrowernumber', $borrowernumber ); $query->param( 'barcode', '' ); - } elsif ( @$borrowers == 0 ) { - # Try to find a user using LDAP if we couldn't find it locally - my $borrowernumber_ldap; - my $ldap_error; - if (C4::Context->config('useldapserver') and - (C4::Context->preference("SearchCardnumberWithLDAP") eq "always" or C4::Context->preference("SearchCardnumberWithLDAP") eq "not_found") - ) { - ($borrowernumber_ldap, $ldap_error) = C4::Auth_with_ldap::findcardnumber_ldap($findborrower); - $template->param(ldap_error => $ldap_error) if $ldap_error; - } - - if ($borrowernumber_ldap) { - $borrowernumber = $borrowernumber_ldap; - } - else { - $template->param(ldap_error => $ldap_error) if $ldap_error; - $query->param( 'findborrower', '' ); - $message = "'$findborrower'"; - } } elsif ( @$borrowers ) { $template->param( borrowers => $borrowers ); + } else { + $query->param( 'findborrower', '' ); + $message = "'$findborrower'"; } } } diff --git a/circ/request-article.pl b/circ/request-article.pl index 7c0877b..f595d9b 100755 --- a/circ/request-article.pl +++ b/circ/request-article.pl @@ -49,10 +49,25 @@ my $biblio = Koha::Biblios->find($biblionumber); output_and_exit( $cgi, $cookie, $template, 'unknown_biblio') unless $biblio; -my $patron = - $patron_id ? Koha::Patrons->find($patron_id) - : $patron_cardnumber ? Koha::Patrons->find( { cardnumber => $patron_cardnumber } ) - : undef; +my $patron = undef; +if ( $patron_id ) { + $patron = Koha::Patrons->find($patron_id); +} elsif ( $patron_cardnumber ) { + $patron = Koha::Patrons->find( { cardnumber => $patron_cardnumber } ); + + # search cardnumber on LDAP server + if ( C4::Context->config('useldapserver') + and ( my $search_cardnumber = C4::Context->preference("SearchCardnumberWithLDAP") ) ne "never" + ) { + my $ldap_error; + if ( $patron ) { + ($patron, $ldap_error) = C4::Auth_with_ldap::updateborrower_ldap($patron) if $search_cardnumber eq "always"; + } else { + ($patron, $ldap_error) = C4::Auth_with_ldap::findcardnumber_ldap($patron_cardnumber); + } + $template->param(ldap_error => $ldap_error) if $ldap_error; + } +} if ( $action eq 'create' ) { my $borrowernumber = $cgi->param('borrowernumber'); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt index b90a8d7..93dd863 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/tables/members_results.tt @@ -1,5 +1,6 @@ [% USE To %] { + [% IF ldap_error %]"ldap_error": "[% ldap_error %]",[% END %] "sEcho": [% sEcho | html %], "iTotalRecords": [% iTotalRecords | html %], "iTotalDisplayRecords": [% iTotalDisplayRecords | html %], diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt index d4a9bf0..796ccc5 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -479,7 +479,7 @@ [% IF (ldap_error) %]
- [% IF ( ldap_error ) %]

An error occured while trying to contact the LDAP server : [% ldap_error | html %]

[% END %] + An error occured while searching for cardnumber on the LDAP server : [% ldap_error | html %]
[% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt index e2257fc..7e50bac 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/request-article.tt @@ -33,6 +33,12 @@
+ [% IF (ldap_error) %] +
+ An error occured while searching for cardnumber on the LDAP server : [% ldap_error | html %] +
+ [% END %] +

Request article from [% INCLUDE 'biblio-title.inc' %]

[% IF no_patrons_found %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt index 54f9899..80d5309 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/common/patron_search.tt @@ -175,6 +175,10 @@ 'url': sSource, 'data': aoData, 'success': function(json){ + if (json.ldap_error) { + $("#error").html(_("An error occured while searching for cardnumber on the LDAP server : %s").format(json.ldap_error)); + $("#error").show(); + } fnCallback(json); } }); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt index 369acb2..06f8d55 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt @@ -88,6 +88,8 @@
[% END %] +
+ @@ -234,6 +236,7 @@ [% Asset.js("js/members-menu.js") | $raw %]