From 71d943ffe95b6ed2224927f1ae94c3053de19c8d Mon Sep 17 00:00:00 2001 From: Chris Hallberg <crhallberg@gmail.com> Date: Tue, 25 Oct 2016 12:19:40 -0400 Subject: [PATCH] Simplify block behavior and improve account block feedback. (#815) - Move block checking to higher-level code for simplicity/consistency - Expose public method for blocking requests so message can be displayed to user - Show account blocks on profile/checkedout screens. --- config/vufind/Demo.ini | 5 +- languages/en.ini | 1 + .../Controller/MyResearchController.php | 23 +++ module/VuFind/src/VuFind/ILS/Driver/Demo.php | 42 +++-- .../src/VuFind/ILS/Driver/VoyagerRestful.php | 32 +++- module/VuFind/src/VuFind/ILS/Logic/Holds.php | 150 ++++++------------ .../templates/RecordTab/holdingsils.phtml | 26 +-- .../templates/RecordTab/holdingsils.phtml | 12 +- 8 files changed, 146 insertions(+), 145 deletions(-) diff --git a/config/vufind/Demo.ini b/config/vufind/Demo.ini index 5669413475f..6cb15e25585 100644 --- a/config/vufind/Demo.ini +++ b/config/vufind/Demo.ini @@ -60,16 +60,15 @@ cancelHolds = 50 cancelILLRequests = 50 cancelStorageRetrievalRequests = 50 changePassword = 33 -checkILLRequestBlock = 10 checkILLRequestIsValid = 10 checkIntermittentFailure = 0 ; chance of simulating low-level system failure checkRenewBlock = 25 -checkRequestBlock = 10 checkRequestIsValid = 10 -checkStorageRetrievalRequestBlock = 10 checkStorageRetrievalRequestIsValid = 10 +getAccountBlocks = 10 getDefaultRequestGroup = 50 getHoldDefaultRequiredDate = 50 +getRequestBlocks = 10 placeHold = 50 placeILLRequest = 50 placeStorageRetrievalRequest = 50 diff --git a/languages/en.ini b/languages/en.ini index 829b42c7c1f..ad8cc6c0d8d 100644 --- a/languages/en.ini +++ b/languages/en.ini @@ -6,6 +6,7 @@ Access URL = "Access URL" access_denied = "Access denied." Accession Number = "Accession Number" Account = "Account" +account_block_options_missing = "Some choices have been removed due to a block on your account. Details: %%details%%" Add = "Add" Add a Library Card = "Add a Library Card" Add a Note = "Add a Note" diff --git a/module/VuFind/src/VuFind/Controller/MyResearchController.php b/module/VuFind/src/VuFind/Controller/MyResearchController.php index 8da9e57df86..e14d1a078de 100644 --- a/module/VuFind/src/VuFind/Controller/MyResearchController.php +++ b/module/VuFind/src/VuFind/Controller/MyResearchController.php @@ -371,6 +371,7 @@ class MyResearchController extends AbstractBase // Obtain user information from ILS: $catalog = $this->getILS(); + $this->addAccountBlocksToFlashMessenger($catalog, $patron); $profile = $catalog->getMyProfile($patron); $profile['home_library'] = $user->home_library; $view->profile = $profile; @@ -386,6 +387,25 @@ class MyResearchController extends AbstractBase return $view; } + /** + * Add account blocks to the flash messenger as errors. + * + * @param \VuFind\ILS\Connection $catalog Catalog connection + * @param array $patron Patron details + * + * @return void + */ + public function addAccountBlocksToFlashMessenger($catalog, $patron) + { + if ($catalog->checkCapability('getAccountBlocks', compact($patron)) + && $blocks = $catalog->getAccountBlocks($patron) + ) { + foreach ($blocks as $block) { + $this->flashMessenger()->addMessage($block, 'error'); + } + } + } + /** * Catalog Login Action * @@ -1078,6 +1098,9 @@ class MyResearchController extends AbstractBase // Connect to the ILS: $catalog = $this->getILS(); + // Display account blocks, if any: + $this->addAccountBlocksToFlashMessenger($catalog, $patron); + // Get the current renewal status and process renewal form, if necessary: $renewStatus = $catalog->checkFunction('Renewals', compact('patron')); $renewResult = $renewStatus diff --git a/module/VuFind/src/VuFind/ILS/Driver/Demo.php b/module/VuFind/src/VuFind/ILS/Driver/Demo.php index 6e0695a24aa..3da19ce3a25 100644 --- a/module/VuFind/src/VuFind/ILS/Driver/Demo.php +++ b/module/VuFind/src/VuFind/ILS/Driver/Demo.php @@ -316,33 +316,31 @@ class Demo extends AbstractBase } /** - * Are holds/recalls blocked? + * Check whether the patron is blocked from placing requests (holds/ILL/SRR). * - * @return bool - */ - protected function checkRequestBlock() - { - return $this->isFailing(__METHOD__, 10); - } - - /** - * Are ILL requests blocked? + * @param array $patron Patron data from patronLogin(). * - * @return bool + * @return mixed A boolean false if no blocks are in place and an array + * of block reasons if blocks are in place */ - protected function checkILLRequestBlock() + public function getRequestBlocks($patron) { - return $this->isFailing(__METHOD__, 10); + return $this->isFailing(__METHOD__, 10) + ? ['simulated request block'] : false; } /** - * Are storage retrieval requests blocked? + * Check whether the patron has any blocks on their account. * - * @return bool + * @param array $patron Patron data from patronLogin(). + * + * @return mixed A boolean false if no blocks are in place and an array + * of block reasons if blocks are in place */ - protected function checkStorageRetrievalRequestBlock() + public function getAccountBlocks($patron) { - return $this->isFailing(__METHOD__, 10); + return $this->isFailing(__METHOD__, 10) + ? ['simulated account block'] : false; } /** @@ -373,16 +371,12 @@ class Demo extends AbstractBase 'callnumber' => $this->getFakeCallNum(), 'duedate' => '', 'is_holdable' => true, - 'addLink' => $patron - ? $this->checkRequestBlock() ? 'block' : true : false, + 'addLink' => $patron ? true : false, 'level' => 'copy', 'storageRetrievalRequest' => 'auto', - 'addStorageRetrievalRequestLink' => $patron - ? $this->checkStorageRetrievalRequestBlock() ? 'block' : 'check' - : false, + 'addStorageRetrievalRequestLink' => $patron ? 'check' : false, 'ILLRequest' => 'auto', - 'addILLRequestLink' => $patron - ? $this->checkILLRequestBlock() ? 'block' : 'check' : false, + 'addILLRequestLink' => $patron ? 'check' : false, 'services' => $status == 'Available' ? $this->getFakeServices() : [] ]; } diff --git a/module/VuFind/src/VuFind/ILS/Driver/VoyagerRestful.php b/module/VuFind/src/VuFind/ILS/Driver/VoyagerRestful.php index 7dca01de977..6f31bc649a3 100644 --- a/module/VuFind/src/VuFind/ILS/Driver/VoyagerRestful.php +++ b/module/VuFind/src/VuFind/ILS/Driver/VoyagerRestful.php @@ -590,7 +590,7 @@ class VoyagerRestful extends Voyager implements \VuFindHttp\HttpServiceAwareInte if ('driver' == $mode && 'auto' == $holdType) { $itemID = isset($data['item_id']) ? $data['item_id'] : false; $result = $this->determineHoldType($patron['id'], $id, $itemID); - if (!$result || $result == 'block') { + if (!$result) { return false; } } @@ -1187,6 +1187,32 @@ class VoyagerRestful extends Voyager implements \VuFindHttp\HttpServiceAwareInte return $blockReason; } + /** + * Check whether the patron is blocked from placing requests (holds/ILL/SRR). + * + * @param array $patron Patron data from patronLogin(). + * + * @return mixed A boolean false if no blocks are in place and an array + * of block reasons if blocks are in place + */ + public function getRequestBlocks($patron) + { + return $this->checkAccountBlocks($patron['id']); + } + + /** + * Check whether the patron has any blocks on their account. + * + * @param array $patron Patron data from patronLogin(). + * + * @return mixed A boolean false if no blocks are in place and an array + * of block reasons if blocks are in place + */ + public function getAccountBlocks($patron) + { + return $this->checkAccountBlocks($patron['id']); + } + /** * Check Account Blocks * @@ -1576,7 +1602,7 @@ EOT; // Check for account Blocks if ($this->checkAccountBlocks($patronId)) { - return 'block'; + return false; } // Check Recalls First @@ -1932,7 +1958,7 @@ EOT; // Let's determine Hold Type now if ($type == 'auto') { $type = $this->determineHoldType($patron['id'], $bibId, $itemId); - if (!$type || $type == 'block') { + if (!$type) { return $this->holdError('hold_error_blocked'); } } diff --git a/module/VuFind/src/VuFind/ILS/Logic/Holds.php b/module/VuFind/src/VuFind/ILS/Logic/Holds.php index 07f65072b3e..358efcb02e5 100644 --- a/module/VuFind/src/VuFind/ILS/Logic/Holds.php +++ b/module/VuFind/src/VuFind/ILS/Logic/Holds.php @@ -200,22 +200,32 @@ class Holds $result = $this->catalog->getHolding($id, $patron ? $patron : null); } + $grb = 'getRequestBlocks'; // use variable to shorten line below: + $blocks + = $patron && $this->catalog->checkCapability($grb, compact($patron)) + ? $this->catalog->getRequestBlocks($patron) : false; + $mode = $this->catalog->getHoldsMode(); if ($mode == "disabled") { $holdings = $this->standardHoldings($result); } else if ($mode == "driver") { - $holdings = $this->driverHoldings($result, $config); + $holdings = $this->driverHoldings($result, $config, !empty($blocks)); } else { $holdings = $this->generateHoldings($result, $mode, $config); } $holdings = $this->processStorageRetrievalRequests( - $holdings, $id, $patron + $holdings, $id, $patron, !empty($blocks) + ); + $holdings = $this->processILLRequests( + $holdings, $id, $patron, !empty($blocks) ); - $holdings = $this->processILLRequests($holdings, $id, $patron); } - return $this->formatHoldings($holdings); + return [ + 'blocks' => $blocks, + 'holdings' => $this->formatHoldings($holdings) + ]; } /** @@ -243,12 +253,13 @@ class Holds /** * Protected method for driver defined holdings * - * @param array $result A result set returned from a driver - * @param array $holdConfig Hold configuration from driver + * @param array $result A result set returned from a driver + * @param array $holdConfig Hold configuration from driver + * @param bool $requestsBlocked Are user requests blocked? * * @return array A sorted results set */ - protected function driverHoldings($result, $holdConfig) + protected function driverHoldings($result, $holdConfig, $requestsBlocked) { $holdings = []; @@ -258,17 +269,15 @@ class Holds if ($show) { if ($holdConfig) { // Is this copy holdable / linkable - if (isset($copy['addLink']) && $copy['addLink']) { - // If the hold is blocked, link to an error page - // instead of the hold form: - $copy['link'] = $copy['addLink'] === 'block' - ? $this->getBlockedDetails($copy) - : $this->getRequestDetails( - $copy, $holdConfig['HMACKeys'], 'Hold' - ); + if (!$requestsBlocked + && isset($copy['addLink']) && $copy['addLink'] + ) { + $copy['link'] = $this->getRequestDetails( + $copy, $holdConfig['HMACKeys'], 'Hold' + ); // If we are unsure whether hold options are available, // set a flag so we can check later via AJAX: - $copy['check'] = $copy['addLink'] == 'check'; + $copy['check'] = $copy['addLink'] === 'check'; } } @@ -371,14 +380,16 @@ class Holds * Process storage retrieval request information in holdings and set the links * accordingly. * - * @param array $holdings Holdings - * @param string $id Record ID - * @param array $patron Patron + * @param array $holdings Holdings + * @param string $id Record ID + * @param array $patron Patron + * @param bool $requestsBlocked Are user requests blocked? * * @return array Modified holdings */ - protected function processStorageRetrievalRequests($holdings, $id, $patron) - { + protected function processStorageRetrievalRequests($holdings, $id, $patron, + $requestsBlocked + ) { if (!is_array($holdings)) { return $holdings; } @@ -397,22 +408,15 @@ class Holds foreach ($holdings as &$location) { foreach ($location as &$copy) { // Is this copy requestable - if (isset($copy['addStorageRetrievalRequestLink']) + if (!$requestsBlocked + && isset($copy['addStorageRetrievalRequestLink']) && $copy['addStorageRetrievalRequestLink'] ) { - // If the request is blocked, link to an error page - // instead of the form: - if ($copy['addStorageRetrievalRequestLink'] === 'block') { - $copy['storageRetrievalRequestLink'] - = $this->getBlockedStorageRetrievalRequestDetails($copy); - } else { - $copy['storageRetrievalRequestLink'] - = $this->getRequestDetails( - $copy, - $requestConfig['HMACKeys'], - 'StorageRetrievalRequest' - ); - } + $copy['storageRetrievalRequestLink'] = $this->getRequestDetails( + $copy, + $requestConfig['HMACKeys'], + 'StorageRetrievalRequest' + ); // If we are unsure whether request options are // available, set a flag so we can check later via AJAX: $copy['checkStorageRetrievalRequest'] @@ -426,13 +430,14 @@ class Holds /** * Process ILL request information in holdings and set the links accordingly. * - * @param array $holdings Holdings - * @param string $id Record ID - * @param array $patron Patron + * @param array $holdings Holdings + * @param string $id Record ID + * @param array $patron Patron + * @param bool $requestsBlocked Are user requests blocked? * * @return array Modified holdings */ - protected function processILLRequests($holdings, $id, $patron) + protected function processILLRequests($holdings, $id, $patron, $requestsBlocked) { if (!is_array($holdings)) { return $holdings; @@ -452,22 +457,14 @@ class Holds foreach ($holdings as &$location) { foreach ($location as &$copy) { // Is this copy requestable - if (isset($copy['addILLRequestLink']) + if (!$requestsBlocked && isset($copy['addILLRequestLink']) && $copy['addILLRequestLink'] ) { - // If the request is blocked, link to an error page - // instead of the form: - if ($copy['addILLRequestLink'] === 'block') { - $copy['ILLRequestLink'] - = $this->getBlockedILLRequestDetails($copy); - } else { - $copy['ILLRequestLink'] - = $this->getRequestDetails( - $copy, - $requestConfig['HMACKeys'], - 'ILLRequest' - ); - } + $copy['ILLRequestLink'] = $this->getRequestDetails( + $copy, + $requestConfig['HMACKeys'], + 'ILLRequest' + ); // If we are unsure whether request options are // available, set a flag so we can check later via AJAX: $copy['checkILLRequest'] @@ -515,53 +512,6 @@ class Holds ]; } - /** - * Returns a URL to display a "blocked hold" message. - * - * @param array $holdDetails An array of item data - * - * @return array Details for generating URL - */ - protected function getBlockedDetails($holdDetails) - { - // Build Params - return [ - 'action' => 'BlockedHold', 'record' => $holdDetails['id'] - ]; - } - - /** - * Returns a URL to display a "blocked storage retrieval request" message. - * - * @param array $details An array of item data - * - * @return array Details for generating URL - */ - protected function getBlockedStorageRetrievalRequestDetails($details) - { - // Build Params - return [ - 'action' => 'BlockedStorageRetrievalRequest', - 'record' => $details['id'] - ]; - } - - /** - * Returns a URL to display a "blocked ILL request" message. - * - * @param array $details An array of item data - * - * @return array Details for generating URL - */ - protected function getBlockedILLRequestDetails($details) - { - // Build Params - return [ - 'action' => 'BlockedILLRequest', - 'record' => $details['id'] - ]; - } - /** * Get a grouping key for a holdings item * diff --git a/themes/bootstrap3/templates/RecordTab/holdingsils.phtml b/themes/bootstrap3/templates/RecordTab/holdingsils.phtml index 03e79777430..93d9c28006d 100644 --- a/themes/bootstrap3/templates/RecordTab/holdingsils.phtml +++ b/themes/bootstrap3/templates/RecordTab/holdingsils.phtml @@ -10,7 +10,7 @@ try { $holdings = $this->driver->getRealTimeHoldings(); } catch (\VuFind\Exception\ILS $e) { - $holdings = []; + $holdings = ['holdings' => []]; $offlineMode = 'ils-offline'; } // Set page title. @@ -19,8 +19,14 @@ <?=$this->context($this)->renderInContext('librarycards/selectcard.phtml', ['user' => $this->auth()->isLoggedIn()]); ?> +<? if (!empty($holdings['blocks'])):?> + <div id="account-block-msg" class="alert alert-danger"> + <?=$this->transEsc('account_block_options_missing', ['%%details%%' => implode('; ', $holdings['blocks'])]) ?> + </div> +<? endif; ?> + <?=($offlineMode == "ils-offline") ? $this->render('Helpers/ils-offline.phtml', ['offlineModeMsg' => 'ils_offline_holdings_message']) : ''?> -<? if (($this->ils()->getHoldsMode() == 'driver' && !empty($holdings)) || $this->ils()->getTitleHoldsMode() == 'driver'): ?> +<? if (($this->ils()->getHoldsMode() == 'driver' && !empty($holdings['holdings'])) || $this->ils()->getTitleHoldsMode() == 'driver'): ?> <? if ($account->loginEnabled() && $offlineMode != 'ils-offline'): ?> <? if (!$user): ?> <div class="alert alert-info"> @@ -34,7 +40,7 @@ <? endif; ?> <? endif; ?> <? $holdingTitleHold = $this->driver->tryMethod('getRealTimeTitleHold'); if (!empty($holdingTitleHold)): ?> - <a class="placehold" data-lightbox title="<?=$this->transEsc('request_place_text')?>" href="<?=$this->recordLink()->getRequestUrl($holdingTitleHold)?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc('title_hold_place')?></a> + <a class="placehold" data-lightbox title="<?=$this->transEsc('request_place_text')?>" href="<?=$this->recordLink()->getRequestUrl($holdingTitleHold)?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc('title_hold_place')?></a> <? endif; ?> <? if (!empty($urls) || $openUrlActive): ?> <h3><?=$this->transEsc("Internet")?></h3> @@ -45,7 +51,7 @@ <? endif; ?> <? if ($openUrlActive): ?><?=$openUrl->renderTemplate()?><? endif; ?> <? endif; ?> -<? foreach ($holdings as $holding): ?> +<? foreach ($holdings['holdings'] as $holding): ?> <h3> <? $locationText = $this->transEsc('location_' . $holding['location'], [], $holding['location']); ?> <? if (isset($holding['locationhref']) && $holding['locationhref']): ?> @@ -87,10 +93,6 @@ $check = isset($row['check']) && $row['check']; $checkStorageRetrievalRequest = isset($row['checkStorageRetrievalRequest']) && $row['checkStorageRetrievalRequest']; $checkILLRequest = isset($row['checkILLRequest']) && $row['checkILLRequest']; - // AJAX block record? - $block = !$check && isset($row['addLink']) && $row['addLink'] === 'block'; - $blockStorageRetrievalRequest = !$checkStorageRetrievalRequest && isset($row['addStorageRetrievalRequestLink']) && $row['addStorageRetrievalRequestLink'] === 'block'; - $blockILLRequest = !$checkILLRequest && isset($row['addILLRequestLink']) && $row['addILLRequestLink'] === 'block'; ?> <? if (isset($row['barcode']) && $row['barcode'] != ""): ?> <tr vocab="http://schema.org/" typeof="Offer"> @@ -106,13 +108,13 @@ <? if ($row['availability']): ?> <? /* Begin Available Items (Holds) */ ?> <span class="text-success"><?=$this->transEsc("Available")?><link property="availability" href="http://schema.org/InStock" /></span> - <? if (!$block && isset($row['link']) && $row['link']): ?> + <? if (isset($row['link']) && $row['link']): ?> <a class="<?=$check ? 'checkRequest ' : ''?>placehold" data-lightbox href="<?=$this->recordLink()->getRequestUrl($row['link'])?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc($check ? "Check Hold" : "Place a Hold")?></a> <? endif; ?> - <? if (!$blockStorageRetrievalRequest && isset($row['storageRetrievalRequestLink']) && $row['storageRetrievalRequestLink']): ?> + <? if (isset($row['storageRetrievalRequestLink']) && $row['storageRetrievalRequestLink']): ?> <a class="<?=$checkStorageRetrievalRequest ? 'checkStorageRetrievalRequest ' : ''?> placeStorageRetrievalRequest" data-lightbox href="<?=$this->recordLink()->getRequestUrl($row['storageRetrievalRequestLink'])?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc($checkStorageRetrievalRequest ? "storage_retrieval_request_check_text" : "storage_retrieval_request_place_text")?></a> <? endif; ?> - <? if (!$blockILLRequest && isset($row['ILLRequestLink']) && $row['ILLRequestLink']): ?> + <? if (isset($row['ILLRequestLink']) && $row['ILLRequestLink']): ?> <a class="<?=$checkILLRequest ? 'checkILLRequest ' : ''?>placeILLRequest" data-lightbox href="<?=$this->recordLink()->getRequestUrl($row['ILLRequestLink'])?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc($checkILLRequest ? "ill_request_check_text" : "ill_request_place_text")?></a> <? endif; ?> <? else: ?> @@ -125,7 +127,7 @@ <? if (isset($row['requests_placed']) && $row['requests_placed'] > 0): ?> <span><?=$this->transEsc("Requests")?>: <?=$this->escapeHtml($row['requests_placed'])?></span> <? endif; ?> - <? if (!$block && isset($row['link']) && $row['link']): ?> + <? if (isset($row['link']) && $row['link']): ?> <a class="<?=$check ? 'checkRequest' : ''?> placehold" data-lightbox href="<?=$this->recordLink()->getRequestUrl($row['link'])?>"><i class="fa fa-flag" aria-hidden="true"></i> <?=$this->transEsc($check ? "Check Recall" : "Recall This")?></a> <? endif; ?> <? endif; ?> diff --git a/themes/jquerymobile/templates/RecordTab/holdingsils.phtml b/themes/jquerymobile/templates/RecordTab/holdingsils.phtml index 3da187abb51..baa02f96759 100644 --- a/themes/jquerymobile/templates/RecordTab/holdingsils.phtml +++ b/themes/jquerymobile/templates/RecordTab/holdingsils.phtml @@ -6,7 +6,7 @@ try { $holdings = $this->driver->getRealTimeHoldings(); } catch (\VuFind\Exception\ILS $e) { - $holdings = []; + $holdings = ['holdings' => []]; $offlineMode = 'ils-offline'; } @@ -16,6 +16,12 @@ <?=$this->context($this)->renderInContext('librarycards/selectcard.phtml', array('user' => $this->auth()->isLoggedIn())); ?> +<? if (!empty($holdings['blocks'])):?> + <div id="account-block-msg" class="error"> + <?=$this->transEsc('account_block_options_missing', ['%%details%%' => implode('; ', $holdings['blocks'])]) ?> + </div> +<? endif; ?> + <? if ($offlineMode == "ils-offline"): ?> <div class="sysInfo"> <h2><?=$this->transEsc('ils_offline_title')?></h2> @@ -25,7 +31,7 @@ <p><a href="mailto:<?=$supportEmail?>"><?=$supportEmail?></a></p> </div> <? endif; ?> -<? if (($this->ils()->getHoldsMode() == 'driver' && !empty($holdings)) || $this->ils()->getTitleHoldsMode() == 'driver'): ?> +<? if (($this->ils()->getHoldsMode() == 'driver' && !empty($holdings['holdings'])) || $this->ils()->getTitleHoldsMode() == 'driver'): ?> <? if ($account->loginEnabled() && $offlineMode != 'ils-offline'): ?> <? if (!$user): ?> <div class="info"> @@ -41,7 +47,7 @@ <? $holdingTitleHold = $this->driver->tryMethod('getRealTimeTitleHold'); if (!empty($holdingTitleHold)): ?> <a rel="external" class="holdPlace" href="<?=$this->recordLink()->getRequestUrl($holdingTitleHold, false)?>"><?=$this->transEsc('title_hold_place')?></a> <? endif; ?> -<? foreach ($holdings as $holding): ?> +<? foreach ($holdings['holdings'] as $holding): ?> <h4> <? $locationText = $this->transEsc('location_' . $holding['location'], array(), $holding['location']); ?> <? if (isset($holding['locationhref']) && $holding['locationhref']): ?> -- GitLab