From afd3e5091c6a0e88f3a24712a57f127416bb95dc Mon Sep 17 00:00:00 2001
From: Demian Katz <demian.katz@villanova.edu>
Date: Tue, 5 Jan 2016 10:29:16 -0500
Subject: [PATCH] Primo Central improvements - Changed the Primo ini file to
 contain a proper URL of the server instead of just an API ID and a port and
 added a configuration update step for it. - Made the Primo Connector throw a
 proper exception by default if it fails to query the server. - Removed the
 useless returnErr parameter from Primo connector. - Adjusted config upgrader
 to be more tolerant of missing config.ini (useful during testing).

---
 config/vufind/Primo.ini                       |  15 +-
 module/VuFind/src/VuFind/Config/Upgrade.php   |  33 +++-
 .../Search/Factory/PrimoBackendFactory.php    |  13 +-
 .../tests/fixtures/configs/primo/Primo.ini    | 179 ++++++++++++++++++
 .../src/VuFindTest/Config/UpgradeTest.php     |  17 ++
 .../VuFindSearch/Backend/Primo/Connector.php  |  61 +++---
 .../VuFindTest/Backend/Primo/BackendTest.php  |   2 +-
 .../Backend/Primo/ConnectorTest.php           |   4 +-
 8 files changed, 278 insertions(+), 46 deletions(-)
 create mode 100644 module/VuFind/tests/fixtures/configs/primo/Primo.ini

diff --git a/config/vufind/Primo.ini b/config/vufind/Primo.ini
index 1556289c996..0e59b01e287 100644
--- a/config/vufind/Primo.ini
+++ b/config/vufind/Primo.ini
@@ -15,12 +15,12 @@ case_sensitive_bools = true
 ; HTTP timeout
 timeout = 30
 
-; Your API id (use "mlplus" if using Metalib Plus instead of Primo Central)
-apiId = my-id
-
-; The port to use when connecting to the API (normally 1701 for Primo Central, 80
-; for Metalib Plus)
-port = 1701
+; URL of the Primo server. For hosted services typically of the form 
+; http://XYZ.hosted.exlibrisgroup.com:1701 where XYZ is the ID of the Primo 
+; instance (e.g. primoapac01). Use http://mlplus.hosted.exlibrisgroup.com for 
+; MetaLib Plus.
+; You may also enter the full URL including a path to the brief search if necessary. 
+url = "http://XYZ.hosted.exlibrisgroup.com:1701"
 
 ; This section controls the result limit options for search results. default_limit
 ; sets the default number of results per page. limit_options is a comma-separated
@@ -38,7 +38,8 @@ default_limit        = 20
 ;default_top_recommend[] = TopFacets:FacetsTop:Primo
 default_side_recommend[] = SideFacets:Facets:CheckboxFacets:Primo
 
-; This section is used to set general parameters for URL generation to make a call to the Primo API
+; This section is used to set general parameters for URL generation to make a call to
+; the Primo API
 bulkSize = 20
 
 ; When you filter a search using facets, VuFind will present a checkbox that can
diff --git a/module/VuFind/src/VuFind/Config/Upgrade.php b/module/VuFind/src/VuFind/Config/Upgrade.php
index e778de4cdcd..445cb3f6295 100644
--- a/module/VuFind/src/VuFind/Config/Upgrade.php
+++ b/module/VuFind/src/VuFind/Config/Upgrade.php
@@ -237,7 +237,8 @@ class Upgrade
     protected function loadOldBaseConfig()
     {
         // Load the base settings:
-        $mainArray = parse_ini_file($this->oldDir . '/config.ini', true);
+        $oldIni = $this->oldDir . '/config.ini';
+        $mainArray = file_exists($oldIni) ? parse_ini_file($oldIni, true) : [];
 
         // Merge in local overrides as needed.  VuFind 2 structures configurations
         // differently, so people who used this mechanism will need to refactor
@@ -1016,6 +1017,9 @@ class Upgrade
         // update permission settings
         $this->upgradePrimoPermissions();
 
+        // update server settings
+        $this->upgradePrimoServerSettings();
+
         // save the file
         $this->saveModifiedConfig('Primo.ini');
     }
@@ -1072,6 +1076,33 @@ class Upgrade
         }
     }
 
