From: Liam Whalen <whalen.ld@gmail.com>
Date: Wed, 28 Aug 2013 05:52:58 +0000 (-0700)
Subject: A few fixes for the Marc Expert Search
X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=refs%2Fheads%2Fuser%2Fldw%2Fexpert_search_fixes;p=working%2FEvergreen.git

A few fixes for the Marc Expert Search

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.

As well, sorting was not working at all in the MARC expert search.

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 expert 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 number_default_sort and string_default_sort to the
metabib code because the current code was hard coding values of '9999'
and 'zzzzzzzz' into the sort.  I believe these hardcoded values are
all that is necessary because of the way ORDER BY works with empty
strings, but I think it is better to be explicit with the sorting values.

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.

Finally, to fix the problem of the staff client returning to the search
page after sorting an expert search, I modified 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.

Additionally, 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 page
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 the metabib.pm.

This commit passes the offset and limit as arguments. 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 sub parameters as well, but it would
require modifying metabib.pm to look for those parameters in both the
arghash and the sub 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
the offset and limit are being supplied as arghash parameters.

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.

As well, I corrected some indenation where spaces and tabs were used
on differnet lines in the same file.

Signed-off-by: Liam Whalen <whalen.ld@gmail.com>
---

diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
index 206812867b..84190cd664 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Search/Biblio.pm
@@ -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,21 +2243,22 @@ 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.  
-    # Default to 2 mins.  Arbitrarily cap at 5 mins.
-    $timeout = 120 if !$timeout or $timeout > 300;
+	# allow caller to pass in a call timeout since MARC searches
+	# can take longer than the default 60-second timeout.  
+	# Default to 2 mins.  Arbitrarily cap at 5 mins.
+	$timeout = 120 if !$timeout or $timeout > 300;
 
 	my @search;
 	push( @search, ($_ => $$args{$_}) ) for (sort keys %$args);
@@ -2267,19 +2267,17 @@ sub marc_search {
 	my $recs = search_cache($ckey, $offset, $limit);
 
 	if(!$recs) {
+		my $ses = OpenSRF::AppSession->create('open-ils.storage');
+		my $req = $ses->request($method, %$args);
+		my $resp = $req->recv($timeout);
 
-        my $ses = OpenSRF::AppSession->create('open-ils.storage');
-        my $req = $ses->request($method, %$args);
-        my $resp = $req->recv($timeout);
-
-        if($resp and $recs = $resp->content) {
+		if($resp and $recs = $resp->content) {
 			put_cache($ckey, scalar(@$recs), $recs);
-			$recs = [ @$recs[$offset..($offset + ($limit - 1))] ];
 		} else {
 			$recs = [];
 		}
 
-        $ses->kill_me;
+		$ses->kill_me;
 	}
 
 	my $count = 0;
diff --git a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/metabib.pm b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/metabib.pm
index 32d2cab4b0..91b356e888 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/metabib.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/Application/Storage/Publisher/metabib.pm
@@ -615,10 +615,17 @@ sub biblio_multi_search_full_rec {
 	$relevance = 1 if (!$copies_visible);
 
 	my $rank = $relevance;
+  
+	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);
+	
 	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'
@@ -634,10 +641,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'
@@ -645,23 +652,25 @@ 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 {
 		$sort = undef;
 	}
 
-
 	if ($copies_visible) {
 		$select = <<"		SQL";
 			SELECT	f.record, $relevance, count(DISTINCT cp.id), $rank
diff --git a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm
index 8d81cf5389..f5ba2b0096 100644
--- a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm
+++ b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm
@@ -649,10 +649,33 @@ 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) {
+            #DESC is the default value for the sort_dir, so we only need to
+            #pass a value to the args if we want an ascending sort. Ascending is indicated
+            #in the interface by only passing the sort axis. 
+            #For example, if I want descending authorsort then I pass
+            #authorsort.descending.  However, if I want ascending authorsort
+            #then I pass only authorsort in the url
+            $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;
diff --git a/Open-ILS/src/templates/opac/parts/searchbar.tt2 b/Open-ILS/src/templates/opac/parts/searchbar.tt2
index 0359c83cda..0e30974abc 100644
--- a/Open-ILS/src/templates/opac/parts/searchbar.tt2
+++ b/Open-ILS/src/templates/opac/parts/searchbar.tt2
@@ -58,9 +58,23 @@
     [% END %]
     [% IF is_advanced || is_special %]
     <div>
-        <input type="hidden" name="_adv" value="1" />
+        [% IF is_advanced %]
+            <input type="hidden" name="_adv" value="1" />
+        [% 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 %]
+                    
         [% IF ctx.processed_search_query OR (NOT is_advanced AND NOT is_special) %]
-        <input name='page' type='hidden' value="0" />
+            <input name='page' type='hidden' value="0" />
         [% END %]
     </div>
     [%- END %]