From 1327ee6ddd53afddb4fa9f2f453ecccedbfa757e Mon Sep 17 00:00:00 2001
From: Demian Katz <demian.katz@villanova.edu>
Date: Tue, 18 May 2021 06:56:07 -0500
Subject: [PATCH] Fix bug in loading custom notification schedule config.
 (#1957)

- Includes expanded test coverage.
---
 module/VuFind/src/VuFind/Search/History.php   | 15 ++++-
 .../src/VuFindTest/Search/HistoryTest.php     | 64 ++++++++++++++++---
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/module/VuFind/src/VuFind/Search/History.php b/module/VuFind/src/VuFind/Search/History.php
index f86850b081e..2e42930776c 100644
--- a/module/VuFind/src/VuFind/Search/History.php
+++ b/module/VuFind/src/VuFind/Search/History.php
@@ -28,6 +28,8 @@
  */
 namespace VuFind\Search;
 
+use Laminas\Config\Config;
+
 /**
  * VuFind Search History Helper
  *
@@ -133,10 +135,19 @@ class History
      */
     public function getScheduleOptions()
     {
+        // If scheduled searches are disabled, return no options:
         if (!($this->config->Account->schedule_searches ?? false)) {
             return [];
         }
-        return $this->config->Account->scheduled_search_frequencies
-            ?? [0 => 'schedule_none', 1 => 'schedule_daily', 7 => 'schedule_weekly'];
+        // If custom frequencies are not provided, return defaults:
+        if (!isset($this->config->Account->scheduled_search_frequencies)) {
+            return [
+                0 => 'schedule_none', 1 => 'schedule_daily', 7 => 'schedule_weekly'
+            ];
+        }
+        // If we have a setting, make sure it is properly formatted as an array:
+        return $this->config->Account->scheduled_search_frequencies instanceof Config
+            ? $this->config->Account->scheduled_search_frequencies->toArray()
+            : (array)$this->config->Account->scheduled_search_frequencies;
     }
 }
diff --git a/module/VuFind/tests/unit-tests/src/VuFindTest/Search/HistoryTest.php b/module/VuFind/tests/unit-tests/src/VuFindTest/Search/HistoryTest.php
index b0338b157cb..28245a39950 100644
--- a/module/VuFind/tests/unit-tests/src/VuFindTest/Search/HistoryTest.php
+++ b/module/VuFind/tests/unit-tests/src/VuFindTest/Search/HistoryTest.php
@@ -28,7 +28,9 @@
  */
 namespace VuFindTest\Search;
 
+use VuFind\Db\Table\Search as SearchTable;
 use VuFind\Search\History;
+use VuFind\Search\Results\PluginManager as ResultsManager;
 use VuFindTest\Unit\TestCase as TestCase;
 
 /**
@@ -48,7 +50,7 @@ class HistoryTest extends TestCase
      *
      * @return void
      */
-    public function testDefaultDisabledScheduleOptions()
+    public function testDefaultDisabledScheduleOptions(): void
     {
         $this->assertEquals([], $this->getHistory()->getScheduleOptions());
     }
@@ -59,7 +61,7 @@ class HistoryTest extends TestCase
      *
      * @return void
      */
-    public function testExplicitlyDisabledScheduleOptions()
+    public function testExplicitlyDisabledScheduleOptions(): void
     {
         $config = new \Laminas\Config\Config(
             [
@@ -78,7 +80,7 @@ class HistoryTest extends TestCase
      *
      * @return void
      */
-    public function testDefaultScheduleOptions()
+    public function testDefaultScheduleOptions(): void
     {
         $config = new \Laminas\Config\Config(
             [
@@ -94,12 +96,54 @@ class HistoryTest extends TestCase
         );
     }
 
+    /**
+     * Test a single non-default schedule option.
+     *
+     * @return void
+     */
+    public function testSingleNonDefaultScheduleOption(): void
+    {
+        $config = new \Laminas\Config\Config(
+            [
+                'Account' => [
+                    'schedule_searches' => true,
+                    'scheduled_search_frequencies' => 'Always'
+                ]
+            ]
+        );
+        $history = $this->getHistory(null, null, $config);
+        $this->assertEquals([0 => 'Always'], $history->getScheduleOptions());
+    }
+
+    /**
+     * Test multiple non-default schedule options.
+     *
+     * @return void
+     */
+    public function testMultipleNonDefaultScheduleOptions(): void
+    {
+        $config = new \Laminas\Config\Config(
+            [
+                'Account' => [
+                    'schedule_searches' => true,
+                    'scheduled_search_frequencies' => [
+                        1 => 'One', 2 => 'Two'
+                    ]
+                ]
+            ]
+        );
+        $history = $this->getHistory(null, null, $config);
+        $this->assertEquals(
+            [1 => 'One', 2 => 'Two'], $history->getScheduleOptions()
+        );
+    }
+
     /**
      * Test that purging history proxies to the right place.
      *
      * @return void
      */
-    public function testPurgeHistory()
+    public function testPurgeHistory(): void
     {
         $table = $this->getMockBuilder(\VuFind\Db\Table\Search::class)
             ->disableOriginalConstructor()->setMethods(['destroySession'])
@@ -113,15 +157,15 @@ class HistoryTest extends TestCase
     /**
      * Get object for testing.
      *
-     * @param \VuFind\Db\Table\Search              $searchTable    Search table
-     * @param \VuFind\Search\Results\PluginManager $resultsManager Results manager
-     * @param \Laminas\Config\Config                  $config         Configuration
+     * @param SearchTable            $searchTable    Search table
+     * @param ResultsManager         $resultsManager Results manager
+     * @param \Laminas\Config\Config $config         Configuration
      *
      * @return History
      */
-    protected function getHistory($searchTable = null,
-        $resultsManager = null, \Laminas\Config\Config $config = null
-    ) {
+    protected function getHistory(SearchTable $searchTable = null,
+        ResultsManager $resultsManager = null, \Laminas\Config\Config $config = null
+    ): History {
         return new History(
             $searchTable ?: $this->getMockBuilder(\VuFind\Db\Table\Search::class)
                 ->disableOriginalConstructor()->getMock(),
-- 
GitLab