-
Notifications
You must be signed in to change notification settings - Fork 316
feat: add notifications for account delegation #12935
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
|
|
||
| namespace OCA\Mail\Service; | ||
|
|
||
| use OCA\Mail\Account; | ||
| use OCA\Mail\Db\AliasMapper; | ||
| use OCA\Mail\Db\Delegation; | ||
| use OCA\Mail\Db\DelegationMapper; | ||
|
|
@@ -18,6 +19,9 @@ | |
| use OCA\Mail\Exception\ClientException; | ||
| use OCA\Mail\Exception\DelegationExistsException; | ||
| use OCP\AppFramework\Db\DoesNotExistException; | ||
| use OCP\AppFramework\Utility\ITimeFactory; | ||
| use OCP\IUserManager; | ||
| use OCP\Notification\IManager; | ||
|
|
||
| class DelegationService { | ||
|
|
||
|
|
@@ -28,10 +32,14 @@ public function __construct( | |
| private MessageMapper $messageMapper, | ||
| private AliasMapper $aliasMapper, | ||
| private LocalMessageMapper $localMessageMapper, | ||
| private IUserManager $userManager, | ||
| private IManager $notificationManager, | ||
| private ITimeFactory $time, | ||
| ) { | ||
| } | ||
|
|
||
| public function delegate(int $accountId, string $userId): Delegation { | ||
| public function delegate(Account $account, string $userId, string $currentUserId): Delegation { | ||
| $accountId = $account->getId(); | ||
| try { | ||
| $this->delegationMapper->find($accountId, $userId); | ||
| throw new DelegationExistsException("Delegation already exists for account $accountId and user $userId"); | ||
|
|
@@ -42,17 +50,21 @@ public function delegate(int $accountId, string $userId): Delegation { | |
| $delegation = new Delegation(); | ||
| $delegation->setAccountId($accountId); | ||
| $delegation->setUserId($userId); | ||
| return $this->delegationMapper->insert($delegation); | ||
| $result = $this->delegationMapper->insert($delegation); | ||
| $this->notify($userId, $currentUserId, $account, true); | ||
| return $result; | ||
|
Comment on lines
+53
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isolate notification failures from delegation state changes.
Proposed hardening-private function notify(string $userId, string $currentUserId, Account $account, bool $delegated) {
- $notification = $this->notificationManager->createNotification();
- $displayName = $this->userManager->get($currentUserId)?->getDisplayName() ?? $currentUserId;
- $time = $this->time->getDateTime('now');
- $notification
- ->setApp('mail')
- ->setUser($userId)
- ->setObject('delegation', (string)$account->getId())
- ->setSubject('account_delegation', [
- 'id' => $account->getId(),
- 'account_email' => $account->getEmail(),
-
- ])
- ->setDateTime($time)
- ->setMessage('account_delegation_changed', [
- 'id' => $account->getId(),
- 'delegated' => $delegated,
- 'current_user_id' => $currentUserId,
- 'current_user_display_name' => $displayName,
- 'account_email' => $account->getEmail(),
- ]
- );
- $this->notificationManager->notify($notification);
+private function notify(string $userId, string $currentUserId, Account $account, bool $delegated): void {
+ try {
+ $notification = $this->notificationManager->createNotification();
+ $displayName = $this->userManager->get($currentUserId)?->getDisplayName() ?? $currentUserId;
+ $time = $this->time->getDateTime('now');
+ $notification
+ ->setApp('mail')
+ ->setUser($userId)
+ ->setObject('delegation', (string)$account->getId())
+ ->setSubject('account_delegation', [
+ 'id' => $account->getId(),
+ 'account_email' => $account->getEmail(),
+ ])
+ ->setDateTime($time)
+ ->setMessage('account_delegation_changed', [
+ 'id' => $account->getId(),
+ 'delegated' => $delegated,
+ 'current_user_id' => $currentUserId,
+ 'current_user_display_name' => $displayName,
+ 'account_email' => $account->getEmail(),
+ ]);
+ $this->notificationManager->notify($notification);
+ } catch (\Throwable) {
+ // best-effort notification; do not fail successful delegation state change
+ }
}Also applies to: 67-68, 136-159
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also valid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR description image shows two delegation notifications in a dark-themed panel — one confirming delegation and one confirming revocation of
private function notify(string $userId, string $currentUserId, Account $account, bool $delegated): void {
try {
$notification = $this->notificationManager->createNotification();
$displayName = $this->userManager->get($currentUserId)?->getDisplayName() ?? $currentUserId;
$time = $this->time->getDateTime('now');
$notification
->setApp('mail')
->setUser($userId)
->setObject('delegation', (string)$account->getId())
->setSubject('account_delegation', [
'id' => $account->getId(),
'account_email' => $account->getEmail(),
])
->setDateTime($time)
->setMessage('account_delegation_changed', [
'id' => $account->getId(),
'delegated' => $delegated,
'current_user_id' => $currentUserId,
'current_user_display_name' => $displayName,
'account_email' => $account->getEmail(),
]);
$this->notificationManager->notify($notification);
} catch (\Throwable) {
// best-effort notification; do not fail a successful delegation state change
}
}Key points:
|
||
| } | ||
|
|
||
| public function findDelegatedToUsersForAccount(int $accountId): array { | ||
| return $this->delegationMapper->findDelegatedToUsers($accountId); | ||
| } | ||
|
|
||
| public function unDelegate(int $accountId, string $userId): void { | ||
| public function unDelegate(Account $account, string $userId, string $currentUserId): void { | ||
| try { | ||
| $accountId = $account->getId(); | ||
| $delegation = $this->delegationMapper->find($accountId, $userId); | ||
| $this->delegationMapper->delete($delegation); | ||
| $this->notify($userId, $currentUserId, $account, false); | ||
| } catch (DoesNotExistException $e) { | ||
| // shouldn't end up here | ||
| // delegation not found nothing to undelegate | ||
|
|
@@ -112,4 +124,37 @@ public function resolveLocalMessageUserId(int $localMessageId, string $currentUs | |
| $accountId = $this->localMessageMapper->findAccountIdForLocalMessage($localMessageId); | ||
| return $this->resolveAccountUserId($accountId, $currentUserId); | ||
| } | ||
|
|
||
| /** | ||
| * Send a notification on delegation | ||
| * @param string $userId The user the account is being delegated to | ||
| * @param string $currentUserId Current user | ||
| * @param Account $account The delegated account | ||
| * @param bool $delegated true for delegate|false for undelegate | ||
| * @return void | ||
| */ | ||
| private function notify(string $userId, string $currentUserId, Account $account, bool $delegated) { | ||
| $notification = $this->notificationManager->createNotification(); | ||
| $displayName = $this->userManager->get($currentUserId)?->getDisplayName() ?? $currentUserId; | ||
| $time = $this->time->getDateTime('now'); | ||
| $notification | ||
| ->setApp('mail') | ||
| ->setUser($userId) | ||
| ->setObject('delegation', (string)$account->getId()) | ||
| ->setSubject('account_delegation', [ | ||
| 'id' => $account->getId(), | ||
| 'account_email' => $account->getEmail(), | ||
|
|
||
| ]) | ||
| ->setDateTime($time) | ||
| ->setMessage('account_delegation_changed', [ | ||
| 'id' => $account->getId(), | ||
| 'delegated' => $delegated, | ||
| 'current_user_id' => $currentUserId, | ||
| 'current_user_display_name' => $displayName, | ||
| 'account_email' => $account->getEmail(), | ||
| ] | ||
| ); | ||
| $this->notificationManager->notify($notification); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\Mail\Tests\Unit\Notification; | ||
|
|
||
| use ChristophWurst\Nextcloud\Testing\TestCase; | ||
| use OCA\Mail\Notification\Notifier; | ||
| use OCP\IL10N; | ||
| use OCP\IURLGenerator; | ||
| use OCP\L10N\IFactory; | ||
| use OCP\Notification\INotification; | ||
| use OCP\Notification\UnknownNotificationException; | ||
| use PHPUnit\Framework\MockObject\MockObject; | ||
|
|
||
| class NotifierTest extends TestCase { | ||
| private IFactory&MockObject $factory; | ||
| private IURLGenerator&MockObject $url; | ||
| private IL10N&MockObject $l10n; | ||
| private Notifier $notifier; | ||
|
|
||
| protected function setUp(): void { | ||
| parent::setUp(); | ||
|
|
||
| $this->factory = $this->createMock(IFactory::class); | ||
| $this->url = $this->createMock(IURLGenerator::class); | ||
| $this->l10n = $this->createMock(IL10N::class); | ||
| $this->l10n->method('t')->willReturnArgument(0); | ||
| $this->factory->method('get')->willReturn($this->l10n); | ||
|
|
||
| $this->notifier = new Notifier($this->factory, $this->url); | ||
| } | ||
|
|
||
| public function testGetID(): void { | ||
| $this->assertEquals('mail', $this->notifier->getID()); | ||
| } | ||
|
|
||
| public function testGetName(): void { | ||
| $this->assertEquals('Mail', $this->notifier->getName()); | ||
| } | ||
|
|
||
| public function testPrepareForeignAppThrows(): void { | ||
| $notification = $this->createMock(INotification::class); | ||
| $notification->method('getApp')->willReturn('other'); | ||
|
|
||
| $this->expectException(UnknownNotificationException::class); | ||
|
|
||
| $this->notifier->prepare($notification, 'en'); | ||
| } | ||
|
|
||
| public function testPrepareUnknownSubjectThrows(): void { | ||
| $notification = $this->createMock(INotification::class); | ||
| $notification->method('getApp')->willReturn('mail'); | ||
| $notification->method('getSubject')->willReturn('something_unknown'); | ||
|
|
||
| $this->expectException(UnknownNotificationException::class); | ||
|
|
||
| $this->notifier->prepare($notification, 'en'); | ||
| } | ||
|
|
||
| public function testPrepareAccountDelegationDelegated(): void { | ||
| $notification = $this->createMock(INotification::class); | ||
| $notification->method('getApp')->willReturn('mail'); | ||
| $notification->method('getSubject')->willReturn('account_delegation'); | ||
| $notification->method('getSubjectParameters')->willReturn([ | ||
| 'id' => 1, | ||
| 'account_email' => 'owner@example.com', | ||
| ]); | ||
| $notification->method('getMessageParameters')->willReturn([ | ||
| 'id' => 1, | ||
| 'delegated' => true, | ||
| 'current_user_id' => 'owner', | ||
| 'current_user_display_name' => 'Owner User', | ||
| 'account_email' => 'owner@example.com', | ||
| ]); | ||
|
|
||
| $this->url->method('linkTo')->with('mail', 'img/delegation.svg')->willReturn('/apps/mail/img/delegation.svg'); | ||
| $this->url->method('getAbsoluteURL')->with('/apps/mail/img/delegation.svg')->willReturn('https://example.com/apps/mail/img/delegation.svg'); | ||
|
|
||
| $notification->expects($this->once()) | ||
| ->method('setIcon') | ||
| ->with('https://example.com/apps/mail/img/delegation.svg') | ||
| ->willReturnSelf(); | ||
| $notification->expects($this->once()) | ||
| ->method('setRichSubject') | ||
| ->with( | ||
| '{account_email} has been delegated to you', | ||
| [ | ||
| 'account_email' => [ | ||
| 'type' => 'highlight', | ||
| 'id' => '1', | ||
| 'name' => 'owner@example.com', | ||
| ], | ||
| ] | ||
| ) | ||
| ->willReturnSelf(); | ||
| $notification->expects($this->once()) | ||
| ->method('setRichMessage') | ||
| ->with( | ||
| '{user} delegated {account} to you', | ||
| [ | ||
| 'user' => [ | ||
| 'type' => 'user', | ||
| 'id' => 'owner', | ||
| 'name' => 'Owner User', | ||
| ], | ||
| 'account' => [ | ||
| 'type' => 'highlight', | ||
| 'id' => '1', | ||
| 'name' => 'owner@example.com', | ||
| ], | ||
| ] | ||
| ) | ||
| ->willReturnSelf(); | ||
|
|
||
| $result = $this->notifier->prepare($notification, 'en'); | ||
|
|
||
| $this->assertSame($notification, $result); | ||
| } | ||
|
|
||
| public function testPrepareAccountDelegationRevoked(): void { | ||
| $notification = $this->createMock(INotification::class); | ||
| $notification->method('getApp')->willReturn('mail'); | ||
| $notification->method('getSubject')->willReturn('account_delegation'); | ||
| $notification->method('getSubjectParameters')->willReturn([ | ||
| 'id' => 1, | ||
| 'account_email' => 'owner@example.com', | ||
| ]); | ||
| $notification->method('getMessageParameters')->willReturn([ | ||
| 'id' => 1, | ||
| 'delegated' => false, | ||
| 'current_user_id' => 'owner', | ||
| 'current_user_display_name' => 'Owner User', | ||
| 'account_email' => 'owner@example.com', | ||
| ]); | ||
|
|
||
| $this->url->method('linkTo')->with('mail', 'img/delegation.svg')->willReturn('/apps/mail/img/delegation.svg'); | ||
| $this->url->method('getAbsoluteURL')->with('/apps/mail/img/delegation.svg')->willReturn('https://example.com/apps/mail/img/delegation.svg'); | ||
|
|
||
| $notification->expects($this->once()) | ||
| ->method('setIcon') | ||
| ->with('https://example.com/apps/mail/img/delegation.svg') | ||
| ->willReturnSelf(); | ||
| $notification->expects($this->once()) | ||
| ->method('setRichSubject') | ||
| ->with( | ||
| '{account_email} is no longer delegated to you', | ||
| [ | ||
| 'account_email' => [ | ||
| 'type' => 'highlight', | ||
| 'id' => '1', | ||
| 'name' => 'owner@example.com', | ||
| ], | ||
| ] | ||
| ) | ||
| ->willReturnSelf(); | ||
| $notification->expects($this->once()) | ||
| ->method('setRichMessage') | ||
| ->with( | ||
| '{user} revoked delagation for {account}', | ||
| [ | ||
| 'user' => [ | ||
| 'type' => 'user', | ||
| 'id' => 'owner', | ||
| 'name' => 'Owner User', | ||
| ], | ||
| 'account' => [ | ||
| 'type' => 'highlight', | ||
| 'id' => '1', | ||
| 'name' => 'owner@example.com', | ||
| ], | ||
| ] | ||
| ) | ||
| ->willReturnSelf(); | ||
|
|
||
| $result = $this->notifier->prepare($notification, 'en'); | ||
|
|
||
| $this->assertSame($notification, $result); | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the user-facing typo in revoked delegation message.
Line 111 uses
delagation, which will surface incorrect text in notifications.Proposed fix
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hamza221— The PR description image also confirms the typo is visible in the live notification UI ("revoked delagation for hamza@nextdev.app"). Thanks for flagging it for a fix — just correctdelagation→delegationon Line 111 when you get to it.