Is this patch ready for testing and sign-off?

Yes, unfortunately I don't get that drop down with "Needs signoff"

Change Importance to "Patch sent" - the pull down only shows when this status is set.

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.

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.

If we give up being able to search extended attributes that's no small regression. I would say that prevents this from passing QA.

I agree with Owen, we should not lose the ability to search attributes in favor of this patch.

Patch causes regression of functionality; marking as Failed QA

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?

Owen and Ian, what is the status on getting a db dump with failing search?

Srdjan,

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.

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.

Thanks Srdjan. I'll take a look at this again today.

Maybe others would like to look too?

I should add, I tested all from every place I know of where one can search patrons, so:

mainpage.pl
members-home.pl
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

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

That signed off patch conflicts with bug 6773 pushed changes.
Find a new patch attached to limit the query to 10.

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).

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
C4::SQLHelper:
- 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

From:
mainpage.pl
members-home.pl
	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.

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

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.

(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.

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.

Inner name searching can be traced on bug report 7053.

Pushed, please test

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.

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.

Pushed followup

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

Searching for patrons seems to work. Please reopen the bug if there is a problem.