Bug 9407 - patrons search should match substrings
Summary: patrons search should match substrings
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: MJ Ray (software.coop)
QA Contact: Jonathan Druart
URL: http://intranet/cgi-bin/koha/members/...
Keywords:
Depends on: 8516
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-16 19:14 UTC by MJ Ray (software.coop)
Modified: 2014-05-26 21:04 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9070: patrons search should match substrings (2.72 KB, patch)
2013-01-16 20:25 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 9407: patrons search should match substrings (2.72 KB, patch)
2013-01-16 20:30 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
Bug 9407: patrons search should match substrings (2.66 KB, patch)
2013-01-17 11:00 UTC, MJ Ray (software.coop)
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9407: patrons search should match substrings (2.71 KB, patch)
2013-01-18 14:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9407: patrons search should match substrings (2.79 KB, patch)
2013-01-22 09:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description MJ Ray (software.coop) 2013-01-16 19:14:00 UTC
If a patron lives at 4345 Library Rd, a search on Street Address for Library should find it. However, it does not, but a search for 4345 Library does.
Comment 1 MJ Ray (software.coop) 2013-01-16 20:25:18 UTC Comment hidden (obsolete)
Comment 2 MJ Ray (software.coop) 2013-01-16 20:30:24 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-01-17 08:46:53 UTC
Hi MJ, could you resubmit your patch please?

Applying: Bug 9407: patrons search should match substrings
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9407: patrons search should match substrings
Comment 4 MJ Ray (software.coop) 2013-01-17 11:00:51 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-01-17 14:34:00 UTC
MJ,

Thanks!

I will signoff your patch but I have 1 question:
Don't you think the following change gives the code easier to read?

-my $searchtype = $input->param('searchtype');
-my %searchtype_ok = ( 'contain' => 1 );
-if ( !defined($searchtype_ok{$searchtype}) ) {
-    undef $searchtype;
-}
-
 my $from = ( $startfrom - 1 ) * $resultsperpage;
 my $to   = $from + $resultsperpage;
 
 my ($count,$results);
 if ($member || keys %$patron) {
     #($results)=Search($member || $patron,{surname=>1,firstname=>1},[$from,$to],undef,["firstname","surname","email","othernames"]  );
-    my $search_scope = $searchtype || ( $quicksearch ? "field_start_with" : "start_with" );
+    my $search_scope = ( $input->param('searchtype') eq 'contain' )
+        ? 'contain'
+        : ( $quicksearch ? "field_start_with" : "start_with" );
Comment 6 MJ Ray (software.coop) 2013-01-18 11:36:53 UTC
That change makes the code easier to read for now, but would have to be changed to something like the perlfaq4 idiom I used if C4::Patrons::Search (and C4::SQLHelper::SearchInTable) gains other possible values that we want to allow, or if someone chooses to add the currently-supported "exact" type.  Should I add a comment to note that I'm using the perlfaq4 idiom to ease future expansion, or use the simpler form for now?
Comment 7 Kyle M Hall 2013-01-18 14:58:47 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2013-01-22 09:15:28 UTC
(In reply to comment #6)
> That change makes the code easier to read for now, but would have to be
> changed to something like the perlfaq4 idiom I used if C4::Patrons::Search
> (and C4::SQLHelper::SearchInTable) gains other possible values that we want
> to allow, or if someone chooses to add the currently-supported "exact" type.
> Should I add a comment to note that I'm using the perlfaq4 idiom to ease
> future expansion, or use the simpler form for now?

MJ,

I think it is more complicated to maintain a code in a philosophy of "think about the future if... and if ...".
To me the code must be simple at a given moment. So I am in favour of not modifying the code until someone add another value.
It was just a proposition and you presented arguments so the patch can pass QA :)
Comment 9 Jonathan Druart 2013-01-22 09:15:41 UTC
Created attachment 14736 [details] [review]
Bug 9407: patrons search should match substrings

If a patron lives at 4345 Library Rd, a search on Street Address for
    Library should find it. However, it does not, but a search for 4345
    Library does.  This patch adds a "Search Type" drop-down, defaulting
    to the current behaviour.

    To test:
    1) Search for a patron based on a non-leading part of a field.
    2) Should return no results.
    3) Search again with "Search Type" of "Contains".
    4) Should return the patron.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jared Camins-Esakov 2013-01-22 23:56:14 UTC
This patch has been pushed to master.
Comment 11 Chris Cormack 2013-02-01 08:25:42 UTC
Pushed to 3.10.x and 3.8.x will be in 3.10.3 ad 3.8.10