Bug 6253

Summary: Unified Patron Search subroutine
Product: Koha Reporter: Ian Walls <koha.sekjal>
Component: PatronsAssignee: Srdjan Jankovic <srdjan>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: normal    
Priority: PATCH-Sent (DO NOT USE) CC: chris, dev_patches, gmcharlt, katrin.fischer, nengard, paul.poulain, robin, wizzyrea
Version: 3.6   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 1707, 4340, 4945, 5266, 5595, 5676, 5730, 5945, 6127    
Bug Blocks: 7053    
Attachments: patch
bug_6253: Removed C4::Members::SearchMembers() in favour of Search(). Amended Search() so it does support some fetures available in SearchMembers(). C4::SQLHelper::SearchInTable() - some fiddling with search type (convert "start_with" to "contain") to sup
[Signed Off] bug_6253: Unified member Search()
replacement patch in order to fix conflict on circ/ysearch.pl
[Signed Off] bug_6253: Unified member Search()
Proposed followup patch

Description Ian Walls 2011-04-22 13:55:27 UTC
Currently Koha uses two patron search routines, Search() and SearchMember().  SearchMember() is used in most places where a patron search is performed, and does not respect Extended Attributes, branchcode/categorycode limits or paging of results.  It does, however, return exact matches for cardnumbers quickly, making it very desirable for Circulation.