+    /**
+     * Translate obsolete server settings.
+     *
+     * @return void
+     */
+    protected function upgradePrimoServerSettings()
+    {
+        $config = & $this->newConfigs['Primo.ini'];
+        $permissions = & $this->newConfigs['permissions.ini'];
+        // Convert apiId to url
+        if (isset($config['General']['apiId'])) {
+            $url = 'http://' . $config['General']['apiId']
+                . '.hosted.exlibrisgroup.com';
+            if (isset($config['General']['port'])) {
+                $url .= ':' . $config['General']['port'];
+            } else {
+                $url .= ':1701';
+            }
+
+            $config['General']['url'] = $url;
+
+            // Remove any old settings remaining in Primo.ini:
+            unset($config['General']['apiId']);
+            unset($config['General']['port']);
+        }
+    }
+
     /**
      * Upgrade WorldCat.ini.
      *
diff --git a/module/VuFind/src/VuFind/Search/Factory/PrimoBackendFactory.php b/module/VuFind/src/VuFind/Search/Factory/PrimoBackendFactory.php
index e00f84de8d7..965830c6008 100644
--- a/module/VuFind/src/VuFind/Search/Factory/PrimoBackendFactory.php
+++ b/module/VuFind/src/VuFind/Search/Factory/PrimoBackendFactory.php
@@ -135,11 +135,10 @@ class PrimoBackendFactory implements FactoryInterface
         // Get the PermissionHandler
         $permHandler = $this->getPermissionHandler();
 
-        // Load credentials and port number:
-        $id = isset($this->primoConfig->General->apiId)
-            ? $this->primoConfig->General->apiId : null;
-        $port = isset($this->primoConfig->General->port)
-            ? $this->primoConfig->General->port : 1701;
+        // Load url and credentials:
+        if (!isset($this->primoConfig->General->url)) {
+            throw new \Exception('Missing url in Primo.ini');
+        }
         $instCode = isset($permHandler)
             ? $permHandler->getInstCode()
             : null;
@@ -150,7 +149,9 @@ class PrimoBackendFactory implements FactoryInterface
             ? $this->primoConfig->General->timeout : 30;
         $client->setOptions(['timeout' => $timeout]);
 
-        $connector = new Connector($id, $instCode, $client, $port);
+        $connector = new Connector(
+            $this->primoConfig->General->url, $instCode, $client
+        );
         $connector->setLogger($this->logger);
         return $connector;
     }
diff --git a/module/VuFind/tests/fixtures/configs/primo/Primo.ini b/module/VuFind/tests/fixtures/configs/primo/Primo.ini
new file mode 100644
index 00000000000..e113beea35e
--- /dev/null
+++ b/module/VuFind/tests/fixtures/configs/primo/Primo.ini
@@ -0,0 +1,179 @@
+; This section contains global settings affecting search behavior.
+; For PrimoCentral API documentation:
+; http://www.exlibrisgroup.org/display/PrimoOI/Brief+Search
+; Note: you will need your EL Commons credentials to access documentation
+[General]
+; This setting controls the default sort order of search results; the selected
+; option should be one of the options present in the [Sorting] section below.
+default_sort         = relevance
+
+; If this setting is true, boolean operators in searches (AND/OR/NOT) will only
+; be recognized if they are ALL UPPERCASE.  If set to false, they will be
+; recognized regardless of case.
+case_sensitive_bools = true
+
+; HTTP timeout
+timeout = 30
+
+; Your API id (use "mlplus" if using Metalib Plus instead of Primo Central)
+apiId = my-id
+
+; The port to use when connecting to the API (normally 1701 for Primo Central, 80
+; for Metalib Plus)
+port = 1701
+
+; This section controls the result limit options for search results. default_limit
+; sets the default number of results per page. limit_options is a comma-separated
+; list of numbers to be presented to the end-user. If only one limit is required,
+; set default_limit and leave limit_options commented out.
+; WARNING: using large limits may require you to raise your PHP memory limits to
+; avoid errors.
+default_limit        = 20
+;limit_options        = 10,20,40,60,80,100
+
+; These are the default recommendations modules to use when no specific setting
+; are found in the [TopRecommendations] or [SideRecommendations] sections below.
+; See the comments above those sections for details on legal settings.  You may
+; repeat these lines to load multiple recommendations.
+;default_top_recommend[] = TopFacets:FacetsTop:Primo
+default_side_recommend[] = SideFacets:Facets:CheckboxFacets:Primo
+
+; This section is used to set general parameters for URL generation to make a call to the Primo API
+bulkSize = 20
+
+; When you filter a search using facets, VuFind will present a checkbox that can
+; be used to apply those filters to the next search you perform.  This setting
+; controls its default state: on (true) or off (false).
+retain_filters_by_default = true
+
+; The filters listed below will be applied to all new searches by default. Omit
+; this setting to have no default filters applied. These differ from hidden
+; filters because they are visible in the UI and may be removed by the user.
+;default_filters[] = "rtype:Books"
+
+; Primo has a fixed cap on how many results you can page through.  Even though
+; it may report more results than this number, you can't actually access results
+; past the limit.  This setting tells VuFind where to cut off its paging mechanism.
+result_limit = 2000
+
+; The following two sections can be used to associate specific recommendations
+; modules with specific search types defined in the [Basic_Searches] section
+; below.  For all the details on how these sections work, see the comments above
+; the equivalent sections of searches.ini.  Recommendations work the same in
+; Primo as they do in the regular Search module.
+[SideRecommendations]
+; No search-specific settings by default -- add your own here.
+;[TopRecommendations]
+; No search-specific settings by default -- add your own here.
+
+; This section is used to identify facets for special treatment by the SideFacets
+; recommendations module.
+[SpecialFacets]
+; Any fields listed below will be treated as date ranges rather than plain facets:
+dateRange[] = creationdate
+
+; The order of display is as shown below
+; The name of the index field is on the left
+; The display name of the field is on the right
+[Facets]
+tlevel  = "Limit To"
+rtype   = "Format"
+creator = "Author"
+jtitle  = "Journal Title"
+topic   = Subjects
+creationdate = "Year of Publication"
+domain  = Source
+;lang    = Language
+
+; Top facets (not used by default)
+[FacetsTop]
+
+; Checkbox facets are facets, which are getting displayed as checkboxes
+; pcAvailability is a special facet. It's used to show all records found in
+; PrimoCentral without checking the local availability against a holdingsfile.
+[CheckboxFacets]
+;pcAvailability = "add_other_libraries"       ; pcAvailability is special; see above
+
+; Facet display settings
+[Results_Settings]
+; Rows and columns for table used by top facets
+top_rows = 2
+top_cols = 3
+; Do we want any facets to be collapsed by default?
+;collapsedFacets = *
+
+; These settings affect the way the facets are displayed
+[Facet_Settings]
+facet_limit        = 30     ; how many values should we show for each facet?
+
+; These settings affect the way facets are displayed on the advanced screen
+[Advanced_Facet_Settings]
+; Some special facets for advanced searching can be turned on by inclusion in
+; the comma-separated list below, or turned off by being excluded.  Currently,
+; just one values is supported: "daterange" for the publication year range
+; control.
+special_facets      = daterange
+
+; Any facets named in the list below will have their values run through the
+; translation code; unlisted facets will displayed as-is without translation. For
+; translated facets, be sure that all of the necessary strings are included in the
+; language files found in the languages directory. You may add a colon and the
+; name of a text domain after the field name to specify translations in a specific
+; text domain (subdirectory of the languages folder). By default, no facets are
+; translated -- uncomment or add lines below to turn on this feature.
+translated_facets[] = tlevel
+
+; This section shows which search types will display in the basic search box at
+; the top of Primo pages.  The name of each setting below corresponds with an
+; index defined in the Primo API.  The value of each setting is the text to
+; display on screen.  All on-screen text will be run through the translator, so
+; be sure to update language files if necessary.  The order of these settings
+; will be maintained in the drop-down list in the UI.
+[Basic_Searches]
+AllFields           = Keyword
+Title               = Title
+Author              = Author
+Subject             = Subject
+;Abstract            = Abstract
+;ISSN                = ISSN
+
+; This section defines which search options will be included on the advanced
+; search screen.  All the notes above [Basic_Searches] also apply here.
+[Advanced_Searches]
+AllFields           = Keyword
+Title               = Title
+Author              = Author
+Subject             = Subject
+Abstract            = Abstract
+ISSN                = ISSN
+
+; This section controls the operators displayed on the advanced search screen;
+; the left side contains the actual operator names, while the right side
+; contains display text that will be run through the translator logic.
+[Advanced_Operators]
+contains            = operator_contains
+exact               = operator_exact
+
+; This section defines the sort options available on Primo search results.
+; PrimoCentral sorting can only go in one direction - either ASC for ascending
+; or DESC for descending. In order to change the sort direction you must go into
+; the primo backend in Primo Home > Advanced Configuration > All Mapping Tables
+; Sort Fields Config and make your changes there.
+[Sorting]
+relevance = sort_relevance
+scdate = sort_year
+screator = sort_author
+stitle = sort_title
+
+; You can set regular expression / institution code pairs here to match up
+; IP addresses with institution codes.
+[Institutions]
+; Consortial example:
+regex[] = "/^1\.2\..*/"    ; match IPs starting with 1.2.
+code[] = "MEMBER1"
+regex[] = "/^2\.3\..*/"    ; match IPs starting with 2.3.
+code[] = "MEMBER2"
+regex[] = "/^3\.4\..*/"    ; match IPs starting with 3.4.
+code[] = "MEMBER2"
+regex[] = "/.*/"           ; default if no previous rule was matched
+code[] = "DEFAULT"
diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Config/UpgradeTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Config/UpgradeTest.php
index 2714b629aa7..5227d803f8b 100644
--- a/module/VuFind/tests/unit-tests/src/VuFindTest/Config/UpgradeTest.php
+++ b/module/VuFind/tests/unit-tests/src/VuFindTest/Config/UpgradeTest.php
@@ -444,4 +444,21 @@ class UpgradeTest extends \VuFindTest\Unit\TestCase
             )
         );
     }
