From 961412e684b8ce88eeffcb16cf4b4fe21cf0ef2a Mon Sep 17 00:00:00 2001 From: Lukas Schaefer Date: Fri, 23 Jan 2026 10:25:27 -0500 Subject: [PATCH] Add better error message on users side for failed approval Signed-off-by: Lukas Schaefer --- lib/Controller/ApprovalController.php | 31 ++++++++++++++++++------ lib/Exceptions/OutdatedEtagException.php | 13 ++++++++++ lib/Service/ApprovalService.php | 30 +++++++++++++++++++---- src/files/actions/approveAction.js | 4 +-- src/files/actions/rejectAction.js | 4 +-- src/files/helpers.js | 28 +++++++++------------ src/files/modals.js | 4 +-- src/views/ApprovalTab.vue | 4 +-- 8 files changed, 82 insertions(+), 36 deletions(-) create mode 100644 lib/Exceptions/OutdatedEtagException.php diff --git a/lib/Controller/ApprovalController.php b/lib/Controller/ApprovalController.php index a696d91d..944d4fed 100644 --- a/lib/Controller/ApprovalController.php +++ b/lib/Controller/ApprovalController.php @@ -8,12 +8,14 @@ namespace OCA\Approval\Controller; use OCA\Approval\AppInfo\Application; +use OCA\Approval\Exceptions\OutdatedEtagException; use OCA\Approval\Service\ApprovalService; use OCA\Approval\Service\RuleService; - +use OCP\AppFramework\Http; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCSController; +use OCP\IL10N; use OCP\IRequest; class ApprovalController extends OCSController { @@ -23,6 +25,7 @@ public function __construct( IRequest $request, private ApprovalService $approvalService, private RuleService $ruleService, + private IL10N $l10n, private ?string $userId, ) { parent::__construct($appName, $request); @@ -76,12 +79,19 @@ public function getPendingNodes(?int $since = null): DataResponse { * * @param int $fileId * @param string|null $message + * @param string|null $etag * @return DataResponse */ #[NoAdminRequired] - public function approve(int $fileId, ?string $message = ''): DataResponse { - $this->approvalService->approve($fileId, $this->userId, $message); - return new DataResponse(1); + public function approve(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse { + try { + if ($this->approvalService->approve($fileId, $this->userId, $message, $etag)) { + return new DataResponse([]); + } + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } catch (OutdatedEtagException) { + return new DataResponse(['error' => $this->l10n->t('The file/folder you tried to approve has an outdated content, please reload and review it again')], Http::STATUS_BAD_REQUEST); + } } /** @@ -89,12 +99,19 @@ public function approve(int $fileId, ?string $message = ''): DataResponse { * * @param int $fileId * @param string|null $message + * @param string|null $etag * @return DataResponse */ #[NoAdminRequired] - public function reject(int $fileId, ?string $message = ''): DataResponse { - $this->approvalService->reject($fileId, $this->userId, $message); - return new DataResponse(1); + public function reject(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse { + try { + if ($this->approvalService->reject($fileId, $this->userId, $message, $etag)) { + return new DataResponse([]); + } + return new DataResponse([], Http::STATUS_BAD_REQUEST); + } catch (OutdatedEtagException) { + return new DataResponse(['error' => $this->l10n->t('The file/folder you tried to reject has an outdated content, please reload and review it again')], Http::STATUS_BAD_REQUEST); + } } /** diff --git a/lib/Exceptions/OutdatedEtagException.php b/lib/Exceptions/OutdatedEtagException.php new file mode 100644 index 00000000..aa3523de --- /dev/null +++ b/lib/Exceptions/OutdatedEtagException.php @@ -0,0 +1,13 @@ +root->getFirstNodeById($fileId); + if ($file !== null) { + return $file->getEtag(); + } + return ''; + } + /** * Get approval state of a given file for a given user * @param int $fileId @@ -339,10 +349,15 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce * @param int $fileId * @param string|null $userId * @param string $message + * @param string $etag optional etag of the file to check if it has changed since approval was requested * @return bool success + * @throws OutdatedEtagException */ - public function approve(int $fileId, ?string $userId, string $message = ''): bool { + public function approve(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool { $fileState = $this->getApprovalState($fileId, $userId); + if ($etag !== '' && $etag !== $this->getEtag($fileId)) { + throw new OutdatedEtagException(); + } // if file has pending tag and user is authorized to approve it if ($fileState['state'] === Application::STATE_APPROVABLE) { $rules = $this->ruleService->getRules(); @@ -377,10 +392,15 @@ public function approve(int $fileId, ?string $userId, string $message = ''): boo * @param int $fileId * @param string|null $userId * @param string $message + * @param string $etag optional etag of the file to check if it has changed since approval was requested * @return bool success + * @throws OutdatedEtagException */ - public function reject(int $fileId, ?string $userId, string $message = ''): bool { + public function reject(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool { $fileState = $this->getApprovalState($fileId, $userId); + if ($etag !== '' && $etag !== $this->getEtag($fileId)) { + throw new OutdatedEtagException(); + } // if file has pending tag and user is authorized to approve it if ($fileState['state'] === Application::STATE_APPROVABLE) { $rules = $this->ruleService->getRules(); diff --git a/src/files/actions/approveAction.js b/src/files/actions/approveAction.js index 292518ee..d6e1bba4 100644 --- a/src/files/actions/approveAction.js +++ b/src/files/actions/approveAction.js @@ -26,7 +26,7 @@ export const approveAction = new FileAction({ async exec({ nodes }) { const node = nodes[0] try { - await approve(node.fileid, node.basename, node) + await approve(node) } catch (error) { console.debug('Approve action failed') } @@ -35,7 +35,7 @@ export const approveAction = new FileAction({ async execBatch({ nodes }) { const promises = nodes .filter(node => node.attributes['approval-state'] === states.APPROVABLE) - .map(node => approve(node.fileid, node.basename, node, false)) + .map(node => approve(node, false)) const results = await Promise.allSettled(promises) return results.map(promise => promise.status === 'fulfilled') }, diff --git a/src/files/actions/rejectAction.js b/src/files/actions/rejectAction.js index 7532d192..f2f18149 100644 --- a/src/files/actions/rejectAction.js +++ b/src/files/actions/rejectAction.js @@ -26,7 +26,7 @@ export const rejectAction = new FileAction({ async exec({ nodes }) { const node = nodes[0] try { - await reject(node.fileid, node.basename, node) + await reject(node) } catch (error) { console.debug('Reject action failed') } @@ -35,7 +35,7 @@ export const rejectAction = new FileAction({ async execBatch({ nodes }) { const promises = nodes .filter(node => node.attributes['approval-state'] === states.APPROVABLE) - .map(node => reject(node.fileid, node.basename, node, false)) + .map(node => reject(node, false)) const results = await Promise.allSettled(promises) return results.map(promise => promise.status === 'fulfilled') }, diff --git a/src/files/helpers.js b/src/files/helpers.js index e15464df..7ff3e449 100644 --- a/src/files/helpers.js +++ b/src/files/helpers.js @@ -83,39 +83,35 @@ export async function requestAfterShareCreation(fileId, fileName, ruleId, node = } } -export async function approve(fileId, fileName, node = null, notify = true, message = '') { - const url = generateOcsUrl('apps/approval/api/v1/approve/{fileId}', { fileId }) +export async function approve(node, notify = true, message = '') { + const url = generateOcsUrl('apps/approval/api/v1/approve/{fileId}', { fileId: node.fileid }) try { - await axios.put(url, { message }) + await axios.put(url, { message, etag: node.attributes.etag }) if (notify) { - showSuccess(t('approval', 'You approved {name}', { name: fileName })) - } - if (node) { - await updateNodeApprovalState(node) + showSuccess(t('approval', 'You approved {name}', { name: node.basename })) } + await updateNodeApprovalState(node) } catch (error) { console.error(error) if (notify) { - showError(t('approval', 'Failed to approve {name}', { name: fileName })) + showError(error.response.data?.ocs?.data?.error ?? t('approval', 'Failed to approve {name}', { name: node.basename })) } throw error } } -export async function reject(fileId, fileName, node = null, notify = true, message = '') { - const url = generateOcsUrl('apps/approval/api/v1/reject/{fileId}', { fileId }) +export async function reject(node, notify = true, message = '') { + const url = generateOcsUrl('apps/approval/api/v1/reject/{fileId}', { fileId: node.fileid }) try { - await axios.put(url, { message }) + await axios.put(url, { message, etag: node.attributes.etag }) if (notify) { - showSuccess(t('approval', 'You rejected {name}', { name: fileName })) - } - if (node) { - await updateNodeApprovalState(node) + showSuccess(t('approval', 'You rejected {name}', { name: node.basename })) } + await updateNodeApprovalState(node) } catch (error) { console.error(error) if (notify) { - showError(t('approval', 'Failed to reject {name}', { name: fileName })) + showError(error.response.data?.ocs?.data?.error ?? t('approval', 'Failed to reject {name}', { name: node.basename })) } throw error } diff --git a/src/files/modals.js b/src/files/modals.js index fabc521d..d0fe2350 100644 --- a/src/files/modals.js +++ b/src/files/modals.js @@ -39,10 +39,10 @@ export function createInfoModal() { console.debug('[Approval] modal closed') }, onApprove: (node, message) => { - approve(node.fileid, node.basename, node, true, message) + approve(node, true, message) }, onReject: (node, message) => { - reject(node.fileid, node.basename, node, true, message) + reject(node, true, message) }, onRequest: (node) => { onRequestFileAction(node) diff --git a/src/views/ApprovalTab.vue b/src/views/ApprovalTab.vue index a0e3edc3..43ba63a1 100644 --- a/src/views/ApprovalTab.vue +++ b/src/views/ApprovalTab.vue @@ -113,12 +113,12 @@ export default { }, async onApprove(message) { this.state = null - await approve(this.fileId, this.fileName, null, true, message) + await approve(this.node, true, message) this.update() }, async onReject(message) { this.state = null - await reject(this.fileId, this.fileName, null, true, message) + await reject(this.node, true, message) this.update() }, async onRequestSubmit(ruleId, createShares) {