Search() is only used on members/member.pl to do an in-depth search of patrons.  It makes use of SQLHelper.pm, which gives us support for Extended Attributes OR branchcode/categorycode limits (can't have both, it seems) and results paging.  It's easy to add new fields from the borrowers table to search.

Koha should really only have 1 search routine for searching for patrons.  This routine should support Extended Attributes, branchcode/categorycode limits and pagination.  Exact matches on cardnumber OR unique, searchable Extended Attributes should return immediately, without having to go through the full search routine.  It should also be easy to configure which fields in the borrowers table are searchable.
Comment 1 Srdjan Jankovic 2011-08-16 01:12:54 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2011-08-16 12:24:32 UTC
Is this patch ready for testing and sign-off?
Comment 3 Srdjan Jankovic 2011-08-16 12:55:07 UTC
Yes, unfortunately I don't get that drop down with "Needs signoff"
Comment 4 Katrin Fischer 2011-08-16 12:57:10 UTC
Change Importance to "Patch sent" - the pull down only shows when this status is set.
Comment 5 Liz Rea 2011-08-16 14:35:23 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2011-08-24 16:02:23 UTC
I find that after applying this patch I no longer get results for two patron attribute searches which work in current master:

- a repeatable, searchable extended patron attribute not tied to an
  authorized value
- a repeatable, searchable extended patron attribute tied to an
  authorized value (code only. No results from the description.)

On the other hand, all the multiple-name searches I tried worked correctly.
Comment 7 Ian Walls 2011-08-24 18:20:48 UTC
This patch makes use of SQLHelper as the driver for search.  The problem is that when using SQLHelper, we have to choose between supporting limitation by branchcode/categorycode OR extended attribute searching.  You can't have both, not with the way SQLHelper is coded.

So, either SQLHelper needs to change, which will have repercussions in Acquisitions (the only area it's really used right now aside from the patron search subroutine), or we need to bypass it altogether when doing Patron Search.  I'd recommend the latter.
Comment 8 Owen Leonard 2011-08-25 16:24:12 UTC
If we give up being able to search extended attributes that's no small regression. I would say that prevents this from passing QA.
Comment 9 Nicole C. Engard 2011-08-25 21:11:53 UTC
I agree with Owen, we should not lose the ability to search attributes in favor of this patch.
Comment 10 Ian Walls 2011-08-25 21:15:53 UTC
Patch causes regression of functionality; marking as Failed QA
Comment 11 Srdjan Jankovic 2011-09-05 02:52:08 UTC
There was no intention to search exclusively on branchcode/categorycode OR extended attribute searching. The algorithm it uses is:
* Search for the card number
* If not found and preference('ExtendedPatronAttributes') SearchIdMatchingAttribute
* If not found do SQLHelper::SearchInTable()

Can we please organise a database dump and example of the search that is not working?
Comment 12 Srdjan Jankovic 2011-09-21 05:20:02 UTC Comment hidden (obsolete)
Comment 13 Srdjan Jankovic 2011-09-21 05:22:08 UTC
Owen and Ian, what is the status on getting a db dump with failing search?
Comment 14 Liz Rea 2011-09-21 16:17:50 UTC

I'm looking at this new patch today. I think I can help you with what is failing.

Go to Administration -> Patron Attribute Types

Add a patron attribute type, mine looked like this: http://screencast.com/t/B0vQ8WZMI

Go to your test patron, edit it to add a borrower attribute of "tabby" (or whatever)

Without your patch, a search for tabby brings back the patron.

With it, the search fails.
Comment 15 Srdjan Jankovic 2011-09-22 04:38:57 UTC Comment hidden (obsolete)
Comment 16 Srdjan Jankovic 2011-09-22 04:41:09 UTC
Thanks Liz, that helped. I think it works now as expected.
A note: I'm not sure that in current master attributes search works well when more than one patron should be returned.
Comment 17 Liz Rea 2011-09-22 14:25:30 UTC
Thanks Srdjan. I'll take a look at this again today.

Maybe others would like to look too?
Comment 18 Liz Rea 2011-09-22 14:53:31 UTC Comment hidden (obsolete)
Comment 19 Liz Rea 2011-09-22 14:59:22 UTC
I should add, I tested all from every place I know of where one can search patrons, so:

The "Check Out" field in the masthead.

I also tested the following, which I neglected to include in my commit message:

Patron search limited by branch: Works
Patron search limited by patron category: works
Comment 20 Liz Rea 2011-09-22 15:13:43 UTC
lol, sorry for all the mail. I'm going back through the depends.

1707 - pages the results from the circulation form, does not crash mozilla. Fixed.
4340 - stem searches work: in my data, I have a bennett and a benson, "ben" brings back both, "ett" brings back nothing. Seems ok.
4945 - has a patch, needs a sign off. Will try to get it.
5266 - not enough data in my test db to test.
5730 - ambiguous - not sure what to do with it
5945 - not addressed
6127 - fixed as noted
Comment 21 dev_patches 2011-09-28 08:49:36 UTC
That signed off patch conflicts with bug 6773 pushed changes.
Find a new patch attached to limit the query to 10.
Comment 22 dev_patches 2011-09-28 08:54:21 UTC Comment hidden (obsolete)
Comment 23 Liz Rea 2011-09-28 16:56:06 UTC
The functionality of the patch seems to be maintained with the changes. A separate issue with autocomplete was found, but it predates this patch, so I'm going to sign off on the functionality with the understanding that there is still something wrong with autocomplete that is possibly unrelated to this patch.

signed off patch coming after lunch (and a bit more testing).
Comment 24 Liz Rea 2011-09-28 18:23:28 UTC
Created attachment 5632 [details] [review]
[Signed Off] bug_6253: Unified member Search()

Removed SearchMembers() and replaced with more generic Search()
Amended Search() to try cardnumber first
Replaced SearchMembers() calls with Search()
Replaced SELECT with Search() where appropriate
- added support for '' key for search filter.
- when passing an array to filter, join with OR (rather than AND)
- added support for key => [val1, val2] in filter
- did not document - there was no input documentation to start with,
  and SQLHelper should be replaced with something better anyway

Signed-off-by: Liz Rea <lrea@nekls.org>
(again - testing merge issue)
The functionality of the patch seems to be maintained with Biblibre's changes.

I tested the following:
Extended attribute searching: works
3 part name searching: works
2 part name searching: works
1 part name searching: works

	Patron search limited by branch: Works
	Patron search limited by patron category: works
	Ordering by cardnumber instead of surname: works
The "Check Out" field in the masthead.

Circ Autocomplete is not reliably functional at this time, but the problem appears to predate this patch.
Comment 25 Paul Poulain 2011-10-12 14:49:52 UTC
QA comment
  * those changes are made in a core part of Koha. I'm quite hesitating pushing such a change 2 weeks before a release (but i'm not RM for 3.6 ;-) ). I fear some side-effect could be missed.

  * One of the changes that could have consequences is:
