LP1084269, LP1050043 Expert Search Fixes
authorLiam Whalen <liam.whalen@bc.libraries.coop>
Sat, 28 Jun 2014 22:37:28 +0000 (15:37 -0700)
committerBen Shum <bshum@biblio.org>
Wed, 27 Aug 2014 04:25:57 +0000 (00:25 -0400)
Sorting Expert Search corrected

LP1050043 was issued because attempting to sort the MARC expert search
results, in the staff client, caused the staff client to return to the
basic search page rather than sort the results.

To fix the sorting I had to modify two perl files.

metabib.pm had to be changed in a number of ways.

1. The way the sort works currently assumes the user is doing a full
text search.  So, it passes in the values of titlesort and authorsort as
the sort variable in the URL.  However, in the expert search, it is
looking for the values 'title' and 'author'.  This resulted in the title
sort and author sort never being executed and returning the relevance
sort instead. I modified the search to use a regex instead of eq.

2. If a record has no value associated with it e.g. no 260 c for
pubdate, no 245 $a for title, no 1xx $a for author, then the SQL is
supposed to replace those values with either 'AAAA' or 'zzzz' depending
on the order of the sort.  This should cause records without the
relevant sort values to be pushed to the bottom of the sort results.

But, the PgSQL function COALESCE, which is supposed to do the
replacing, does not do anything if no rows are returned for a record's
relevant value.  To work around this, I surrounded the relevant values
with the MAX() function call, which returns NULL if no row is returned.

Because author sort works on any 1xx value, it sorts those values in
numerical order when there is more than one 1xx in a record.  This
requires the use of an ORDER BY statement, which seems to nullify
the effect of MAX().  So, I had to move the MAX() into a second SELECT
statement in the case of author sort.

3. I added the variables number_default_sort and string_default_sort
to the metabib code because the current code hard codes values of
'9999' and 'zzzzzzzz' into the sort.

Search.pm had to be changed to pass the sort values from the TPAC
into the relevant sort subroutines.  This was done with the addition of
an $arghash variable that checks the cgi parameters for the sort
variable.

As well, Expert search puts the tag and _special params in the
search form.  This causes all other searches to perform Expert searches
afterwords.  To fix this, Search.pm is modified to look for an empty
query value before it conducts an Expert search.  If it is empty and
the Expert search variables are present, then the Expert search is
performed.  Otherwise, a normal search is done.

Finally, to fix the problem reported by LP1050043, I had to modify
searchbar.tt2.  Currently, searchbar.tt2 does not have the values from
the Expert search entered into the <form> where the sort functionality
is located.  This means, when a sort option is chosen, the TPAC no
longer knows which values were used for the Expert search.

This is not obvious at first because after choosing to sort, the
user is returned to a page where the URL has the Expert search values
in it.  This happens because the sort is submitting a form without a
query value, so the TPAC redirects the user to the referring page,
which is the initial Expert search.

To fix this, I added the Expert search values as hidden variables
into the <form> where the sort functionality is located.  I also
removed the use of the "_adv" hidden variable from the <form> when
not in the advanced search.

[LP1094269] Retrive past 100 records in Expert Search

Currently, when paging through the results of the Expert search, the
system will not return more than 100 records.  Going past page 10
causes a no results screen.

This is due to the offset and limit values not being passed into the
argument hash used to create the SQL for the search.  Once the SQL
returns, it returns record ids within the range of the offset and
the limit. So, if the offset is 100 and the limit is 10, the record
ids for search results 100 to 109 are returned.

But, because offset and limit are not being passed into the arghash,
they default to 0 and 100 respectively.  This means the first 100
records are always returned regardless of where in the search pages
you are viewing.  These 100 results were then pruned to the relevant
10 results in Biblio.pm.  However, if the offset starts beyond 100
then no results are returned because the results passed 100 are not
returned by metabib.pm.

This commit passes the offset and limit as arguments in the search
hash.  It also removes offset and limit as parameters of the
search.biblio.marc call. Because offset and limit are needed in the
arghash to limit the number of records returned, it is better to
pass them only via the arghash to ensure that no more than the
necessary number of records are returned.

Offset and limit could be kept as parameters, but it would require
modifying metabib as well to look for those parameters, so I think
forcing the Expert search to pass them as argument parameters is the
better option.

As well, this commit removes the pruning in Biblio.pm that was based
on the offset and limit because that is handled in metabib.pm now
that offset and limit are being supplied in the argument hash.

I have checked the source code and the marc_search function in
Biblio.pm is only called via the Expert search in
WWW/EGCatLoader/Search.pm, so modifying Biblio.pm in this manner
will only affect the Expert search and nothing else.

