Skip to content

Commit c3fe19e

Browse files
lukasdotcomlukasdotcom
authored
Merge pull request #366 from nextcloud/backport/365/stable32
[stable32] Add better error message on users side for failed approval
2 parents d551743 + 4305ab5 commit c3fe19e

8 files changed

Lines changed: 89 additions & 38 deletions

File tree

‎lib/Controller/ApprovalController.php‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,14 @@
88
namespace OCA\Approval\Controller;
99

1010
use OCA\Approval\AppInfo\Application;
11+
use OCA\Approval\Exceptions\OutdatedEtagException;
1112
use OCA\Approval\Service\ApprovalService;
1213
use OCA\Approval\Service\RuleService;
13-
14+
use OCP\AppFramework\Http;
1415
use OCP\AppFramework\Http\Attribute\NoAdminRequired;
1516
use OCP\AppFramework\Http\DataResponse;
1617
use OCP\AppFramework\OCSController;
18+
use OCP\IL10N;
1719
use OCP\IRequest;
1820

1921
class ApprovalController extends OCSController {
@@ -23,6 +25,7 @@ public function __construct(
2325
IRequest $request,
2426
private ApprovalService $approvalService,
2527
private RuleService $ruleService,
28+
private IL10N $l10n,
2629
private ?string $userId,
2730
) {
2831
parent::__construct($appName, $request);
@@ -76,25 +79,39 @@ public function getPendingNodes(?int $since = null): DataResponse {
7679
*
7780
* @param int $fileId
7881
* @param string|null $message
82+
* @param string|null $etag
7983
* @return DataResponse
8084
*/
8185
#[NoAdminRequired]
82-
public function approve(int $fileId, ?string $message = ''): DataResponse {
83-
$this->approvalService->approve($fileId, $this->userId, $message);
84-
return new DataResponse(1);
86+
public function approve(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse {
87+
try {
88+
if ($this->approvalService->approve($fileId, $this->userId, $message, $etag)) {
89+
return new DataResponse([]);
90+
}
91+
return new DataResponse([], Http::STATUS_BAD_REQUEST);
92+
} catch (OutdatedEtagException) {
93+
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);
94+
}
8595
}
8696

8797
/**
8898
* Reject a file
8999
*
90100
* @param int $fileId
91101
* @param string|null $message
102+
* @param string|null $etag
92103
* @return DataResponse
93104
*/
94105
#[NoAdminRequired]
95-
public function reject(int $fileId, ?string $message = ''): DataResponse {
96-
$this->approvalService->reject($fileId, $this->userId, $message);
97-
return new DataResponse(1);
106+
public function reject(int $fileId, ?string $message = '', ?string $etag = ''): DataResponse {
107+
try {
108+
if ($this->approvalService->reject($fileId, $this->userId, $message, $etag)) {
109+
return new DataResponse([]);
110+
}
111+
return new DataResponse([], Http::STATUS_BAD_REQUEST);
112+
} catch (OutdatedEtagException) {
113+
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);
114+
}
98115
}
99116

100117
/**

‎lib/Exceptions/OutdatedEtagException.php‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*/
7+
8+
namespace OCA\Approval\Exceptions;
9+
10+
use Exception;
11+
12+
class OutdatedEtagException extends Exception {
13+
}

‎lib/Service/ApprovalService.php‎

Lines changed: 25 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
use DateTime;
1111
use OCA\Approval\Activity\ActivityManager;
1212
use OCA\Approval\AppInfo\Application;
13+
use OCA\Approval\Exceptions\OutdatedEtagException;
1314
use OCA\DAV\Connector\Sabre\Node as SabreNode;
1415
use OCP\App\IAppManager;
1516
use OCP\Files\FileInfo;
@@ -19,15 +20,12 @@
1920
use OCP\IL10N;
2021
use OCP\IUser;
2122
use OCP\IUserManager;
22-
2323
use OCP\Notification\IManager as INotificationManager;
2424
use OCP\Share\IManager as IShareManager;
25-
2625
use OCP\Share\IShare;
2726
use OCP\SystemTag\ISystemTagObjectMapper;
2827
use OCP\SystemTag\TagNotFoundException;
2928
use Psr\Log\LoggerInterface;
30-
3129
use Sabre\DAV\INode;
3230
use Sabre\DAV\PropFind;
3331

@@ -270,6 +268,18 @@ public function getPendingNodes(string $userId, ?int $since = null): array {
270268
return $result;
271269
}
272270

271+
/**
272+
* @param int $fileId
273+
* @return string
274+
*/
275+
public function getEtag(int $fileId): string {
276+
$file = $this->root->getFirstNodeById($fileId);
277+
if ($file !== null) {
278+
return $file->getEtag();
279+
}
280+
return '';
281+
}
282+
273283
/**
274284
* Get approval state of a given file for a given user
275285
* @param int $fileId
@@ -339,10 +349,15 @@ public function getApprovalState(int $fileId, ?string $userId, bool $userHasAcce
339349
* @param int $fileId
340350
* @param string|null $userId
341351
* @param string $message
352+
* @param string $etag optional etag of the file to check if it has changed since approval was requested
342353
* @return bool success
354+
* @throws OutdatedEtagException
343355
*/
344-
public function approve(int $fileId, ?string $userId, string $message = ''): bool {
356+
public function approve(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool {
345357
$fileState = $this->getApprovalState($fileId, $userId);
358+
if ($etag !== '' && $etag !== $this->getEtag($fileId)) {
359+
throw new OutdatedEtagException();
360+
}
346361
// if file has pending tag and user is authorized to approve it
347362
if ($fileState['state'] === Application::STATE_APPROVABLE) {
348363
$rules = $this->ruleService->getRules();
@@ -377,10 +392,15 @@ public function approve(int $fileId, ?string $userId, string $message = ''): boo
377392
* @param int $fileId
378393
* @param string|null $userId
379394
* @param string $message
395+
* @param string $etag optional etag of the file to check if it has changed since approval was requested
380396
* @return bool success
397+
* @throws OutdatedEtagException
381398
*/
382-
public function reject(int $fileId, ?string $userId, string $message = ''): bool {
399+
public function reject(int $fileId, ?string $userId, string $message = '', string $etag = ''): bool {
383400
$fileState = $this->getApprovalState($fileId, $userId);
401+
if ($etag !== '' && $etag !== $this->getEtag($fileId)) {
402+
throw new OutdatedEtagException();
403+
}
384404
// if file has pending tag and user is authorized to approve it
385405
if ($fileState['state'] === Application::STATE_APPROVABLE) {
386406
$rules = $this->ruleService->getRules();

‎src/files/actions/approveAction.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export const approveAction = new FileAction({
2525
order: 0,
2626
async exec(node) {
2727
try {
28-
await approve(node.fileid, node.basename, node)
28+
await approve(node)
2929
} catch (error) {
3030
console.debug('Approve action failed')
3131
}
@@ -34,7 +34,7 @@ export const approveAction = new FileAction({
3434
async execBatch(nodes) {
3535
const promises = nodes
3636
.filter(node => node.attributes['approval-state'] === states.APPROVABLE)
37-
.map(node => approve(node.fileid, node.basename, node, false))
37+
.map(node => approve(node, false))
3838
const results = await Promise.allSettled(promises)
3939
return results.map(promise => promise.status === 'fulfilled')
4040
},

‎src/files/actions/rejectAction.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ export const rejectAction = new FileAction({
2525
order: 0,
2626
async exec(node) {
2727
try {
28-
await reject(node.fileid, node.basename, node)
28+
await reject(node)
2929
} catch (error) {
3030
console.debug('Reject action failed')
3131
}
@@ -34,7 +34,7 @@ export const rejectAction = new FileAction({
3434
async execBatch(nodes) {
3535
const promises = nodes
3636
.filter(node => node.attributes['approval-state'] === states.APPROVABLE)
37-
.map(node => reject(node.fileid, node.basename, node, false))
37+
.map(node => reject(node, false))
3838
const results = await Promise.allSettled(promises)
3939
return results.map(promise => promise.status === 'fulfilled')
4040
},

‎src/files/helpers.js‎

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -83,39 +83,37 @@ export async function requestAfterShareCreation(fileId, fileName, ruleId, node =
8383
}
8484
}
8585

86-
export async function approve(fileId, fileName, node = null, notify = true, message = '') {
87-
const url = generateOcsUrl('apps/approval/api/v1/approve/{fileId}', { fileId })
86+
export async function approve(node, notify = true, message = '', updateNode = true) {
87+
const url = generateOcsUrl('apps/approval/api/v1/approve/{fileId}', { fileId: node.fileid })
8888
try {
89-
await axios.put(url, { message })
89+
await axios.put(url, { message, etag: node.attributes.etag })
9090
if (notify) {
91-
showSuccess(t('approval', 'You approved {name}', { name: fileName }))
91+
showSuccess(t('approval', 'You approved {name}', { name: node.basename }))
9292
}
93-
if (node) {
93+
if (updateNode) {
9494
await updateNodeApprovalState(node)
9595
}
9696
} catch (error) {
9797
console.error(error)
9898
if (notify) {
99-
showError(t('approval', 'Failed to approve {name}', { name: fileName }))
99+
showError(error.response.data?.ocs?.data?.error ?? t('approval', 'Failed to approve {name}', { name: node.basename }))
100100
}
101101
throw error
102102
}
103103
}
104104

105-
export async function reject(fileId, fileName, node = null, notify = true, message = '') {
106-
const url = generateOcsUrl('apps/approval/api/v1/reject/{fileId}', { fileId })
105+
export async function reject(node, notify = true, message = '') {
106+
const url = generateOcsUrl('apps/approval/api/v1/reject/{fileId}', { fileId: node.fileid })
107107
try {
108-
await axios.put(url, { message })
108+
await axios.put(url, { message, etag: node.attributes.etag })
109109
if (notify) {
110-
showSuccess(t('approval', 'You rejected {name}', { name: fileName }))
111-
}
112-
if (node) {
113-
await updateNodeApprovalState(node)
110+
showSuccess(t('approval', 'You rejected {name}', { name: node.basename }))
114111
}
112+
await updateNodeApprovalState(node)
115113
} catch (error) {
116114
console.error(error)
117115
if (notify) {
118-
showError(t('approval', 'Failed to reject {name}', { name: fileName }))
116+
showError(error.response.data?.ocs?.data?.error ?? t('approval', 'Failed to reject {name}', { name: node.basename }))
119117
}
120118
throw error
121119
}

‎src/files/modals.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,10 @@ export function createInfoModal() {
3939
console.debug('[Approval] modal closed')
4040
},
4141
onApprove: (node, message) => {
42-
approve(node.fileid, node.basename, node, true, message)
42+
approve(node, true, message)
4343
},
4444
onReject: (node, message) => {
45-
reject(node.fileid, node.basename, node, true, message)
45+
reject(node, true, message)
4646
},
4747
onRequest: (node) => {
4848
onRequestFileAction(node)

‎src/views/ApprovalTab.vue‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,13 @@ export default {
5858
},
5959
6060
computed: {
61+
node() {
62+
const result = {}
63+
result.fileid = this.fileInfo.id
64+
result.basename = this.fileInfo.basename
65+
result.attributes = this.fileInfo.attributes
66+
return result
67+
},
6168
},
6269
6370
watch: {
@@ -93,16 +100,12 @@ export default {
93100
},
94101
async onApprove(message) {
95102
this.state = null
96-
const fileId = this.fileInfo.id
97-
const fileName = this.fileInfo.name
98-
await approve(fileId, fileName, null, true, message)
103+
await approve(this.node, true, message, false)
99104
this.update(this.fileInfo)
100105
},
101106
async onReject(message) {
102107
this.state = null
103-
const fileId = this.fileInfo.id
104-
const fileName = this.fileInfo.name
105-
await reject(fileId, fileName, null, true, message)
108+
await reject(this.node, true, message, false)
106109
this.update(this.fileInfo)
107110
},
108111
async onRequestSubmit(ruleId, createShares) {

0 commit comments

Comments
 (0)