+
+    /**
+     * Test Primo upgrade.
+     *
+     * @return void
+     */
+    public function testPrimoUpgrade()
+    {
+        $upgrader = $this->getUpgrader('primo');
+        $upgrader->run();
+        $this->assertEquals([], $upgrader->getWarnings());
+        $results = $upgrader->getNewConfigs();
+        $this->assertEquals(
+            'http://my-id.hosted.exlibrisgroup.com:1701',
+            $results['Primo.ini']['General']['url']
+        );
+    }
 }
\ No newline at end of file
diff --git a/module/VuFindSearch/src/VuFindSearch/Backend/Primo/Connector.php b/module/VuFindSearch/src/VuFindSearch/Backend/Primo/Connector.php
index a22a36b677c..2482d484a95 100644
--- a/module/VuFindSearch/src/VuFindSearch/Backend/Primo/Connector.php
+++ b/module/VuFindSearch/src/VuFindSearch/Backend/Primo/Connector.php
@@ -26,6 +26,7 @@
  * @author   Anna Headley <aheadle1@swarthmore.edu>
  * @author   Chelsea Lobdell <clobdel1@swarthmore.edu>
  * @author   Demian Katz <demian.katz@villanova.edu>
+ * @author   Ere Maijala <ere.maijala@helsinki.fi>
  * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
  * @link     http://vufind.org
  */