Signed-off-by: Liam Whalen <liam.whalen@bc.libraries.coop>
Signed-off-by: Sarah Childs <sarahc@zionsvillelibrary.org>
Signed-off-by: Ben Shum <bshum@biblio.org>

Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/metabib.pm
Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm
Open-ILS/src/templates/opac/parts/result/table.tt2
Open-ILS/src/templates/opac/parts/searchbar.tt2

index 15685b0..22c0966 100644 (file)
@@ -2176,8 +2176,7 @@ __PACKAGE__->register_method(
                         'See perldoc ' . __PACKAGE__ . ' for more detail.',
                 type => 'object'
             },
-            {desc => 'limit (optional)',  type => 'number'},
-            {desc => 'offset (optional)', type => 'number'}
+            {desc => 'timeout (optional)',  type => 'number'}
         ],
         return => {
             desc => 'Results object like: { "count": $i, "ids": [...] }',
@@ -2187,7 +2186,7 @@ __PACKAGE__->register_method(
 );
 }
 
-=head3 open-ils.search.biblio.marc (arghash, limit, offset)
+=head3 open-ils.search.biblio.marc (arghash, timeout)
 
 As elsewhere the arghash is the required argument, and must be a hashref.  The keys are:
 
@@ -2244,16 +2243,17 @@ Presently, search uses the cache unconditionally.
 =cut
 
 # FIXME: that example above isn't actually tested.
