From 085ab000eab2834b5a6905ce3f7d768ab5279406 Mon Sep 17 00:00:00 2001 From: Liam Whalen Date: Sat, 28 Jun 2014 15:37:28 -0700 Subject: [PATCH] LP1084269, LP1050043 Expert Search Fixes 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
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 where the sort functionality is located. I also removed the use of the "_adv" hidden variable from the 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 Signed-off-by: Sarah Childs Signed-off-by: Ben Shum --- .../lib/OpenILS/Application/Search/Biblio.pm | 13 ++++---- .../Application/Storage/Publisher/metabib.pm | 31 ++++++++++++------- .../lib/OpenILS/WWW/EGCatLoader/Search.pm | 24 ++++++++++++-- .../src/templates/opac/parts/result/table.tt2 | 2 +- .../src/templates/opac/parts/searchbar.tt2 | 11 +++++++ 5 files changed, 59 insertions(+), 22 deletions(-) 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 15685b0af1..22c0966966 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,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 = []; } 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 0fe1a3495c..fad6ef306a 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 @@ -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 { 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 3a69fb1937..19f3a258e9 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Search.pm @@ -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; diff --git a/Open-ILS/src/templates/opac/parts/result/table.tt2 b/Open-ILS/src/templates/opac/parts/result/table.tt2 index 25514e4188..eac0bc547e 100644 --- a/Open-ILS/src/templates/opac/parts/result/table.tt2 +++ b/Open-ILS/src/templates/opac/parts/result/table.tt2 @@ -364,7 +364,7 @@ END;
mkurl()}, ['query','tag','subfield','term','_special','sort','page']) %]" [% html_text_attr('title', l('Place Hold on [_1]', attrs.title)) %] class="no-dec"> [% END %] + [% IF is_special %] + [% + number_of_expert_rows = CGI.param('tag').list.size; + index = 0; + WHILE index < number_of_expert_rows %] + + + + [% index = index + 1; %] + [% END %] + [% END %]
[%- END %] [% UNLESS took_care_of_form %][% END %] -- 2.43.2