From 4ae5c35f51545dcb7e0b3f09bc9c4816c8355e80 Mon Sep 17 00:00:00 2001 From: Demian Katz <demian.katz@villanova.edu> Date: Fri, 6 Nov 2020 11:14:07 -0500 Subject: [PATCH] Add more record driver tests and improve code (#1785) - Add tests for SolrMarcRemote and SolrOverdrive - Includes general style cleanup/improvement - Fixes a bug (typo) in SolrOverdrive - Removes some unneeded SolrOverdrive methods (they were trying to work around MARC-specific logic that doesn't actually exist in SolrMarc) --- .../VuFind/RecordDriver/SolrMarcRemote.php | 7 +- .../src/VuFind/RecordDriver/SolrOverdrive.php | 211 ++++--------- .../RecordDriver/SolrMarcRemoteTest.php | 117 +++++++ .../RecordDriver/SolrOverdriveTest.php | 292 ++++++++++++++++++ 4 files changed, 478 insertions(+), 149 deletions(-) create mode 100644 module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrMarcRemoteTest.php create mode 100644 module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrOverdriveTest.php diff --git a/module/VuFind/src/VuFind/RecordDriver/SolrMarcRemote.php b/module/VuFind/src/VuFind/RecordDriver/SolrMarcRemote.php index b17dc642c33..0d4f0d67969 100644 --- a/module/VuFind/src/VuFind/RecordDriver/SolrMarcRemote.php +++ b/module/VuFind/src/VuFind/RecordDriver/SolrMarcRemote.php @@ -77,10 +77,9 @@ class SolrMarcRemote extends SolrMarc implements parent::__construct($mainConfig, $recordConfig, $searchSettings); // get config values for remote fullrecord service - if (! $mainConfig->Record->get('remote_marc_url')) { + $this->uriPattern = $mainConfig->Record->remote_marc_url ?? null; + if (!$this->uriPattern) { throw new \Exception('SolrMarcRemote baseUrl-setting missing.'); - } else { - $this->uriPattern = $mainConfig->Record->get('remote_marc_url'); } } @@ -96,7 +95,7 @@ class SolrMarcRemote extends SolrMarc implements // handle availability of fullrecord if (!isset($this->fields['fullrecord'])) { // retrieve fullrecord from external source - if (! isset($this->fields['id'])) { + if (!isset($this->fields['id'])) { throw new \Exception( 'No unique id given for fullrecord retrieval' ); diff --git a/module/VuFind/src/VuFind/RecordDriver/SolrOverdrive.php b/module/VuFind/src/VuFind/RecordDriver/SolrOverdrive.php index 4b173d78ade..790a90b89d9 100644 --- a/module/VuFind/src/VuFind/RecordDriver/SolrOverdrive.php +++ b/module/VuFind/src/VuFind/RecordDriver/SolrOverdrive.php @@ -79,8 +79,6 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface $this->connector = $connector; $this->config = $connector->getConfig(); parent::__construct($mainConfig, $recordConfig, null); - - $this->debug("SolrOverdrive Rec Driver constructed"); } /** @@ -118,13 +116,13 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface $od_id = $this->getOverdriveID(); if ($checkout = $this->connector->getCheckout($od_id, false)) { - //if we are already locked in, then we need free ones and locked in ones. + // If we're already locked in, then we need free ones and locked in ones. if ($checkout->isFormatLockedIn) { foreach ($checkout->formats as $format) { $formatType = $format->formatType; $formats[$formatType] = $formatNames[$formatType]; } - //if we aren't locked in, we can show all formats + // If we aren't locked in, we can show all formats } else { foreach ($this->getDigitalFormats() as $format) { $formats[$format->id] = $formatNames[$format->id]; @@ -146,7 +144,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface { $formats = []; $formatNames = $this->connector->getFormatNames(); - if ($this->config->isMarc) { + if ($this->getIsMarc()) { $od_id = $this->getOverdriveID(); $fulldata = $this->connector->getMetadata([$od_id]); $data = $fulldata[strtolower($od_id)]; @@ -246,7 +244,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function supportsAjaxStatus() { - //Future: add this as an overdrive configuration to turn it off + // TODO: add this as an Overdrive configuration to turn it off return true; } @@ -256,7 +254,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface * Pass-through to the connector to determine whether logged-in user * has access to Overdrive actions * - * @return boolean Whether the logged-in user has access to Overdrive. + * @return bool Whether the logged-in user has access to Overdrive. */ public function getOverdriveAccess() { @@ -268,7 +266,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface * * Returns whether the current user is logged in * - * @return object|boolean User if logged in, false if not. + * @return object|bool User if logged in, false if not. */ public function isLoggedIn() { @@ -289,9 +287,9 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface $result = 0; if ($this->config) { - if ($this->config->isMarc) { + if ($this->getIsMarc()) { $field = $this->config->idField; - $subfield = $this->confif->idSubfield; + $subfield = $this->config->idSubfield; $result = strtolower( $this->getFieldArray($field, $subfield)[0] ); @@ -344,7 +342,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface $result->data = false; } } - //if it didn't work, an error should be logged from the connector + // If it didn't work, an error should be logged from the connector return $result; } @@ -367,7 +365,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface } } } - //if it didn't work, an error should be logged from the connector + // If it didn't work, an error should be logged from the connector return false; } @@ -378,11 +376,8 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getBreadcrumb() { - if (!$this->getShortTitle()) { - return $this->getTitle(); - } else { - return $this->getShortTitle(); - } + $short = $this->getShortTitle(); + return $short ? $short : $this->getTitle(); } /** @@ -397,48 +392,33 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface { if ($this->getIsMarc()) { return parent::getMarcRecord(); - } else { - //return new fake marc class - return new class { - /** - * Get the field - * - * @param string $f Fieldname - * - * @return string - */ - public function getField($f) - { - return ""; - } - - /** - * Get the fields - * - * @param array $f Fieldnames - * - * @return array - */ - public function getFields($f) - { - return []; - } - }; } - } + // No MARC support? Return new fake MARC class. + return new class { + /** + * Get the field + * + * @param string $f Fieldname + * + * @return string + */ + public function getField($f) + { + return ""; + } - /** - * Get Subtitle - * - * @return string - */ - public function getSubtitle() - { - if ($this->getIsMarc()) { - return parent::getSubtitle(); - } else { - return $this->fields['title_sub']; - } + /** + * Get the fields + * + * @param array $f Fieldnames + * + * @return array + */ + public function getFields($f) + { + return []; + } + }; } /** @@ -448,26 +428,9 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getTitleSection() { - if ($this->getIsMarc()) { - return parent::getTitleSection(); - } else { - //I don't think overdrive has this metadata - return ""; - } - } - - /** - * Get Short Title - * - * @return string - */ - public function getShortTitle() - { - if ($this->getIsMarc()) { - return parent::getShortTitle(); - } else { - return $this->fields['title_short']; - } + return $this->getIsMarc() + ? parent::getTitleSection() + : ''; // I don't think Overdrive has this metadata } /** @@ -477,11 +440,7 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getGeneralNotes() { - if ($this->config->isMarc) { - return parent::getGeneralNotes(); - } else { - return []; - } + return $this->getIsMarc() ? parent::getGeneralNotes() : []; } /** @@ -497,36 +456,25 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface * @return string|array|bool * @throws \Exception */ - public function getThumbnail( - $size = 'small' - ) { - if ($size == 'large') { - $cover = "cover300Wide"; - } elseif ($size == 'medium') { - $cover = "cover150Wide"; - } elseif ($size == 'small') { - $cover = 'thumbnail'; - } else { - $cover = "cover"; - } + public function getThumbnail($size = 'small') + { + $coverMap = [ + 'large' => 'cover300Wide', + 'medium' => 'cover150Wide', + 'small' => 'thumbnail' + ]; + $cover = $coverMap[$size] ?? 'cover'; - //if the record is marc then the cover links probably aren't there. - if ($this->config->isMarc) { + // If the record is marc then the cover links probably aren't there. + if ($this->getIsMarc()) { $od_id = $this->getOverdriveID(); $fulldata = $this->connector->getMetadata([$od_id]); $data = $fulldata[strtolower($od_id)]; } else { - $result = false; $jsonData = $this->fields['fullrecord']; $data = json_decode($jsonData, false); } - - if (isset($data->images)) { - if (isset($data->images->{$cover})) { - $result = $data->images->{$cover}->href; - } - } - return $result; + return $data->images->{$cover}->href ?? false; } /** @@ -536,15 +484,15 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getSummary() { - if ($this->config->isMarc) { + if ($this->getIsMarc()) { return parent::getSummary(); - } else { - $desc = $this->fields["description"]; - - $newDesc = preg_replace("/’/i", "", $desc); - $newDesc = strip_tags($newDesc); - return ["Summary" => $newDesc]; } + // Non-MARC case: + $desc = $this->fields["description"] ?? ''; + + $newDesc = preg_replace("/’/i", "", $desc); + $newDesc = strip_tags($newDesc); + return ["Summary" => $newDesc]; } /** @@ -555,13 +503,9 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getRawData() { - if ($this->config->isMarc) { - return parent::getRawData(); - } else { - $jsonData = $this->fields['fullrecord']; - $data = json_decode($jsonData, true); - return $data; - } + return $this->getIsMarc() + ? parent::getRawData() + : json_decode($this->fields['fullrecord'], true); } /** @@ -591,35 +535,12 @@ class SolrOverdrive extends SolrMarc implements LoggerAwareInterface */ public function getAllSubjectHeadings($extended = false) { - if ($this->config) { - if ($this->config->isMarc) { - return parent::getAllSubjectHeadings($extended); - } else { - $headings = []; - foreach (['topic', 'geographic', 'genre', 'era'] as $field) { - if (isset($this->fields[$field])) { - $headings = array_merge( - $headings, $this->fields[$field] - ); - } - } - - // The default index schema doesn't currently store subject - // headings in a - // broken-down format, so we'll just send each value as a - // single chunk. - // Other record drivers (i.e. SolrMarc) can offer this data - // in a more granular format. - $callback = function ($i) use ($extended) { - return $extended - ? ['heading' => [$i], 'type' => '', 'source' => ''] - : [$i]; - }; - return array_map($callback, array_unique($headings)); - } - } else { + if (!$this->config) { return []; } + return $this->getIsMarc() + ? parent::getAllSubjectHeadings($extended) + : DefaultRecord::getAllSubjectHeadings($extended); } /** diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrMarcRemoteTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrMarcRemoteTest.php new file mode 100644 index 00000000000..ca7c8bf2349 --- /dev/null +++ b/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrMarcRemoteTest.php @@ -0,0 +1,117 @@ +<?php +/** + * SolrMarcRemote Record Driver Test Class + * + * PHP version 7 + * + * Copyright (C) Villanova University 2020. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * @category VuFind + * @package Tests + * @author Demian Katz <demian.katz@villanova.edu> + * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License + * @link https://vufind.org/wiki/development:testing:unit_tests Wiki + */ +namespace VuFindTest\RecordDriver; + +use Exception; +use Laminas\Config\Config; +use Laminas\Http\Response; +use VuFind\RecordDriver\SolrMarcRemote; +use VuFindHttp\HttpServiceInterface; + +/** + * SolrMarcRemote Record Driver Test Class + * + * @category VuFind + * @package Tests + * @author Demian Katz <demian.katz@villanova.edu> + * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License + * @link https://vufind.org/wiki/development:testing:unit_tests Wiki + */ +class SolrMarcRemoteTest extends \VuFindTest\Unit\TestCase +{ + use \VuFindTest\Unit\FixtureTrait; + + /** + * Test config validation. + * + * @return void + */ + public function testRequiredConfigException(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('SolrMarcRemote baseUrl-setting missing.'); + new SolrMarcRemote(); + } + + /** + * Test record ID validation (a record with no ID cannot be resolved). + * + * @return void + */ + public function testMissingRecordId(): void + { + $this->expectException(Exception::class); + $this->expectExceptionMessage('No unique id given for fullrecord retrieval'); + $this->getDriver()->getSummary(); + } + + /** + * Test actually retrieving a record. + * + * @return void + */ + public function testRecordRetrieval(): void + { + $driver = $this->getDriver(); + $driver->setHttpService($this->getMockHttpService()); + $driver->setRawData(['id' => 1]); + $this->assertEquals(4, count($driver->getTOC())); + } + + /** + * Get a SolrMarcRemote driver preconfigured to load a record. + * + * @return SolrMarcRemote + */ + protected function getDriver(): SolrMarcRemote + { + $url = 'http://foo/%s'; + $config = new Config(['Record' => ['remote_marc_url' => $url]]); + $driver = new SolrMarcRemote($config); + return $driver; + } + + /** + * Get a mock HttpService for testing + * + * @return HttpServiceInterface + */ + protected function getMockHttpService(): HttpServiceInterface + { + $marc = $this->getFixture('marc/toc2.xml'); + $response = Response::fromString( + "HTTP/1.1 200 OK\n\n" + . $marc + ); + $service = $this->getMockBuilder(HttpServiceInterface::class)->getMock(); + $service->expects($this->once())->method('get') + ->with($this->equalTo('http://foo/1')) + ->will($this->returnValue($response)); + return $service; + } +} diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrOverdriveTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrOverdriveTest.php new file mode 100644 index 00000000000..c7ca4fc5d3c --- /dev/null +++ b/module/VuFind/tests/unit-tests/src/VuFindTest/RecordDriver/SolrOverdriveTest.php @@ -0,0 +1,292 @@ +<?php +/** + * SolrOverdrive Record Driver Test Class + * + * PHP version 7 + * + * Copyright (C) Villanova University 2020. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * @category VuFind + * @package Tests + * @author Demian Katz <demian.katz@villanova.edu> + * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License + * @link https://vufind.org/wiki/development:testing:unit_tests Wiki + */ +namespace VuFindTest\RecordDriver; + +use Laminas\Config\Config; +use VuFind\DigitalContent\OverdriveConnector; +use VuFind\RecordDriver\SolrOverdrive; + +/** + * SolrOverdrive Record Driver Test Class + * + * @category VuFind + * @package Tests + * @author Demian Katz <demian.katz@villanova.edu> + * @license http://opensource.org/licenses/gpl-2.0.php GNU General Public License + * @link https://vufind.org/wiki/development:testing:unit_tests Wiki + */ +class SolrOverdriveTest extends \VuFindTest\Unit\TestCase +{ + use \VuFindTest\Unit\FixtureTrait; + + /** + * Test supportsOpenUrl() + * + * @return void + */ + public function testSupportsOpenUrl(): void + { + // Not supported: + $this->assertFalse($this->getDriver()->supportsOpenUrl()); + $this->assertFalse($this->getDriver()->supportsCoinsOpenUrl()); + } + + /** + * Test getOverdriveID in MARC mode + * + * @return void + */ + public function testGetOverdriveIDWithMarc(): void + { + $connector = $this->getMockConnector( + '{ "isMarc": true, "idField": "010", "idSubfield": "a" }' + ); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['fullrecord' => $this->getFixture('marc/marctraits.xml')] + ); + $this->assertEquals('lc123', $driver->getOverdriveID()); + } + + /** + * Test getOverdriveID in non-MARC mode + * + * @return void + */ + public function testGetOverdriveIDWithoutMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['id' => 'LC345'] + ); + $this->assertEquals('lc345', $driver->getOverdriveID()); + } + + /** + * Test getBreadcrumb() + * + * @return void + */ + public function testGetBreadcrumb(): void + { + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + // Confirm that we use short title when available, title otherwise: + $driver->setRawData(['title' => 'title : full', 'title_short' => 'title']); + $this->assertEquals('title', $driver->getBreadcrumb()); + $driver->setRawData(['title' => 'title : full']); + $this->assertEquals('title : full', $driver->getBreadcrumb()); + } + + /** + * Test getThumbnail without MARC + * + * @return void + */ + public function testGetThumbnailNoMarc(): void + { + $data = '{"images": {' + . '"cover300Wide": {"href": "http://300wide"},' + . '"cover150Wide": {"href": "http://150wide"},' + . '"thumbnail": {"href": "http://thumb"},' + . '"cover": {"href": "http://cover"}' + . '}}'; + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData(['fullrecord' => $data]); + $this->assertEquals('http://300wide', $driver->getThumbnail('large')); + $this->assertEquals('http://150wide', $driver->getThumbnail('medium')); + $this->assertEquals('http://thumb', $driver->getThumbnail('small')); + $this->assertEquals('http://cover', $driver->getThumbnail('foo')); + } + + /** + * Test getTitleSection() + * + * @return void + */ + public function testGetTitleSection(): void + { + $connector = $this->getMockConnector('{ "isMarc": true }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['fullrecord' => $this->getFixture('marc/marctraits.xml')] + ); + $this->assertEquals('2. Return', $driver->getTitleSection()); + } + + /** + * Test getGeneralNotes() + * + * @return void + */ + public function testGetGeneralNotes(): void + { + $connector = $this->getMockConnector('{ "isMarc": true }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['fullrecord' => $this->getFixture('marc/marctraits.xml')] + ); + $this->assertEquals( + ['General notes here.', 'Translation.'], $driver->getGeneralNotes() + ); + } + + /** + * Test getSummary() with MARC + * + * @return void + */ + public function testGetSummaryMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": true }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['fullrecord' => $this->getFixture('marc/marctraits.xml')] + ); + $this->assertEquals( + ['Summary.'], $driver->getSummary() + ); + } + + /** + * Test getSummary() without MARC + * + * @return void + */ + public function testGetSummaryNonMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['description' => '<tag>’’Summary.</tag>'] + ); + $this->assertEquals( + ['Summary.'], array_values($driver->getSummary()) + ); + } + + /** + * Test getAllSubjectHeadings() with MARC + * + * @return void + */ + public function testGetAllSubjectHeadingsMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": true }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['fullrecord' => $this->getFixture('marc/marctraits.xml')] + ); + $this->assertEquals( + [['Foo', 'test'], ['Bar', 'test again']], + $driver->getAllSubjectHeadings() + ); + } + + /** + * Test getAllSubjectHeadings() without MARC + * + * @return void + */ + public function testGetAllSubjectHeadingsNonMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + $driver->setRawData( + ['era' => ['foo'], 'genre' => ['bar']] + ); + $this->assertEquals( + [['bar'], ['foo']], $driver->getAllSubjectHeadings() + ); + } + + /** + * Test getRawData behavior in MARC mode + * + * @return void + */ + public function testGetRawDataMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": true }'); + $driver = $this->getDriver(null, null, $connector); + $raw = ['foo' => 'bar']; + $driver->setRawData($raw); + $this->assertEquals($raw, $driver->getRawData()); + } + + /** + * Test getRawData behavior in non-MARC mode + * + * @return void + */ + public function testGetRawDataNonMarc(): void + { + $connector = $this->getMockConnector('{ "isMarc": false }'); + $driver = $this->getDriver(null, null, $connector); + $raw = ['foo' => 'bar']; + $driver->setRawData(['fullrecord' => json_encode($raw)]); + $this->assertEquals($raw, $driver->getRawData()); + } + + /** + * Get a record driver to test with. + * + * @param Config $config Main configuration + * @param Config $recordConfig Record configuration + * @param OverdriveConnector $connector Overdrive connector + * + * @return SolrOverdrive + */ + protected function getDriver(Config $config = null, Config $recordConfig = null, + OverdriveConnector $connector = null + ): SolrOverdrive { + return new SolrOverdrive( + $config ?? new Config([]), + $recordConfig ?? new Config([]), + $connector ?? $this->getMockConnector() + ); + } + + /** + * Get a mock Overdrive connector. + * + * @param string $config JSON-formatted configuration + * + * @return OverdriveConnector + */ + protected function getMockConnector(string $config = '{}'): OverdriveConnector + { + $connector = $this->getMockBuilder(OverdriveConnector::class) + ->disableOriginalConstructor()->getMock(); + $connector->expects($this->any())->method('getConfig') + ->will($this->returnValue(json_decode($config))); + return $connector; + } +} -- GitLab