+# FIXME: sort and limit added.  item_type not tested yet.
 # TODO: docache option?
 sub marc_search {
-    my( $self, $conn, $args, $limit, $offset, $timeout ) = @_;
+    my( $self, $conn, $args, $timeout ) = @_;
 
     my $method = 'open-ils.storage.biblio.full_rec.multi_search';
     $method .= ".staff" if $self->api_name =~ /staff/;
     $method .= ".atomic";
 
-    $limit  ||= 10;     # FIXME: what about $args->{limit} ?
-    $offset ||=  0;     # FIXME: what about $args->{offset} ?
+    my $limit = $args->{limit} || 10;
+    my $offset = $args->{offset} || 0;
 
     # allow caller to pass in a call timeout since MARC searches
     # can take longer than the default 60-second timeout.  
@@ -2274,7 +2274,6 @@ sub marc_search {
 
         if($resp and $recs = $resp->content) {
             put_cache($ckey, scalar(@$recs), $recs);
-            $recs = [ @$recs[$offset..($offset + ($limit - 1))] ];
         } else {
             $recs = [];
         }
index 0fe1a34..fad6ef3 100644 (file)
@@ -509,11 +509,17 @@ sub biblio_multi_search_full_rec {
     my $relevance = 'sum(f.sum)';
     $relevance = 1 if (!$copies_visible);
 
+    my $string_default_sort = 'zzzz';
+    $string_default_sort = 'AAAA' if ($sort_dir =~ /^DESC$/i);
+
+    my $number_default_sort = '9999';
+    $number_default_sort = '0000' if ($sort_dir =~/^DESC$/i);
+
     my $rank = $relevance;
     if (lc($sort) eq 'pubdate') {
         $rank = <<"        RANK";
             ( FIRST ((
-                SELECT  COALESCE(SUBSTRING(frp.value FROM E'\\\\d+'),'9999')::INT
+                SELECT  COALESCE(SUBSTRING(MAX(frp.value) FROM E'\\\\d{4}'), '$number_default_sort')::INT
                   FROM  $metabib_full_rec frp
                   WHERE frp.record = f.record
                     AND frp.tag = '260'
@@ -529,10 +535,10 @@ sub biblio_multi_search_full_rec {
         $rank = <<"        RANK";
             ( FIRST (( SELECT edit_date FROM $br_table rbr WHERE rbr.id = f.record)) )
         RANK
-    } elsif (lc($sort) eq 'title') {
+    } elsif (lc($sort) =~ /^title/i) {
         $rank = <<"        RANK";
             ( FIRST ((
-                SELECT  COALESCE(LTRIM(SUBSTR( frt.value, COALESCE(SUBSTRING(frt.ind2 FROM E'\\\\d+'),'0')::INT + 1 )),'zzzzzzzz')
+                SELECT  COALESCE(LTRIM(SUBSTR(MAX(frt.value), COALESCE(SUBSTRING(MAX(frt.ind2) FROM E'\\\\d+'),'0')::INT + 1 )),'$string_default_sort')
                   FROM  $metabib_full_rec frt
                   WHERE frt.record = f.record
                     AND frt.tag = '245'
@@ -540,16 +546,19 @@ sub biblio_multi_search_full_rec {
                   LIMIT 1
             )) )
         RANK
-    } elsif (lc($sort) eq 'author') {
+    } elsif (lc($sort) =~ /^author/i) {
         $rank = <<"        RANK";
             ( FIRST((
-                SELECT  COALESCE(LTRIM(fra.value),'zzzzzzzz')
-                  FROM  $metabib_full_rec fra
-                  WHERE fra.record = f.record
-                    AND fra.tag LIKE '1%'
-                    AND fra.subfield = 'a'
-                  ORDER BY fra.tag::text::int
-                  LIMIT 1
+                SELECT  COALESCE(LTRIM(MAX(query.value)), '$string_default_sort')
+                  FROM  (
+                            SELECT fra.value
+                            FROM $metabib_full_rec fra
+                            WHERE fra.record = f.record
+                                AND fra.tag LIKE '1%'
+                                AND fra.subfield = 'a'
+                            ORDER BY fra.tag::text::int
+                            LIMIT 1
+                        ) query
             )) )
         RANK
     } else {
index 3a69fb1..19f3a25 100644 (file)
@@ -351,7 +351,8 @@ sub load_rresults {
     # Special alternative searches here.  This could all stand to be cleaner.
     if ($cgi->param("_special")) {
         $self->timelog("Calling MARC expert search");
-        return $self->marc_expert_search(%args) if scalar($cgi->param("tag"));
+        return $self->marc_expert_search(%args) if (scalar($cgi->param("tag")) and
+            (!defined $cgi->param("query") or $cgi->param("query") =~ /^\s*$/));
         $self->timelog("Calling item barcode search");
         return $self->item_barcode_shortcut if (
             $cgi->param("qtype") and ($cgi->param("qtype") eq "item_barcode") and not $internal
@@ -686,10 +687,27 @@ sub marc_expert_search {
     $method .= '.staff' if $self->ctx->{is_staff};
     my $timeout = 120;
     my $ses = OpenSRF::AppSession->create('open-ils.search');
+
+    #when a sort variable is passed in we need to add its value to the arguments
+    my $arghash = {searches => $query, org_unit => $self->ctx->{search_ou}};
+    if (defined $self->cgi->param("sort")) {
+        my ($sort, $sort_dir) = split /\./, $self->cgi->param('sort');
+        $arghash = {%$arghash, sort => "$sort"};
+        if (!defined $sort_dir) {
+            $arghash = {%$arghash, sort_dir => "ASC"};
+        }
+    }
+
+    #add the offset and limit to the argash, so we can go past 100 results
+    #if offset and limit are not in the args then they default to 0 and 100
+    #respectively in biblio_multi_search_full_rec, which limits the results to 100
+    #records
+    $arghash = {%$arghash, offset => $offset, limit => $limit}; 
+
     my $req = $ses->request(
         $method,
-        {searches => $query, org_unit => $self->ctx->{search_ou}}, 
-        $limit, $offset, $timeout);
+        $arghash,
+        $timeout);
 
     my $resp = $req->recv($timeout);
     my $results = $resp ? $resp->content : undef;
index 25514e4..eac0bc5 100644 (file)
@@ -364,7 +364,7 @@ END;
                                                         <div class="results_aux_utils place_hold"><a
                                                                 href="[% mkurl(ctx.opac_root _ '/place_hold',
                                                                     {hold_target => rec.id, hold_type => hold_type, 
-                                                                      hold_source_page => mkurl()}, ['query']) %]"
+                                                                      hold_source_page => mkurl()}, ['query','tag','subfield','term','_special','sort','page']) %]"
                                                                 [% html_text_attr('title', l('Place Hold on [_1]', attrs.title)) %]
                                                                     class="no-dec"><img
                                                                 src="[% ctx.media_prefix %]/images/green_check.png"
index a7bdb90..ac594ee 100644 (file)
         [% IF ctx.processed_search_query OR (NOT is_advanced AND NOT is_special) %]
         <input name='page' type='hidden' value="0" />
         [% END %]
+        [% IF is_special %]
+            <input type="hidden" name="_special" value="1" /> [%
+            number_of_expert_rows = CGI.param('tag').list.size;
+            index = 0;
+            WHILE index < number_of_expert_rows %]
+                <input type="hidden" name="tag" value="[% CGI.param('tag').list.$index %]" />
+                <input type="hidden" name="subfield" value="[% CGI.param('subfield').list.$index %]" />
+                <input type="hidden" name="term" value="[% CGI.param('term').list.$index %]" />
+                [% index = index + 1; %]
+            [% END %]
+        [% END %]
     </div>
     [%- END %]
     [% UNLESS took_care_of_form %]</form>[% END %]