feat: Account delegation#12727
Conversation
hamza221
added
enhancement
3. to review
skill:backend
hamza221
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
|
Copied over design review from the other pr NimishaAwesome! Looks really nice! Only some small changes. Overall:
In the "Delegation" modal:
In the "Add delegates" modal:
Laura
I was going off of form group components, not a dialog, in this context, I think the two different headings don't make sense either, we could just have one "Delegates" title :) LauraAdding to what nimishavijay said, the "Revoke" button should also probably not have an icon, a checkmark with danger styling is always confusing :) Additionally, the user selection dropdown should be full-width and there should be a bit more padding between the explanation text beneath and the buttons, also in the revoke access dialog. |
|
For provisioned accounts I would suggest to hide the ui option to create a delegation (and maybe even api wise) for now. We have currently two (soon 3) way how authentication works: 1) by using the password from the user session, 2) static password, 3) master password (which is similar to static password). Case 1: This needs the password of the owner and hence, delegating would only work if the user has been logged in once. That should be the case anyway, however this still seems a bit fragile (think of password change cases, etc). Case 2: Static password should work, but in the current version wa also use the middleware to update the password which probably wont work currently. Case 3: Master password should work but is currently not merged. Community PR that I took over #12442. It needs a bit of polishing and we wanted to move the password updating from the middleware to the factory. |
Done, It's enough to check if the account has a provisioning id right ? |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Implements an MVP for “account delegation” so a user can act on behalf of another user’s mail account, including backend delegation storage/lookup and frontend UI for managing delegates.
Changes:
- Adds delegation persistence and domain logic (DB table + mapper/entity + service + controller + routes).
- Updates multiple controllers to resolve an “effective user” via delegation when accessing accounts/mailboxes/messages/local messages/aliases.
- Adds frontend delegation modal + navigation entry and updates API/account payloads with an
isDelegatedflag; updates unit/integration tests accordingly.
Reviewed changes
Copilot reviewed 51 out of 52 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Service/DelegationServiceTest.php | Adds unit tests for delegation service operations and effective user resolution. |
| tests/Unit/Service/AccountServiceTest.php | Updates constructor wiring to include the new delegation mapper dependency. |
| tests/Unit/Controller/ThreadControllerTest.php | Adapts tests for ThreadController’s new DelegationService dependency and behavior. |
| tests/Unit/Controller/SieveControllerTest.php | Mocks delegation-based account user resolution for sieve endpoints. |
| tests/Unit/Controller/PageControllerTest.php | Updates expectations for delegated accounts + isDelegated field in initial state. |
| tests/Unit/Controller/OutboxControllerTest.php | Adds DelegationService dependency/mocks for delegated local message/account access. |
| tests/Unit/Controller/MessagesControllerTest.php | Adds DelegationService dependency/mocks for delegated message/mailbox access. |
| tests/Unit/Controller/MessageApiControllerTest.php | Adds DelegationService dependency/mocks for delegated account/message access. |
| tests/Unit/Controller/MailboxesControllerTest.php | Adds DelegationService dependency/mocks for delegated account/mailbox access. |
| tests/Unit/Controller/MailboxesApiControllerTest.php | Adds DelegationService dependency/mocks for delegated account/mailbox access. |
| tests/Unit/Controller/ListControllerTest.php | Mocks delegation-based message user resolution for list actions. |
| tests/Unit/Controller/DraftsControllerTest.php | Adds DelegationService dependency/mocks for delegated draft/local message access. |
| tests/Unit/Controller/DelegationControllerTest.php | Adds unit tests for new DelegationController endpoints and authorization paths. |
| tests/Unit/Controller/AliasesControllerTest.php | Adds DelegationService dependency/mocks for delegated alias/account access. |
| tests/Unit/Controller/AccountsControllerTest.php | Adds DelegationService dependency/mocks and expects delegated accounts list calls. |
| tests/Unit/Controller/AccountApiControllerTest.php | Adds DelegationService dependency and expects delegated accounts list calls. |
| tests/Integration/MailboxSynchronizationTest.php | Wires DelegationService into MailboxesController in integration setup. |
| src/service/DelegationService.js | Adds frontend API wrapper for delegation CRUD endpoints. |
| src/components/NavigationAccount.vue | Adds “Delegate account” action + modal trigger and delegation eligibility logic. |
| src/components/DelegationModal.vue | Adds UI to list/add/revoke delegates and user search integration. |
| src/components/AppSettingsMenu.vue | Separates delegated accounts in settings menu and filters normal account list. |
| lib/Service/DelegationService.php | Implements delegation CRUD and effective user resolution for account/mailbox/message/alias/local message. |
| lib/Service/AccountService.php | Adds fetching delegated accounts for the current user. |
| lib/ResponseDefinitions.php | Extends account list response typing with isDelegated. |
| lib/Migration/Version5008Date20260312152753.php | Adds mail_delegations table with unique index and FK to mail_accounts. |
| lib/Exception/DelegationExistsException.php | Adds a typed exception for existing delegations. |
| lib/Db/MessageMapper.php | Adds query method to get account id from message. |
| lib/Db/MailboxMapper.php | Adds query method to get account id from mailbox. |
| lib/Db/LocalMessageMapper.php | Adds query method to get account id from local message. |
| lib/Db/DelegationMapper.php | Adds DB access methods for delegation lookup and owner resolution. |
| lib/Db/Delegation.php | Adds delegation entity and JSON serialization. |
| lib/Db/AliasMapper.php | Adds query method to get account id from alias. |
| lib/Controller/ThreadController.php | Resolves effective user for thread actions (move/snooze/delete/summarize/etc.). |
| lib/Controller/SieveController.php | Resolves effective user for sieve operations. |
| lib/Controller/PageController.php | Adds delegated accounts to initial state / accounts JSON with isDelegated. |
| lib/Controller/OutboxController.php | Resolves effective user for outbox/draft/local message operations. |
| lib/Controller/MessagesController.php | Resolves effective user for mailbox/message operations + AI actions. |
| lib/Controller/MessageApiController.php | Resolves effective user for message API operations (send/get/raw/attachments). |
| lib/Controller/MailboxesController.php | Resolves effective user for mailbox operations (index/patch/sync/etc.). |
| lib/Controller/MailboxesApiController.php | Resolves effective user for mailboxes API list and listMessages endpoints. |
| lib/Controller/ListController.php | Resolves effective user for unsubscribe based on message id. |
| lib/Controller/FollowUpController.php | Resolves effective user for mailbox access when checking follow-up message ids. |
| lib/Controller/FilterController.php | Resolves effective user for filter get/update authorization. |
| lib/Controller/DraftsController.php | Resolves effective user for draft/local message operations. |
| lib/Controller/DelegationController.php | Adds API endpoints to list/add/remove delegates for an account. |
| lib/Controller/AliasesController.php | Resolves effective user for alias CRUD based on alias/account ownership/delegation. |
| lib/Controller/AccountsController.php | Adds delegated accounts to index and resolves effective user for account actions. |
| lib/Controller/AccountApiController.php | Returns owned + delegated accounts with isDelegated and aliases. |
| img/delegation.svg | Adds delegation icon asset for UI. |
| appinfo/routes.php | Registers delegation API routes. |
| appinfo/info.xml | Bumps app version. |
| REUSE.toml | Adds new SVG asset to REUSE annotations. |
Comments suppressed due to low confidence (4)
src/components/DelegationModal.vue:1
- The modal uses
this.account.accountIdfor API calls, but other components (e.g., NavigationAccount) treat the account identifier asaccount.id. If the passedaccountobject doesn’t containaccountId, these calls will sendundefinedto the backend routes and fail. Align on a single field (likelyaccount.id) or adapt the prop shape so the ID is consistently available.
src/components/DelegationModal.vue:1 - The modal uses
this.account.accountIdfor API calls, but other components (e.g., NavigationAccount) treat the account identifier asaccount.id. If the passedaccountobject doesn’t containaccountId, these calls will sendundefinedto the backend routes and fail. Align on a single field (likelyaccount.id) or adapt the prop shape so the ID is consistently available.
src/components/DelegationModal.vue:1 - The modal uses
this.account.accountIdfor API calls, but other components (e.g., NavigationAccount) treat the account identifier asaccount.id. If the passedaccountobject doesn’t containaccountId, these calls will sendundefinedto the backend routes and fail. Align on a single field (likelyaccount.id) or adapt the prop shape so the ID is consistently available.
lib/Service/DelegationService.php:1 - This is subject to a race: two concurrent requests can both pass
find()(non-existent) and then one insert will violate the unique index. If the insert throws a DB exception, it will likely surface as a 500. Prefer relying on the DB uniqueness and catching the unique-constraint violation on insert (mapping it toDelegationExistsException), or keep the existence check but still handle insert-time uniqueness violations.
|
@copilot Any undesired behaviors or bugs introduced by this PR to the SMIME encryption feature |
Yes, there is one S/MIME bug introduced by the delegation changes. Bug: In Fixed in commit 65bca97 — replaced Note: S/MIME decryption when reading messages works correctly — |
hamza221
force-pushed
the
feat/delegation-mvp
branch
from
65bca97 to
99884ca
Compare
|
resolved conflicts, proceeding with the reviews now |
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
…context Agent-Logs-Url: https://github.com/nextcloud/mail/sessions/caf30cdf-84f3-48b1-81a8-853a9ce58ded Co-authored-by: hamza221 <40746210+hamza221@users.noreply.github.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
kesselb
force-pushed
the
feat/delegation-mvp
branch
from
e91d8d4 to
caa529d
Compare
|
For the final commit, I would prefer to have one "feat: Account delegation". If you want to keep the current structure, please use the right types.
|
I'll squash into one commit once ready |
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds mailbox delegation: a DB migration and Delegation entity, mappers, a DelegationService with resolution helpers, three REST endpoints and DelegationController, delegation-aware wiring across Mail controllers, frontend modal and JS service to manage delegates, updated response types, updated REUSE metadata and version, and unit/integration tests. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
| $effectiveUserId = $this->delegationService->resolveAccountUserId($id, $this->currentUserId); | ||
| $account = $this->accountService->find($effectiveUserId, $id); |
| $effectiveUserId = $this->delegationService->resolveAccountUserId($accountId, $this->currentUserId); | ||
| $account = $this->accountService->findById($accountId); | ||
|
|
||
| if ($account->getUserId() !== $this->currentUserId) { | ||
| if ($account->getUserId() !== $effectiveUserId) { | ||
| return new JSONResponse([], Http::STATUS_NOT_FOUND); |
| public function delegate(int $accountId, string $userId): Delegation { | ||
| try { | ||
| $this->delegationMapper->find($accountId, $userId); | ||
| throw new DelegationExistsException("Delegation already exists for account $accountId and user $userId"); | ||
| } catch (DoesNotExistException) { | ||
| // delegation doesn't exist, continue | ||
| } | ||
|
|
||
| $delegation = new Delegation(); | ||
| $delegation->setAccountId($accountId); | ||
| $delegation->setUserId($userId); | ||
| return $this->delegationMapper->insert($delegation); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/Controller/ThreadController.php (1)
5-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLicense header uses
AGPL-3.0-onlyinstead ofAGPL-3.0-or-later.The SPDX license identifier should be
AGPL-3.0-or-laterper coding guidelines.Proposed fix
/** * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-only + * SPDX-License-Identifier: AGPL-3.0-or-later */As per coding guidelines: "New files must use
AGPL-3.0-or-laterlicense identifier, neverAGPL-3.0-only".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/Controller/ThreadController.php` around lines 5 - 8, The SPDX license header at the top of ThreadController.php uses "AGPL-3.0-only"; update the SPDX-License-Identifier token to "AGPL-3.0-or-later" in the file header (the comment block above the ThreadController class) so the file conforms to the project's license identifier guideline.lib/Controller/SieveController.php (1)
5-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLicense header uses
AGPL-3.0-onlyinstead ofAGPL-3.0-or-later.The SPDX license identifier should be
AGPL-3.0-or-laterper coding guidelines.Proposed fix
/** * SPDX-FileCopyrightText: 2021 Nextcloud GmbH and Nextcloud contributors - * SPDX-License-Identifier: AGPL-3.0-only + * SPDX-License-Identifier: AGPL-3.0-or-later */As per coding guidelines: "New files must use
AGPL-3.0-or-laterlicense identifier, neverAGPL-3.0-only".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/Controller/SieveController.php` around lines 5 - 8, Update the SPDX license identifier in the file header of SieveController.php: replace "SPDX-License-Identifier: AGPL-3.0-only" with "SPDX-License-Identifier: AGPL-3.0-or-later" in the top comment block so the file uses the required AGPL-3.0-or-later identifier.
🧹 Nitpick comments (5)
src/components/AccountSettings.vue (1)
109-109: ⚡ Quick winGuard
scrollToAccountSettingswhenAccountFormcan be absent.With Line 109 hiding the mail-server section for delegated accounts,
this.$refs.accountFormmay be undefined whenrename-primary-aliasis emitted. Add a guard before dereferencing.Suggested fix
scrollToAccountSettings() { - this.$refs.accountForm.$el.scrollIntoView({ + const accountForm = this.$refs.accountForm + if (!accountForm?.$el) { + return + } + accountForm.$el.scrollIntoView({ behavior: 'smooth', }) },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AccountSettings.vue` at line 109, The handler for the rename-primary-alias event calls scrollToAccountSettings which dereferences this.$refs.accountForm that can be undefined when the mail-server section is hidden; update the event handler (where rename-primary-alias is handled) to guard before accessing this.$refs.accountForm (or calling scrollToAccountSettings) by checking that this.$refs.accountForm exists (or that accountForm ref is present) and only then invoking scrollToAccountSettings, ensuring no direct dereference of undefined AccountForm.tests/Unit/Controller/AccountsControllerTest.php (1)
154-157: ⚡ Quick winAdd a positive delegated-account case in
testIndex.Line 154 currently validates only the empty delegated list path. Please add one case where
findDelegatedAccountsreturns at least one delegated account and assertisDelegatedistrue, so regressions in delegated serialization don’t slip through.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Controller/AccountsControllerTest.php` around lines 154 - 157, Add a positive delegated-account path inside the testIndex unit test: mock AccountService::findDelegatedAccounts to return a non-empty array (e.g., one delegated account) for $this->userId, call the controller/index flow as in the existing empty-case, and add assertions that the serialized response marks that account with isDelegated === true; update the mock expectation for findDelegatedAccounts (or add a second expectation) and assert the output contains the delegated account and its isDelegated flag is true to prevent regression in delegated serialization.lib/Controller/PageController.php (1)
187-205: ⚡ Quick winConsider consolidating account transformation logic.
The account-to-JSON transformation logic for delegated accounts (lines 187-205) largely duplicates the owned account transformation (lines 169-186). Consider extracting this shared logic into a private helper method or a service to reduce duplication and improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/Controller/PageController.php` around lines 187 - 205, The delegated-account loop duplicates the owned-account JSON transformation; extract the shared logic into a private helper like transformAccountToJson($account, bool $isDelegated = false) and call it from both places (use current usages of $delegatedAccount->jsonSerialize(), aliasesService->findAll(...), mailManager->getMailboxes(...), and logger handling inside the helper). The helper should accept an Account object, set isDelegated accordingly, populate aliases via $this->aliasesService->findAll($account->getId(), $account->getUserId()), attempt to load mailboxes with $this->mailManager->getMailboxes($account) inside the same try/catch that logs using $this->logger and sets mailboxes/error defaults, and return the assembled array so callers (both owned and delegated loops) push the returned array into $accountsJson.lib/Controller/DelegationController.php (1)
50-52: 💤 Low valueConsider using HTTP 403 FORBIDDEN instead of 401 UNAUTHORIZED for authorization failures.
When a user is authenticated but attempts to access another user's account, HTTP 403 FORBIDDEN is more semantically correct than 401 UNAUTHORIZED:
- 401 UNAUTHORIZED: Authentication required (no valid credentials)
- 403 FORBIDDEN: Authenticated but not authorized for this resource
The user has a valid session (
$this->currentUserIdis set via injection), they're just not the owner of the account.Suggested change
if ($account->getUserId() !== $this->currentUserId) { - return new JSONResponse([], Http::STATUS_UNAUTHORIZED); + return new JSONResponse([], Http::STATUS_FORBIDDEN); }Also applies to: 72-74, 110-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/Controller/DelegationController.php` around lines 50 - 52, Replace the 401 responses used for authorization failures with 403 FORBIDDEN: locate the checks in DelegationController where you compare $account->getUserId() !== $this->currentUserId (and the analogous checks at the other occurrences referenced) and change the returned JSONResponse from Http::STATUS_UNAUTHORIZED to Http::STATUS_FORBIDDEN so authenticated-but-unauthorized access returns 403; keep the response body unchanged.tests/Unit/Service/DelegationServiceTest.php (1)
41-44: ⚡ Quick winRemove unused test fixtures.
The test declares and initializes mocks for
IUserManager,IManager(notification manager),ITimeFactory, andIEventDispatcher(lines 41-44, 58-61), and defines amockNotification()helper (lines 79-94) that's called in two tests (lines 97, 156). However,DelegationService's constructor (lines 63-70 in the service file) does not accept any of these dependencies—it only injects the mapper collaborators. These mocks are never used and represent dead test code.🧹 Proposed cleanup
Remove the unused mock properties, their initialization in setUp(), and the mockNotification() calls:
class DelegationServiceTest extends TestCase { private DelegationMapper&MockObject $delegationMapper; private AccountService&MockObject $accountService; private MailboxMapper&MockObject $mailboxMapper; private MessageMapper&MockObject $messageMapper; private AliasMapper&MockObject $aliasMapper; private LocalMessageMapper&MockObject $localMessageMapper; - private IUserManager&MockObject $userManager; - private IManager&MockObject $notificationManager; - private ITimeFactory&MockObject $timeFactory; - private IEventDispatcher&MockObject $eventDispatcher; private DelegationService $service; protected function setUp(): void { parent::setUp(); $this->delegationMapper = $this->createMock(DelegationMapper::class); $this->accountService = $this->createMock(AccountService::class); $this->mailboxMapper = $this->createMock(MailboxMapper::class); $this->messageMapper = $this->createMock(MessageMapper::class); $this->aliasMapper = $this->createMock(AliasMapper::class); $this->localMessageMapper = $this->createMock(LocalMessageMapper::class); - $this->userManager = $this->createMock(IUserManager::class); - $this->notificationManager = $this->createMock(IManager::class); - $this->timeFactory = $this->createMock(ITimeFactory::class); - $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->service = new DelegationService( $this->delegationMapper, $this->accountService, $this->mailboxMapper, $this->messageMapper, $this->aliasMapper, $this->localMessageMapper, ); ... } - private function mockNotification(): void { - ... - } public function testDelegateSuccess(): void { - $this->mockNotification(); ... } public function testUnDelegateSuccess(): void { - $this->mockNotification(); ... }Also remove the unused imports (lines 26-31 in the test file).
Also applies to: 58-61, 79-94, 97-97, 156-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Service/DelegationServiceTest.php` around lines 41 - 44, Remove the dead test fixtures: delete the mock properties for IUserManager, IManager (notification manager), ITimeFactory, and IEventDispatcher, remove their initialization in setUp(), and remove the mockNotification() helper and its calls in the two tests; also drop the corresponding unused imports. Verify that only the mapper collaborator mocks (the dependencies actually accepted by DelegationService's constructor) remain and run tests to ensure nothing else references those symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/Controller/AccountsController.php`:
- Around line 128-131: The delegationService->resolveAccountUserId call in show
(and in patchAccount, updateSignature, draft, getQuota, updateSmimeCertificate,
testAccountConnection) can throw DoesNotExistException and currently isn't
handled; wrap the resolveAccountUserId($id, $this->currentUserId) invocation in
a try-catch that catches DoesNotExistException and returns an HTTP 403 JSON
response (matching MailboxesController's behavior), otherwise proceed to call
accountService->find (or the existing logic) with the resolved $effectiveUserId;
apply this same try-catch pattern to the listed methods (show, patchAccount,
updateSignature, draft, getQuota, updateSmimeCertificate, testAccountConnection)
to consistently handle delegation resolution failures.
In `@lib/Controller/AliasesController.php`:
- Around line 48-51: AliasesController's index (and other methods: update,
destroy, create, updateSignature) call
$this->delegationService->resolveAccountUserId($accountId, $this->currentUserId)
without catching DoesNotExistException, so delegation/account-not-found errors
propagate; update each method (index, update, destroy, create, updateSignature)
to wrap the resolveAccountUserId call in a try/catch that catches
DoesNotExistException and returns the same HTTP 403 response used in
MessagesController/MailboxesController (i.e., catch DoesNotExistException and
return a JSON/HTTP 403 response), keeping the rest of the method flow unchanged
when resolution succeeds.
In `@lib/Controller/OutboxController.php`:
- Around line 67-70: The show method calls
delegationService->resolveLocalMessageUserId($id, $this->userId) without
handling a DoesNotExistException; wrap that call in a try-catch that catches
DoesNotExistException (the same pattern used in
MessagesController/MailboxesController) and return an appropriate JsonResponse
(e.g., JsonResponse::notFound or JsonResponse::error with 404) when caught, then
proceed to call service->getMessage($id, $effectiveUserId) only if resolution
succeeds; apply the same try-catch pattern to other delegation resolution calls
in this controller.
In `@lib/Db/DelegationMapper.php`:
- Around line 52-70: In findAccountOwnerForDelegatedUser remove the incorrect
type argument passed to the column-to-column comparison in the join: the call to
$qb->expr()->eq('d.account_id', 'a.id', IQueryBuilder::PARAM_INT) should be
changed to a plain column comparison (no PARAM_* type) so expr()->eq compares
two columns; update the join invocation accordingly (leave other usages of
createNamedParameter with PARAM_INT unchanged).
In `@src/components/DelegationModal.vue`:
- Around line 268-281: The revokeDelegate method currently updates delegates and
shows success but does not close the modal; after a successful revoke (inside
revokeDelegate, after the this.delegates filter and the showSuccess call) call
this.closeDialog() to dismiss the 'revoke' dialog and return the user to the
main view; ensure you call this.closeDialog() only in the try path (not in
catch) and keep existing error logging in revokeDelegate unchanged.
---
Outside diff comments:
In `@lib/Controller/SieveController.php`:
- Around line 5-8: Update the SPDX license identifier in the file header of
SieveController.php: replace "SPDX-License-Identifier: AGPL-3.0-only" with
"SPDX-License-Identifier: AGPL-3.0-or-later" in the top comment block so the
file uses the required AGPL-3.0-or-later identifier.
In `@lib/Controller/ThreadController.php`:
- Around line 5-8: The SPDX license header at the top of ThreadController.php
uses "AGPL-3.0-only"; update the SPDX-License-Identifier token to
"AGPL-3.0-or-later" in the file header (the comment block above the
ThreadController class) so the file conforms to the project's license identifier
guideline.
---
Nitpick comments:
In `@lib/Controller/DelegationController.php`:
- Around line 50-52: Replace the 401 responses used for authorization failures
with 403 FORBIDDEN: locate the checks in DelegationController where you compare
$account->getUserId() !== $this->currentUserId (and the analogous checks at the
other occurrences referenced) and change the returned JSONResponse from
Http::STATUS_UNAUTHORIZED to Http::STATUS_FORBIDDEN so
authenticated-but-unauthorized access returns 403; keep the response body
unchanged.
In `@lib/Controller/PageController.php`:
- Around line 187-205: The delegated-account loop duplicates the owned-account
JSON transformation; extract the shared logic into a private helper like
transformAccountToJson($account, bool $isDelegated = false) and call it from
both places (use current usages of $delegatedAccount->jsonSerialize(),
aliasesService->findAll(...), mailManager->getMailboxes(...), and logger
handling inside the helper). The helper should accept an Account object, set
isDelegated accordingly, populate aliases via
$this->aliasesService->findAll($account->getId(), $account->getUserId()),
attempt to load mailboxes with $this->mailManager->getMailboxes($account) inside
the same try/catch that logs using $this->logger and sets mailboxes/error
defaults, and return the assembled array so callers (both owned and delegated
loops) push the returned array into $accountsJson.
In `@src/components/AccountSettings.vue`:
- Line 109: The handler for the rename-primary-alias event calls
scrollToAccountSettings which dereferences this.$refs.accountForm that can be
undefined when the mail-server section is hidden; update the event handler
(where rename-primary-alias is handled) to guard before accessing
this.$refs.accountForm (or calling scrollToAccountSettings) by checking that
this.$refs.accountForm exists (or that accountForm ref is present) and only then
invoking scrollToAccountSettings, ensuring no direct dereference of undefined
AccountForm.
In `@tests/Unit/Controller/AccountsControllerTest.php`:
- Around line 154-157: Add a positive delegated-account path inside the
testIndex unit test: mock AccountService::findDelegatedAccounts to return a
non-empty array (e.g., one delegated account) for $this->userId, call the
controller/index flow as in the existing empty-case, and add assertions that the
serialized response marks that account with isDelegated === true; update the
mock expectation for findDelegatedAccounts (or add a second expectation) and
assert the output contains the delegated account and its isDelegated flag is
true to prevent regression in delegated serialization.
In `@tests/Unit/Service/DelegationServiceTest.php`:
- Around line 41-44: Remove the dead test fixtures: delete the mock properties
for IUserManager, IManager (notification manager), ITimeFactory, and
IEventDispatcher, remove their initialization in setUp(), and remove the
mockNotification() helper and its calls in the two tests; also drop the
corresponding unused imports. Verify that only the mapper collaborator mocks
(the dependencies actually accepted by DelegationService's constructor) remain
and run tests to ensure nothing else references those symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 994d9c90-0537-4357-ab4e-fa4d54acd0a8
⛔ Files ignored due to path filters (1)
img/delegation.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
REUSE.tomlappinfo/info.xmlappinfo/routes.phplib/Controller/AccountApiController.phplib/Controller/AccountsController.phplib/Controller/AliasesController.phplib/Controller/DelegationController.phplib/Controller/DraftsController.phplib/Controller/FilterController.phplib/Controller/FollowUpController.phplib/Controller/ListController.phplib/Controller/MailboxesApiController.phplib/Controller/MailboxesController.phplib/Controller/MessageApiController.phplib/Controller/MessagesController.phplib/Controller/OutboxController.phplib/Controller/PageController.phplib/Controller/SieveController.phplib/Controller/ThreadController.phplib/Db/AliasMapper.phplib/Db/Delegation.phplib/Db/DelegationMapper.phplib/Db/LocalMessageMapper.phplib/Db/MailAccountMapper.phplib/Db/MailboxMapper.phplib/Db/MessageMapper.phplib/Exception/DelegationExistsException.phplib/Migration/Version5008Date20260312152753.phplib/ResponseDefinitions.phplib/Service/AccountService.phplib/Service/DelegationService.phpsrc/components/AccountSettings.vuesrc/components/AppSettingsMenu.vuesrc/components/DelegationModal.vuesrc/components/NavigationAccount.vuesrc/service/DelegationService.jstests/Integration/MailboxSynchronizationTest.phptests/Unit/Controller/AccountApiControllerTest.phptests/Unit/Controller/AccountsControllerTest.phptests/Unit/Controller/AliasesControllerTest.phptests/Unit/Controller/DelegationControllerTest.phptests/Unit/Controller/DraftsControllerTest.phptests/Unit/Controller/ListControllerTest.phptests/Unit/Controller/MailboxesApiControllerTest.phptests/Unit/Controller/MailboxesControllerTest.phptests/Unit/Controller/MessageApiControllerTest.phptests/Unit/Controller/MessagesControllerTest.phptests/Unit/Controller/OutboxControllerTest.phptests/Unit/Controller/PageControllerTest.phptests/Unit/Controller/SieveControllerTest.phptests/Unit/Controller/ThreadControllerTest.phptests/Unit/Service/AccountServiceTest.phptests/Unit/Service/DelegationServiceTest.php
| public function show(int $id): JSONResponse { | ||
| return new JSONResponse($this->accountService->find($this->currentUserId, $id)); | ||
| $effectiveUserId = $this->delegationService->resolveAccountUserId($id, $this->currentUserId); | ||
| return new JSONResponse($this->accountService->find($effectiveUserId, $id)); | ||
| } |
There was a problem hiding this comment.
Missing DoesNotExistException handling for delegation resolution.
The show method and several others (patchAccount, updateSignature, draft, getQuota, updateSmimeCertificate, testAccountConnection) call resolveAccountUserId without catching DoesNotExistException, unlike MailboxesController which wraps these calls in try-catch blocks returning HTTP 403.
Proposed fix for `show` method
#[TrapError]
public function show(int $id): JSONResponse {
+ try {
$effectiveUserId = $this->delegationService->resolveAccountUserId($id, $this->currentUserId);
+ } catch (DoesNotExistException $e) {
+ return new JSONResponse([], Http::STATUS_FORBIDDEN);
+ }
return new JSONResponse($this->accountService->find($effectiveUserId, $id));
}Apply similar pattern to other affected methods.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function show(int $id): JSONResponse { | |
| return new JSONResponse($this->accountService->find($this->currentUserId, $id)); | |
| $effectiveUserId = $this->delegationService->resolveAccountUserId($id, $this->currentUserId); | |
| return new JSONResponse($this->accountService->find($effectiveUserId, $id)); | |
| } | |
| public function show(int $id): JSONResponse { | |
| try { | |
| $effectiveUserId = $this->delegationService->resolveAccountUserId($id, $this->currentUserId); | |
| } catch (DoesNotExistException $e) { | |
| return new JSONResponse([], Http::STATUS_FORBIDDEN); | |
| } | |
| return new JSONResponse($this->accountService->find($effectiveUserId, $id)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/Controller/AccountsController.php` around lines 128 - 131, The
delegationService->resolveAccountUserId call in show (and in patchAccount,
updateSignature, draft, getQuota, updateSmimeCertificate, testAccountConnection)
can throw DoesNotExistException and currently isn't handled; wrap the
resolveAccountUserId($id, $this->currentUserId) invocation in a try-catch that
catches DoesNotExistException and returns an HTTP 403 JSON response (matching
MailboxesController's behavior), otherwise proceed to call accountService->find
(or the existing logic) with the resolved $effectiveUserId; apply this same
try-catch pattern to the listed methods (show, patchAccount, updateSignature,
draft, getQuota, updateSmimeCertificate, testAccountConnection) to consistently
handle delegation resolution failures.
| public function index(int $accountId): JSONResponse { | ||
| return new JSONResponse($this->aliasService->findAll($accountId, $this->currentUserId)); | ||
| $effectiveUserId = $this->delegationService->resolveAccountUserId($accountId, $this->currentUserId); | ||
| return new JSONResponse($this->aliasService->findAll($accountId, $effectiveUserId)); | ||
| } |
There was a problem hiding this comment.
Missing DoesNotExistException handling for delegation resolution.
Unlike MessagesController and MailboxesController, this controller does not catch DoesNotExistException from the delegation resolution calls. If the delegation doesn't exist or the account is not found, this will propagate as an unhandled exception rather than returning HTTP 403.
While #[TrapError] may catch generic exceptions, the pattern in other controllers explicitly handles this case for consistent error responses.
Proposed fix
#[TrapError]
public function index(int $accountId): JSONResponse {
+ try {
$effectiveUserId = $this->delegationService->resolveAccountUserId($accountId, $this->currentUserId);
+ } catch (DoesNotExistException $e) {
+ return new JSONResponse([], Http::STATUS_FORBIDDEN);
+ }
return new JSONResponse($this->aliasService->findAll($accountId, $effectiveUserId));
}Apply similar pattern to update, destroy, create, and updateSignature methods.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/Controller/AliasesController.php` around lines 48 - 51,
AliasesController's index (and other methods: update, destroy, create,
updateSignature) call $this->delegationService->resolveAccountUserId($accountId,
$this->currentUserId) without catching DoesNotExistException, so
delegation/account-not-found errors propagate; update each method (index,
update, destroy, create, updateSignature) to wrap the resolveAccountUserId call
in a try/catch that catches DoesNotExistException and returns the same HTTP 403
response used in MessagesController/MailboxesController (i.e., catch
DoesNotExistException and return a JSON/HTTP 403 response), keeping the rest of
the method flow unchanged when resolution succeeds.
| public function show(int $id): JsonResponse { | ||
| $message = $this->service->getMessage($id, $this->userId); | ||
| $effectiveUserId = $this->delegationService->resolveLocalMessageUserId($id, $this->userId); | ||
| $message = $this->service->getMessage($id, $effectiveUserId); | ||
| return JsonResponse::success($message); |
There was a problem hiding this comment.
Missing DoesNotExistException handling for delegation resolution.
Same issue as AliasesController — the delegation resolution calls (resolveLocalMessageUserId, resolveAccountUserId) are not wrapped in try-catch blocks. This is inconsistent with the pattern established in MessagesController and MailboxesController.
Proposed fix for `show` method (apply similar pattern to other methods)
#[TrapError]
public function show(int $id): JsonResponse {
+ try {
$effectiveUserId = $this->delegationService->resolveLocalMessageUserId($id, $this->userId);
+ } catch (DoesNotExistException $e) {
+ return JsonResponse::error('Not found', Http::STATUS_FORBIDDEN);
+ }
$message = $this->service->getMessage($id, $effectiveUserId);
return JsonResponse::success($message);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function show(int $id): JsonResponse { | |
| $message = $this->service->getMessage($id, $this->userId); | |
| $effectiveUserId = $this->delegationService->resolveLocalMessageUserId($id, $this->userId); | |
| $message = $this->service->getMessage($id, $effectiveUserId); | |
| return JsonResponse::success($message); | |
| #[TrapError] | |
| public function show(int $id): JsonResponse { | |
| try { | |
| $effectiveUserId = $this->delegationService->resolveLocalMessageUserId($id, $this->userId); | |
| } catch (DoesNotExistException $e) { | |
| return JsonResponse::error('Not found', Http::STATUS_FORBIDDEN); | |
| } | |
| $message = $this->service->getMessage($id, $effectiveUserId); | |
| return JsonResponse::success($message); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/Controller/OutboxController.php` around lines 67 - 70, The show method
calls delegationService->resolveLocalMessageUserId($id, $this->userId) without
handling a DoesNotExistException; wrap that call in a try-catch that catches
DoesNotExistException (the same pattern used in
MessagesController/MailboxesController) and return an appropriate JsonResponse
(e.g., JsonResponse::notFound or JsonResponse::error with 404) when caught, then
proceed to call service->getMessage($id, $effectiveUserId) only if resolution
succeeds; apply the same try-catch pattern to other delegation resolution calls
in this controller.
| public function findAccountOwnerForDelegatedUser(int $accountId, string $delegatedUserId): string { | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->select('a.user_id') | ||
| ->from($this->getTableName(), 'd') | ||
| ->join('d', 'mail_accounts', 'a', | ||
| $qb->expr()->eq('d.account_id', 'a.id', IQueryBuilder::PARAM_INT)) | ||
| ->where($qb->expr()->eq('d.account_id', $qb->createNamedParameter($accountId, IQueryBuilder::PARAM_INT))) | ||
| ->andWhere($qb->expr()->eq('d.user_id', $qb->createNamedParameter($delegatedUserId))); | ||
|
|
||
| $result = $qb->executeQuery(); | ||
| $row = $result->fetch(); | ||
| $result->closeCursor(); | ||
|
|
||
| if ($row === false) { | ||
| throw new DoesNotExistException("No delegation found for account $accountId and user $delegatedUserId"); | ||
| } | ||
|
|
||
| return (string)$row['user_id']; | ||
| } |
There was a problem hiding this comment.
Remove incorrect parameter type from column-to-column comparison.
Line 57 passes IQueryBuilder::PARAM_INT as the third argument to eq() when joining two columns. The type parameter in expr()->eq() is only for comparing columns to literal values or bound parameters, not for column-to-column comparisons. This could cause unexpected query behavior.
🔧 Proposed fix
$qb->select('a.user_id')
->from($this->getTableName(), 'd')
->join('d', 'mail_accounts', 'a',
- $qb->expr()->eq('d.account_id', 'a.id', IQueryBuilder::PARAM_INT))
+ $qb->expr()->eq('d.account_id', 'a.id'))
->where($qb->expr()->eq('d.account_id', $qb->createNamedParameter($accountId, IQueryBuilder::PARAM_INT)))
->andWhere($qb->expr()->eq('d.user_id', $qb->createNamedParameter($delegatedUserId)));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/Db/DelegationMapper.php` around lines 52 - 70, In
findAccountOwnerForDelegatedUser remove the incorrect type argument passed to
the column-to-column comparison in the join: the call to
$qb->expr()->eq('d.account_id', 'a.id', IQueryBuilder::PARAM_INT) should be
changed to a plain column comparison (no PARAM_* type) so expr()->eq compares
two columns; update the join invocation accordingly (leave other usages of
createNamedParameter with PARAM_INT unchanged).
| async revokeDelegate() { | ||
| if (!this.revokeUser) { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| await unDelegate(this.account.accountId, this.revokeUser.userId) | ||
| this.delegates = this.delegates.filter((d) => d.userId !== this.revokeUser.userId) | ||
| showSuccess(t('mail', 'Revoked access for {userId}', { userId: this.revokeUser.userId })) | ||
| } catch (error) { | ||
| logger.error('Could not revoke delegation', { error }) | ||
| showError(t('mail', 'Could not revoke delegation')) | ||
| } | ||
| }, |
There was a problem hiding this comment.
Missing closeDialog() call after successful revoke.
After successfully revoking delegation, the view remains on the 'revoke' dialog. The user should be returned to the main view.
Proposed fix
async revokeDelegate() {
if (!this.revokeUser) {
return
}
try {
await unDelegate(this.account.accountId, this.revokeUser.userId)
this.delegates = this.delegates.filter((d) => d.userId !== this.revokeUser.userId)
showSuccess(t('mail', 'Revoked access for {userId}', { userId: this.revokeUser.userId }))
+ this.closeDialog()
} catch (error) {
logger.error('Could not revoke delegation', { error })
showError(t('mail', 'Could not revoke delegation'))
}
+ this.revokeUser = null
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async revokeDelegate() { | |
| if (!this.revokeUser) { | |
| return | |
| } | |
| try { | |
| await unDelegate(this.account.accountId, this.revokeUser.userId) | |
| this.delegates = this.delegates.filter((d) => d.userId !== this.revokeUser.userId) | |
| showSuccess(t('mail', 'Revoked access for {userId}', { userId: this.revokeUser.userId })) | |
| } catch (error) { | |
| logger.error('Could not revoke delegation', { error }) | |
| showError(t('mail', 'Could not revoke delegation')) | |
| } | |
| }, | |
| async revokeDelegate() { | |
| if (!this.revokeUser) { | |
| return | |
| } | |
| try { | |
| await unDelegate(this.account.accountId, this.revokeUser.userId) | |
| this.delegates = this.delegates.filter((d) => d.userId !== this.revokeUser.userId) | |
| showSuccess(t('mail', 'Revoked access for {userId}', { userId: this.revokeUser.userId })) | |
| this.closeDialog() | |
| } catch (error) { | |
| logger.error('Could not revoke delegation', { error }) | |
| showError(t('mail', 'Could not revoke delegation')) | |
| } | |
| this.revokeUser = null | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/DelegationModal.vue` around lines 268 - 281, The
revokeDelegate method currently updates delegates and shows success but does not
close the modal; after a successful revoke (inside revokeDelegate, after the
this.delegates filter and the showSuccess call) call this.closeDialog() to
dismiss the 'revoke' dialog and return the user to the main view; ensure you
call this.closeDialog() only in the try path (not in catch) and keep existing
error logging in revokeDelegate unchanged.
|
@claude could you review that one please, just to make copilot and coderrabbitai didn't miss anything? Please use mythos if possible. |
Rabbit was autorun 😅 |
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/Controller/AccountApiController.php`:
- Around line 64-66: Remove the unused local variable $ownerUserId in the
array_map callback that builds $delegatedList: inside the closure used in
array_map (mapping Account objects to transformed data via
transformAccountList), delete the line assigning $ownerUserId =
$account->getUserId() so the closure only calls
$this->transformAccountList($account, true); this removes the PHPMD warning
while preserving behavior.
In `@src/components/DelegationModal.vue`:
- Around line 260-271: The current finally block always calls closeDialog(),
which closes the modal even on delegation errors and leaves transient state
(e.g., selectedUser, user suggestions) intact for a stale resubmit; change the
flow so that closeDialog() is only invoked on success (after showSuccess) and
not in the finally that runs on errors, and add explicit state cleanup inside
the closeDialog() or a new reset method to clear transient fields (reset
selectedUser, suggestion list, and any temporary flags) so reopening the
DelegationModal shows a fresh form; apply the same change to the other
delegation block referenced (around the delegation call at lines ~289-291) and
ensure delegating is still set to false in finally.
- Around line 13-15: The modal title currently renders t('mail', 'Delegation')
in the h2 with id "delegation-modal-title"; change it to use the same phrasing
as the rest of the flow (e.g., t('mail', 'Add delegate') or t('mail', 'Delegate
access') to match surrounding buttons/labels), update or add the corresponding
translation key in the i18n resource, and ensure any tests or snapshots
referencing "Delegation" are updated to the new string; locate the title in
DelegationModal.vue (h2 id="delegation-modal-title") to make the change.
- Around line 1-4: The SPDX header in the comment block uses extra bullet
prefixes; update the header comment so it exactly matches the repository's
required SPDX lines by removing the "- " prefixes and leaving two comment lines:
"SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors" and
"SPDX-Identifier: AGPL-3.0-or-later" (i.e., modify the comment at the top of
DelegationModal.vue to contain those exact SPDX keys/values).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c92ab44b-fcf6-4d38-b725-ff594bab75df
📒 Files selected for processing (3)
lib/Controller/AccountApiController.phplib/Service/DelegationService.phpsrc/components/DelegationModal.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/Service/DelegationService.php
| <!-- | ||
| - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| - SPDX-License-Identifier: AGPL-3.0-or-later | ||
| --> |
There was a problem hiding this comment.
Use the repository’s exact SPDX header format.
This header is close, but the repo rule for new src/**/*.vue files requires the exact SPDX keys and values without the bullet prefixes. As written, this can slip past automated license checks.
Proposed fix
<!--
- - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
- - SPDX-License-Identifier: AGPL-3.0-or-later
+SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+SPDX-Identifier: AGPL-3.0-or-later
-->As per coding guidelines, "SPDX license header format must be: SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors and SPDX-Identifier: AGPL-3.0-or-later"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- | |
| - SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | |
| - SPDX-License-Identifier: AGPL-3.0-or-later | |
| --> | |
| <!-- | |
| SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | |
| SPDX-Identifier: AGPL-3.0-or-later | |
| --> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/DelegationModal.vue` around lines 1 - 4, The SPDX header in
the comment block uses extra bullet prefixes; update the header comment so it
exactly matches the repository's required SPDX lines by removing the "- "
prefixes and leaving two comment lines: "SPDX-FileCopyrightText: 2026 Nextcloud
GmbH and Nextcloud contributors" and "SPDX-Identifier: AGPL-3.0-or-later" (i.e.,
modify the comment at the top of DelegationModal.vue to contain those exact SPDX
keys/values).
| <h2 id="delegation-modal-title"> | ||
| {{ t('mail', 'Delegation') }} | ||
| </h2> |
There was a problem hiding this comment.
Align the modal title with the rest of the delegation flow.
This still uses Delegation, while the surrounding actions read as Add delegate / Delegate access. Using one phrase throughout will make the flow feel less disjointed.
Proposed fix
- {{ t('mail', 'Delegation') }}
+ {{ t('mail', 'Delegate account') }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <h2 id="delegation-modal-title"> | |
| {{ t('mail', 'Delegation') }} | |
| </h2> | |
| <h2 id="delegation-modal-title"> | |
| {{ t('mail', 'Delegate account') }} | |
| </h2> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/DelegationModal.vue` around lines 13 - 15, The modal title
currently renders t('mail', 'Delegation') in the h2 with id
"delegation-modal-title"; change it to use the same phrasing as the rest of the
flow (e.g., t('mail', 'Add delegate') or t('mail', 'Delegate access') to match
surrounding buttons/labels), update or add the corresponding translation key in
the i18n resource, and ensure any tests or snapshots referencing "Delegation"
are updated to the new string; locate the title in DelegationModal.vue (h2
id="delegation-modal-title") to make the change.
| this.delegating = true | ||
| try { | ||
| const delegation = await delegate(this.account.accountId, this.selectedUser.id) | ||
| this.delegates.push(delegation) | ||
| showSuccess(t('mail', 'Delegated access to {userId}', { userId: this.selectedUser.id })) | ||
| } catch (error) { | ||
| logger.error('Could not delegate access', { error }) | ||
| showError(t('mail', 'Could not delegate access')) | ||
| } finally { | ||
| this.delegating = false | ||
| this.closeDialog() | ||
| } |
There was a problem hiding this comment.
Keep the add dialog open on errors, and clear its transient state on close.
finally always routes back to main, so a failed delegation kicks the user out of the form immediately after the error toast. On top of that, closeDialog() only flips view, so reopening the dialog can keep the previous selection/suggestions around and allow a stale resubmit.
Proposed fix
async addDelegate() {
if (!this.selectedUser) {
return
}
this.delegating = true
try {
const delegation = await delegate(this.account.accountId, this.selectedUser.id)
this.delegates.push(delegation)
showSuccess(t('mail', 'Delegated access to {userId}', { userId: this.selectedUser.id }))
+ this.closeDialog()
} catch (error) {
logger.error('Could not delegate access', { error })
showError(t('mail', 'Could not delegate access'))
} finally {
this.delegating = false
- this.closeDialog()
}
},
@@
closeDialog() {
+ this.selectedUser = null
+ this.userSuggestions = []
+ this.revokeUser = null
this.view = 'main'
},Also applies to: 289-291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/DelegationModal.vue` around lines 260 - 271, The current
finally block always calls closeDialog(), which closes the modal even on
delegation errors and leaves transient state (e.g., selectedUser, user
suggestions) intact for a stale resubmit; change the flow so that closeDialog()
is only invoked on success (after showSuccess) and not in the finally that runs
on errors, and add explicit state cleanup inside the closeDialog() or a new
reset method to clear transient fields (reset selectedUser, suggestion list, and
any temporary flags) so reopening the DelegationModal shows a fresh form; apply
the same change to the other delegation block referenced (around the delegation
call at lines ~289-291) and ensure delegating is still set to false in finally.
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
Mvp for #12403
Summary by CodeRabbit
New Features
Improvements
Tests