-			$sql.= do { local $"=') AND ('; 
+			$sql.= do { local $"=') OR ('; 
(in SearchInTable)
doesn't this silent switch from AND to OR have possible side-effects ?
I see 2 possible effects:
  * loss of performances X_indexed = something AND Y_notindexed = SMTHelse result in a X index search while X_indexed = something OR Y_notindexed = SMTHelse won't. That could be a real problem !
  * different results. Of course A and B is different from A or B

Maybe there's something i'm missing, but please clarify !
Note : SearchInTable are also/only used in Budgets.pm and Contract.pm
Comment 26 Srdjan Jankovic 2011-10-13 02:49:26 UTC
SQLHelper::SearchInTable() is not the best tool for creating queries, but it is much better than having nothing. Member search can get quite complex, so I had to address some deficiencies in order to be able to build it. Other parts of the system are using SearchInTable() in its simplest form, so they are not affected:
* SearchInTable() used in Budgets:
- admin/aqbudgetperiods.pl calls GetBudgetPeriods() without any params, ie SearchInTable() is used to return a full table scan
- suggestion/suggestion.pl calls GetBudgetPeriods() with simple filter budget_branchcode => $code, so no ANDing nor ORing; all other calls are without any params
* SearchInTable() used in Contracts:
- admin/aqcontract.pl calls GetContract() with simple filter contractnumber => $contractnumber, or booksellerid => $booksellerid
- acqui/supplier.pl calls GetContract() with simple filter booksellerid => $booksellerid

It is not up the query builder to consider indexing. Although some useful tweaks can be done (eg if value list is supplied that translates to field_x IN (val1, val2, ...) it is a good optimisation to write it as field_x = val1 if there's only one value), in general query builder should make an accurate query, and indexing should be considered outside.
Comment 27 Paul Poulain 2011-10-14 08:13:25 UTC
(In reply to comment #26)
> Other parts of
> the system are using SearchInTable() in its simplest form, so they are not
> affected:
OK, I confirm, other parts of the system are not affected.

> It is not up the query builder to consider indexing. Although some useful
> tweaks can be done (eg if value list is supplied that translates to field_x IN
> (val1, val2, ...) it is a good optimisation to write it as field_x = val1 if
> there's only one value), in general query builder should make an accurate
> query, and indexing should be considered outside.
I was not only a question of indexes, it also a question of which query we want to do. A and B won't return the same result as A or B (in french we say : "trying to push open-door", I agree ;-) )
So this change is not a small one. But as only your query uses it, it's OK.

So I vote for "passed QA", and pass the ball to Ian and/or chris : this update is not minor, please check my opinion & accept or object.
Comment 28 Ian Walls 2011-10-18 16:53:55 UTC
While this patch does not handle 'inner name' searching (that is "james acevedo" out of "henry james acevedo III") still fails, but that really isn't within the scope of this, and is a fringe case.  Since it can be easily worked around by putting "%" at the beginning of one's search string, I don't think it's worth blocking QA.

Fixes three part name searching, as well as category/library limits, while maintaining extended attribute searching.  Marking as Passed QA.

Good work everyone on this one!  Glad it's finally ready to roll.
Comment 29 Ian Walls 2011-10-18 17:18:29 UTC
Inner name searching can be traced on bug report 7053.
Comment 30 Chris Cormack 2011-10-19 03:24:21 UTC
Pushed, please test
Comment 31 Ian Walls 2011-10-19 15:45:48 UTC
This patch is breaking t/db_dependent/Members.t, but not staff client usage.

I believe the root of this is C4/SQLHelper.pm, line 418:

    $operand = [$operand] unless ref $operand;

ref $operand could return all kinds of values other than "ARRAY".  Modifying to:

    $operand = [$operand] unless ref $operand eq 'ARRAY';

prevents the script from erring out early.  This doesn't guarantee that all tests are passed, but they are all run with this change.

Followup patch forthcoming.
Comment 32 Ian Walls 2011-10-19 15:51:32 UTC
Created attachment 5986 [details] [review]
Proposed followup patch

Adds explicit check for ref $operand eq 'ARRAY' before deferencing as as an array.  This stops t/db_dependent/Members.t from aborting after test 3.  All tests now complete.
Comment 33 Chris Cormack 2011-10-19 20:32:40 UTC
Pushed followup
Comment 34 Srdjan Jankovic 2011-10-19 23:38:24 UTC
I am not sure if this was the right thing to do. Although it fixes the test throwing an exception, a hashref makes no sense there - will make a pointles condition tin SQL that will evaluate false. What needs to be done is either
* fix the test or
* support hashref
I believe that fixing the test is the way to go
Comment 35 Jared Camins-Esakov 2012-05-23 12:04:08 UTC
Searching for patrons seems to work. Please reopen the bug if there is a problem.