From c4c0cb329dd5a7a095cdf0a37ef93c4aecca0e07 Mon Sep 17 00:00:00 2001
From: David Cook <dcook@prosentient.com.au>
Date: Wed, 18 Jun 2014 16:26:31 +1000
Subject: [PATCH] Bug 12443 - Initial re-factoring of buildQuery

This patch reduces three repeated code fragments into a single
internal subroutine, which is easier to read, has comments,
and should make it easier to refactor more buildQuery code
in the future.

_TEST PLAN_

Before applying

1) Run a bunch of different searches in the staff client and OPAC
in separate tabs

2) Apply the patch

3) Run the same searches again (maybe in yet more tabs) and notice
that the results are exactly the same.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Same results, no errors.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
---
 C4/Search.pm | 84 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 48 insertions(+), 36 deletions(-)

diff --git a/C4/Search.pm b/C4/Search.pm
index 748bb1ce3c..c94d92497c 100644
--- a/C4/Search.pm
+++ b/C4/Search.pm
@@ -1486,43 +1486,19 @@ sub buildQuery {
 
                 warn "FIELD WEIGHTED OPERAND: >$weighted_operand<" if $DEBUG;
 
-                # If there's a previous operand, we need to add an operator
-                if ($previous_operand) {
-
-                    # User-specified operator
-                    if ( $operators[ $i - 1 ] ) {
-                        $query     .= " $operators[$i-1] ";
-                        $query     .= " $index_plus " unless $indexes_set;
-                        $query     .= " $operand";
-                        $query_cgi .= "&op=".uri_escape($operators[$i-1]);
-                        $query_cgi .= "&idx=".uri_escape($index) if $index;
-                        $query_cgi .= "&q=".uri_escape($operands[$i]) if $operands[$i];
-                        $query_desc .=
-                          " $operators[$i-1] $index_plus $operands[$i]";
-                    }
-
-                    # Default operator is and
-                    else {
-                        $query      .= " and ";
-                        $query      .= "$index_plus " unless $indexes_set;
-                        $query      .= "$operand";
-                        $query_cgi  .= "&op=and&idx=".uri_escape($index) if $index;
-                        $query_cgi  .= "&q=".uri_escape($operands[$i]) if $operands[$i];
-                        $query_desc .= " and $index_plus $operands[$i]";
-                    }
-                }
-
-                # There isn't a pervious operand, don't need an operator
-                else {
+                ($query,$query_cgi,$query_desc,$previous_operand) = _build_initial_query({
+                    query => $query,
+                    query_cgi => $query_cgi,
+                    query_desc => $query_desc,
+                    operator => ($operators[ $i - 1 ]) ? $operators[ $i - 1 ] : '',
+                    parsed_operand => $operand,
+                    original_operand => ($operands[$i]) ? $operands[$i] : '',
+                    index => $index,
+                    index_plus => $index_plus,
+                    indexes_set => $indexes_set,
+                    previous_operand => $previous_operand,
+                });
 
-                    # Field-weighted queries already have indexes set
-                    $query .= " $index_plus " unless $indexes_set;
-                    $query .= $operand;
-                    $query_desc .= " $index_plus $operands[$i]";
-                    $query_cgi  .= "&idx=".uri_escape($index) if $index;
-                    $query_cgi  .= "&q=".uri_escape($operands[$i]) if $operands[$i];
-                    $previous_operand = 1;
-                }
             }    #/if $operands
         }    # /for
     }
@@ -1627,6 +1603,42 @@ sub buildQuery {
     );
 }
 
+=head2 _build_initial_query
+
+  ($query, $query_cgi, $query_desc, $previous_operand) = _build_initial_query($initial_query_params);
+
+  Build a section of the initial query containing indexes, operators, and operands.
+
+=cut
+
+sub _build_initial_query {
+    my ($params) = @_;
+
+    my $operator = "";
+    if ($params->{previous_operand}){
+        #If there is a previous operand, add a supplied operator or the default 'and'
+        $operator = ($params->{operator}) ? " ".($params->{operator})." " : ' and ';
+    }
+
+    #NOTE: indexes_set is typically set when doing truncation or field weighting
+    my $operand = ($params->{indexes_set}) ? $params->{parsed_operand} : $params->{index_plus}.$params->{parsed_operand};
+
+    #e.g. "kw,wrdl:test"
+    #e.g. " and kw,wrdl:test"
+    $params->{query} .= $operator . $operand;
+
+    $params->{query_cgi} .= "&op=".uri_escape($operator) if $operator;
+    $params->{query_cgi} .= "&idx=".uri_escape($params->{index}) if $params->{index};
+    $params->{query_cgi} .= "&q=".uri_escape($params->{original_operand}) if $params->{original_operand};
+
+    #e.g. " and kw,wrdl: test"
+    $params->{query_desc} .= $operator . $params->{index_plus} . " " . $params->{original_operand};
+
+    $params->{previous_operand} = 1 unless $params->{previous_operand}; #If there is no previous operand, mark this as one
+
+    return ($params->{query}, $params->{query_cgi}, $params->{query_desc}, $params->{previous_operand});
+}
+
 =head2 searchResults
 
   my @search_results = searchResults($search_context, $searchdesc, $hits, 
-- 
2.39.5