Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bulk favorites save from search results #3960

Merged
merged 9 commits into from
Oct 16, 2024
6 changes: 6 additions & 0 deletions module/VuFind/src/VuFind/Controller/AbstractRecord.php
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,12 @@ public function saveAction()
return $response;
}

if ($this->formWasSubmitted('newList')) {
// Remove submit now from parameters
$this->getRequest()->getPost()->set('newList', null)->set('submitButton', null);
return $this->forwardTo('MyResearch', 'editList', ['id' => 'NEW']);
}

// Process form submission:
if ($this->formWasSubmitted()) {
return $this->processSave();
Expand Down
24 changes: 16 additions & 8 deletions module/VuFind/src/VuFind/Controller/CartController.php
Original file line number Diff line number Diff line change
Expand Up @@ -533,9 +533,22 @@ public function saveAction()
['cartIds' => $ids, 'cartAction' => 'Save']
);
}

$viewModel = $this->createViewModel(
[
'records' => $this->getRecordLoader()->loadBatch($ids),
'lists' => $this->getDbService(UserListServiceInterface::class)->getUserListsByUser($user),
]
);
if ($submitDisabled ?? false) {
return $viewModel;
}
if ($this->formWasSubmitted('newList')) {
// Remove submit now from parameters
$this->getRequest()->getPost()->set('newList', null)->set('submitButton', null);
return $this->forwardTo('MyResearch', 'editlist', ['id' => 'NEW']);
}
// Process submission if necessary:
if (!($submitDisabled ?? false) && $this->formWasSubmitted()) {
if ($this->formWasSubmitted()) {
$results = $this->getService(FavoritesService::class)
->saveRecordsToFavorites($this->getRequest()->getPost()->toArray(), $user);
$listUrl = $this->url()->fromRoute(
Expand All @@ -553,11 +566,6 @@ public function saveAction()
}

// Pass record and list information to view:
return $this->createViewModel(
[
'records' => $this->getRecordLoader()->loadBatch($ids),
'lists' => $this->getDbService(UserListServiceInterface::class)->getUserListsByUser($user),
]
);
return $viewModel;
}
}
36 changes: 16 additions & 20 deletions module/VuFind/src/VuFind/Controller/MyResearchController.php
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
use VuFind\Crypt\SecretCalculator;
use VuFind\Db\Entity\SearchEntityInterface;
use VuFind\Db\Entity\UserEntityInterface;
use VuFind\Db\Entity\UserListEntityInterface;
use VuFind\Db\Service\SearchServiceInterface;
use VuFind\Db\Service\UserListServiceInterface;
use VuFind\Db\Service\UserResourceServiceInterface;
Expand Down Expand Up @@ -1180,9 +1181,9 @@ protected function processEditList(UserEntityInterface $user, $list)
// If the user is in the process of saving a record, send them back
// to the save screen; otherwise, send them back to the list they
// just edited.
$recordId = $this->params()->fromQuery('recordId');
$recordSource
= $this->params()->fromQuery('recordSource', DEFAULT_SEARCH_BACKEND);
$recordId = $this->params()->fromQuery('recordId') ?? $this->params()->fromPost('recordId');
$recordSource = $this->params()->fromQuery('recordSource')
?? $this->params()->fromPost('recordSource', DEFAULT_SEARCH_BACKEND);
if (!empty($recordId)) {
$details = $this->getRecordRouter()->getActionRouteDetails(
$recordSource . '|' . $recordId,
Expand All @@ -1196,19 +1197,12 @@ protected function processEditList(UserEntityInterface $user, $list)

// Similarly, if the user is in the process of bulk-saving records,
// send them back to the appropriate place in the cart.
$bulkIds = $this->params()->fromPost(
'ids',
$this->params()->fromQuery('ids', [])
);
$bulkIds = $this->params()->fromPost('ids') ?? $this->params()->fromQuery('ids', []);
if (!empty($bulkIds)) {
$params = [];
foreach ($bulkIds as $id) {
$params[] = urlencode('ids[]') . '=' . urlencode($id);
}
$saveUrl = $this->url()->fromRoute('cart-save');
$saveUrl .= (!str_contains($saveUrl, '?')) ? '?' : '&';
return $this->redirect()
->toUrl($saveUrl . implode('&', $params));
// Add final id of the list to request post so cartcontroller saveaction
// can properly load the list
$this->getRequest()->getPost()->set('list', $finalId);
return $this->forwardTo('Cart', 'Save');
}

return $this->redirect()->toRoute('userList', ['id' => $finalId]);
Expand Down Expand Up @@ -1240,7 +1234,7 @@ public function editlistAction()

// Is this a new list or an existing list? Handle the special 'NEW' value
// of the ID parameter:
$id = $this->params()->fromRoute('id', $this->params()->fromQuery('id'));
$id = $this->params()->fromRoute('id') ?? $this->params()->fromQuery('id') ?? $this->params()->fromPost('id');
$newList = ($id == 'NEW');
// If this is a new list, use the FavoritesService to pre-populate some values in
// a fresh object; if it's an existing list, we can just fetch from the database.
Expand All @@ -1255,10 +1249,8 @@ public function editlistAction()
}

// Process form submission:
if ($this->formWasSubmitted()) {
if ($redirect = $this->processEditList($user, $list)) {
return $redirect;
}
if ($this->formWasSubmitted() && $redirect = $this->processEditList($user, $list)) {
return $redirect;
}

$listTags = null;
Expand All @@ -1273,6 +1265,10 @@ public function editlistAction()
'list' => $list,
'newList' => $newList,
'listTags' => $listTags,
'recordIds' => $this->params()->fromQuery('ids') ?? $this->params()->fromPost('ids', []),
'recordId' => $this->params()->fromQuery('recordId') ?? $this->params()->fromPost('recordId', false),
'recordSource' => $this->params()->fromQuery('recordSource')
?? $this->params()->fromPost('recordSource', DEFAULT_SEARCH_BACKEND),
]
);
}
Expand Down
24 changes: 24 additions & 0 deletions themes/bootstrap3/templates/cart/form-record-hidden-inputs.phtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
$recordId = $this->driver?->getUniqueId() ?? $this->recordId ?? false;
$recordSource = $this->driver?->getSourceIdentifier() ?? $this->recordSource ?? DEFAULT_SEARCH_BACKEND;
if ($recordId && str_contains($recordId, '|')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that this could malfunction in multiple ways if an ID contains a pipe character. Is there a safer way to determine when unpacking is required? i.e. do we only unpack recordId if there is no driver and no source set? (I didn't analyze all the code paths to figure out options, but I can try to spend a little time on that if it would help).

Additionally, are the variables reversed? Down below, you construct "source|id" strings, but here you're unpacking it as "id|source".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, missed that... will adjust them also!

Copy link
Contributor Author

@LuomaJuha LuomaJuha Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, i think it would be safer to leave it out and let the calling templates to give what is needed? I think that part was littlebit of overthinking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely safer this way -- thanks!

[$recordId, $recordSource] = explode('|', $recordId, 2);
}
?>
<?php if ($recordId): ?>
<input type="hidden" name="recordId" value="<?=$this->escapeHtmlAttr($recordId)?>">
<input type="hidden" name="recordSource" value="<?=$this->escapeHtmlAttr($recordSource)?>">
<?php return; ?>
<?php endif; ?>

<?php
if (!isset($this->recordIds)) {
$this->recordIds = array_map(
fn ($record) => $record->getSourceIdentifier() . '|' . $record->getUniqueId(),
$this->records ?? []
);
}
?>
<?php foreach ($this->recordIds as $current): ?>
<input type="hidden" name="ids[]" value="<?=$this->escapeHtmlAttr($current)?>">
<?php endforeach; ?>
8 changes: 2 additions & 6 deletions themes/bootstrap3/templates/cart/save.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@

<?php if (count($this->records) > 0): ?>
<form class="form-cart-save" method="post" action="<?=$this->url('cart-save')?>" name="bulkSave">
<?php $idParams = []; ?>
<?php foreach ($this->records as $current): ?>
<?php $idParams[] = urlencode('ids[]') . '=' . urlencode($current->getSourceIdentifier() . '|' . $current->getUniqueId()) ?>
<input type="hidden" name="ids[]" value="<?=$this->escapeHtmlAttr($current->getSourceIdentifier() . '|' . $current->getUniqueId())?>">
<?php endforeach; ?>
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<div class="form-group">
<label class="control-label"><?=$this->transEsc('Title')?>:</label>
<?php if (count($this->records) > 1): ?>
Expand Down Expand Up @@ -48,7 +44,7 @@
<option value=""><?=$this->transEsc('default_list_title') ?></option>
<?php endif; ?>
</select>
<a class="btn btn-link" id="make-list" href="<?=$this->url('editList', ['id' => 'NEW']) . '?' . implode('&amp;', $idParams) ?>"><?=$this->transEsc('or create a new list'); ?></a>
<button type="submit" id="make-list" name="newList" class="btn btn-link"><?=$this->transEsc('or create a new list'); ?></button>
</div>

<?php if ($this->usertags()->getMode() !== 'disabled'): ?>
Expand Down
3 changes: 2 additions & 1 deletion themes/bootstrap3/templates/myresearch/editlist.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@

<h2><?=$this->transEsc($pageTitle); ?></h2>

<form class="form-edit-list" method="post" name="<?=$this->newList ? 'newList' : 'editListForm'?>">
<form class="form-edit-list" method="post" name="<?=$this->newList ? 'newList' : 'editListForm'?>" action="<?=$this->url('editList'); ?>">
<input type="hidden" name="id" value="<?=empty($listId = $this->list->getId()) ? 'NEW' : $this->escapeHtmlAttr($listId) ?>">
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<div class="form-group">
<label class="control-label" for="list_title"><?=$this->transEsc('List'); ?>:</label>
<input id="list_title" class="form-control" type="text" name="title" value="<?=$this->escapeHtmlAttr($this->list->getTitle())?>">
Expand Down
5 changes: 2 additions & 3 deletions themes/bootstrap3/templates/record/save.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
<h2><?=$this->translate('add_to_favorites_html', ['%%title%%' => $this->escapeHtml($this->driver->getBreadcrumb())]) ?></h2>
<form class="form-record-save" method="post" action="<?=$this->escapeHtmlAttr($this->recordLinker()->getActionUrl($this->driver, 'Save')) ?>" name="saveRecord" data-lightbox-onclose="VuFind.saveStatuses.refresh">
<input type="hidden" name="submitButton" value="1">
<input type="hidden" name="id" value="<?=$this->escapeHtmlAttr($this->driver->getUniqueId()) ?>">
<input type="hidden" name="source" value="<?=$this->escapeHtmlAttr($this->driver->getSourceIdentifier())?>">
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<?php if (!empty($this->containingLists)): ?>
<p><?=$this->transEsc('This item is already part of the following list/lists') ?>:
<?php foreach ($this->containingLists as $i => $list): ?>
Expand Down Expand Up @@ -41,7 +40,7 @@
<?php endif; ?>
</select>
<?php endif; ?>
<a class="btn btn-link" id="make-list" href="<?=$this->url('editList', ['id' => 'NEW'])?>?recordId=<?=urlencode($this->driver->getUniqueId())?>&amp;recordSource=<?=urlencode($this->driver->getSourceIdentifier())?>"><?=$showLists ? $this->transEsc('or create a new list') : $this->transEsc('Create a List'); ?></a>
<button type="submit" name="newList" class="btn btn-link" id="make-list"><?=$showLists ? $this->transEsc('or create a new list') : $this->transEsc('Create a List'); ?></button>
</div>

<?php if ($showLists): ?>
Expand Down
24 changes: 24 additions & 0 deletions themes/bootstrap5/templates/cart/form-record-hidden-inputs.phtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
$recordId = $this->record?->getUniqueId() ?? $this->driver?->getUniqueId() ?? $this->recordId ?? false;
$recordSource = $this->record?->getSourceIdentifier() ?? $this->driver?->getSourceIdentifier() ?? $this->recordSource ?? DEFAULT_SEARCH_BACKEND;
if ($recordId && str_contains($recordId, '|')) {
[$recordId, $recordSource] = explode('|', $recordId, 2);
}
?>
<?php if ($recordId): ?>
<input type="hidden" name="recordId" value="<?=$this->escapeHtmlAttr($recordId)?>">
<input type="hidden" name="recordSource" value="<?=$this->escapeHtmlAttr($recordSource)?>">
<?php return; ?>
<?php endif; ?>

<?php
if (!isset($this->recordIds)) {
$this->recordIds = array_map(
fn ($record) => $record->getSourceIdentifier() . '|' . $record->getUniqueId(),
$this->records ?? []
);
}
?>
<?php foreach ($this->recordIds as $current): ?>
<input type="hidden" name="ids[]" value="<?=$this->escapeHtmlAttr($current)?>">
<?php endforeach; ?>
8 changes: 2 additions & 6 deletions themes/bootstrap5/templates/cart/save.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@

<?php if (count($this->records) > 0): ?>
<form class="form-cart-save" method="post" action="<?=$this->url('cart-save')?>" name="bulkSave">
<?php $idParams = []; ?>
<?php foreach ($this->records as $current): ?>
<?php $idParams[] = urlencode('ids[]') . '=' . urlencode($current->getSourceIdentifier() . '|' . $current->getUniqueId()) ?>
<input type="hidden" name="ids[]" value="<?=$this->escapeHtmlAttr($current->getSourceIdentifier() . '|' . $current->getUniqueId())?>">
<?php endforeach; ?>
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<div class="form-group">
<label class="control-label"><?=$this->transEsc('Title')?>:</label>
<?php if (count($this->records) > 1): ?>
Expand Down Expand Up @@ -48,7 +44,7 @@
<option value=""><?=$this->transEsc('default_list_title') ?></option>
<?php endif; ?>
</select>
<a class="btn btn-link" id="make-list" href="<?=$this->url('editList', ['id' => 'NEW']) . '?' . implode('&amp;', $idParams) ?>"><?=$this->transEsc('or create a new list'); ?></a>
<button type="submit" id="make-list" name="newList" class="btn btn-link"><?=$this->transEsc('or create a new list'); ?></button>
</div>

<?php if ($this->usertags()->getMode() !== 'disabled'): ?>
Expand Down
3 changes: 2 additions & 1 deletion themes/bootstrap5/templates/myresearch/editlist.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@

<h2><?=$this->transEsc($pageTitle); ?></h2>

<form class="form-edit-list" method="post" name="<?=$this->newList ? 'newList' : 'editListForm'?>">
<form class="form-edit-list" method="post" name="<?=$this->newList ? 'newList' : 'editListForm'?>" action="<?=$this->url('editList'); ?>">
<input type="hidden" name="id" value="<?=empty($listId = $this->list->getId()) ? 'NEW' : $this->escapeHtmlAttr($listId) ?>">
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<div class="form-group">
<label class="control-label" for="list_title"><?=$this->transEsc('List'); ?>:</label>
<input id="list_title" class="form-control" type="text" name="title" value="<?=$this->escapeHtmlAttr($this->list->getTitle())?>">
Expand Down
5 changes: 2 additions & 3 deletions themes/bootstrap5/templates/record/save.phtml
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
<h2><?=$this->translate('add_to_favorites_html', ['%%title%%' => $this->escapeHtml($this->driver->getBreadcrumb())]) ?></h2>
<form class="form-record-save" method="post" action="<?=$this->escapeHtmlAttr($this->recordLinker()->getActionUrl($this->driver, 'Save')) ?>" name="saveRecord" data-lightbox-onclose="VuFind.saveStatuses.refresh">
<input type="hidden" name="submitButton" value="1">
<input type="hidden" name="id" value="<?=$this->escapeHtmlAttr($this->driver->getUniqueId()) ?>">
<input type="hidden" name="source" value="<?=$this->escapeHtmlAttr($this->driver->getSourceIdentifier())?>">
<?=$this->context($this)->renderInContext('cart/form-record-hidden-inputs.phtml', []); ?>
<?php if (!empty($this->containingLists)): ?>
<p><?=$this->transEsc('This item is already part of the following list/lists') ?>:
<?php foreach ($this->containingLists as $i => $list): ?>
Expand Down Expand Up @@ -41,7 +40,7 @@
<?php endif; ?>
</select>
<?php endif; ?>
<a class="btn btn-link" id="make-list" href="<?=$this->url('editList', ['id' => 'NEW'])?>?recordId=<?=urlencode($this->driver->getUniqueId())?>&amp;recordSource=<?=urlencode($this->driver->getSourceIdentifier())?>"><?=$showLists ? $this->transEsc('or create a new list') : $this->transEsc('Create a List'); ?></a>
<button type="submit" name="newList" class="btn btn-link" id="make-list"><?=$showLists ? $this->transEsc('or create a new list') : $this->transEsc('Create a List'); ?></button>
</div>

<?php if ($showLists): ?>
Expand Down
Loading