@@ -41,6 +42,7 @@ use Zend\Http\Client as HttpClient;
  * @author   Anna Headley <aheadle1@swarthmore.edu>
  * @author   Chelsea Lobdell <clobdel1@swarthmore.edu>
  * @author   Demian Katz <demian.katz@villanova.edu>
+ * @author   Ere Maijala <ere.maijala@helsinki.fi>
  * @license  http://opensource.org/licenses/gpl-2.0.php GNU General Public License
  * @link     http://vufind.org
  */
@@ -69,6 +71,18 @@ class Connector implements \Zend\Log\LoggerAwareInterface
      */
     protected $host;
 
+    /**
+     * Response for an empty search
+     *
+     * @var array
+     */
+    protected static $emptyQueryResponse = [
+        'recordCount' => 0,
+        'documents' => [],
+        'facets' => [],
+        'error' => 'Primo does not accept an empty query'
+    ];
+
     /**
      * Debug status
      *
@@ -81,21 +95,27 @@ class Connector implements \Zend\Log\LoggerAwareInterface
      *
      * Sets up the Primo API Client
      *
-     * @param string     $apiId  Primo API ID
+     * @param string     $url    Primo API URL (either a host name and port or a full
+     * path to the brief search including a trailing question mark)
      * @param string     $inst   Institution code
      * @param HttpClient $client HTTP client
-     * @param int        $port   API connection port
      */
-    public function __construct($apiId, $inst, $client, $port = 1701)
+    public function __construct($url, $inst, $client)
     {
-        $this->host = "http://$apiId.hosted.exlibrisgroup.com:{$port}/"
-            . "PrimoWebServices/xservice/search/brief?";
+        $parts = parse_url($url);
+        if (empty($parts['path']) || $parts['path'] == '/') {
+            $parts['path'] = '/PrimoWebServices/xservice/search/brief';
+        }
+        $this->host = $parts['scheme'] . '://' . $parts['host']
+            . (!empty($parts['port']) ? ':' . $parts['port'] : '')
+            . $parts['path'] . '?';
+
         $this->inst = $inst;
         $this->client = $client;
     }
 
     /**
-     * Execute a search.  adds all the querystring parameters into
+     * Execute a search. Adds all the querystring parameters into
      * $this->client and returns the parsed response
      *
      * @param string $institution Institution
@@ -110,8 +130,6 @@ class Connector implements \Zend\Log\LoggerAwareInterface
      *     pageNumber  string: index of first record (default 1)
      *     limit       string: number of records to return (default 20)
      *     sort        string: value to be used by for sorting (default null)
-     *     returnErr   bool:   false to fail on error; true to return empty
-     *                         empty result set with an error field (def true)
      *     Anything in $params not listed here will be ignored.
      *
      * Note: some input parameters accepted by Primo are not implemented here:
@@ -137,29 +155,13 @@ class Connector implements \Zend\Log\LoggerAwareInterface
             "pcAvailability" => false,
             "pageNumber" => 1,
             "limit" => 20,
-            "sort" => null,
-            "returnErr" => true,
+            "sort" => null
         ];
         if (isset($params)) {
             $args = array_merge($args, $params);
         }
 
-        // run search, deal with exceptions
-        try {
-            $result = $this->performSearch($institution, $terms, $args);
-        } catch (\Exception $e) {
-            if ($args["returnErr"]) {
-                $this->debug($e->getMessage());
-                return [
-                    'recordCount' => 0,
-                    'documents' => [],
-                    'facets' => [],
-                    'error' => $e->getMessage()
-                ];
-            } else {
-                throw $e;
-            }
-        }
+        $result = $this->performSearch($institution, $terms, $args);
         return $result;
     }
 
@@ -330,8 +332,9 @@ class Connector implements \Zend\Log\LoggerAwareInterface
             // Send Request
             $result = $this->call(implode('&', $qs));
         } else {
-            throw new \Exception('Primo API does not accept a null query');
+            return self::$emptyQueryResponse;
         }
+
         return $result;
     }
 
@@ -621,7 +624,7 @@ class Connector implements \Zend\Log\LoggerAwareInterface
             // Send Request
             $result = $this->call(implode('&', $qs));
         } else {
-            throw new \Exception('Primo API does not accept a null query');
+            return self::$emptyQueryResponse;
         }
 
         return $result;
@@ -658,7 +661,7 @@ class Connector implements \Zend\Log\LoggerAwareInterface
             // Send Request
             $result = $this->call(implode('&', $qs));
         } else {
-            throw new \Exception('Primo API does not accept a null query');
+            return self::$emptyQueryResponse;
         }
 
         return $result;
diff --git a/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/BackendTest.php b/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/BackendTest.php
index 4c57ff0eb58..ee0f9aa13bd 100644
--- a/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/BackendTest.php
+++ b/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/BackendTest.php
@@ -212,7 +212,7 @@ class BackendTest extends \VuFindTest\Unit\TestCase
         $client = $this->getMock('Zend\Http\Client');
         return $this->getMock(
             'VuFindSearch\Backend\Primo\Connector', $mock,
-            ['api-id', 'inst-id', $client]
+            ['http://fakeaddress.none', 'inst-id', $client]
         );
     }
 }
diff --git a/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/ConnectorTest.php b/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/ConnectorTest.php
index 316ca772b5d..532f2e358c9 100644
--- a/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/ConnectorTest.php
+++ b/module/VuFindSearch/tests/unit-tests/src/VuFindTest/Backend/Primo/ConnectorTest.php
@@ -81,7 +81,7 @@ class ConnectorTest extends PHPUnit_Framework_TestCase
         $terms = [];
         $result = $conn->query('dummyinst', $terms);
         $this->assertEquals(0, $result['recordCount']);
-        $this->assertEquals('Primo API does not accept a null query', $result['error']);
+        $this->assertEquals('Primo does not accept an empty query', $result['error']);
     }
 
     /**
@@ -159,7 +159,7 @@ class ConnectorTest extends PHPUnit_Framework_TestCase
         }
         $client = new HttpClient();
         $client->setAdapter($adapter);
-        $conn = new Connector('fakeid', 'fakeinst', $client);
+        $conn = new Connector('http://fakeaddress.none', 'fakeinst', $client);
         return $conn;
     }
 }
\ No newline at end of file
-- 
GitLab