feat: add notifications for account delegation#12935
Conversation
hamza221
force-pushed
the
feat/delegation/add-notifications
branch
from
cbb141d to
e58a3f6
Compare
hamza221
force-pushed
the
feat/delegation/add-notifications
branch
from
e58a3f6 to
3cffe22
Compare
Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
hamza221
force-pushed
the
feat/delegation/add-notifications
branch
from
3cffe22 to
79e2590
Compare
AI-assisted: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Hamza <hamzamahjoubi221@gmail.com>
hamza221
force-pushed
the
feat/delegation/add-notifications
branch
from
4c7c5b7 to
db6ddb3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughDelegationService now accepts Account objects and current user IDs, triggers notifications on delegation changes, and the controller, notification system, and tests are updated to support the refactored signatures and new notification behavior. ChangesAccount Delegation with Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/Unit/Service/DelegationServiceTest.php (1)
177-189: ⚡ Quick winAssert that no notification is sent when undelegation target does not exist.
This branch currently verifies deletion is skipped, but not that
IManager::notifyis skipped too.Proposed test strengthening
public function testUnDelegateWhenNotFound(): void { $this->mockNotification(); + $this->notificationManager->expects($this->never()) + ->method('notify'); + $this->delegationMapper->expects($this->once()) ->method('find') ->with(1, 'delegatee') ->willThrowException(new DoesNotExistException('Not found'));
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d7d080e5-d6ca-4d23-92b8-6cc947971454
📒 Files selected for processing (6)
lib/Controller/DelegationController.phplib/Notification/Notifier.phplib/Service/DelegationService.phptests/Unit/Controller/DelegationControllerTest.phptests/Unit/Notification/NotifierTest.phptests/Unit/Service/DelegationServiceTest.php
| 'name' => $parameters['account_email'] | ||
| ] | ||
| ]); | ||
| $notification->setRichMessage($l->t('{user} revoked delagation for {account}'), |
There was a problem hiding this comment.
Fix the user-facing typo in revoked delegation message.
Line 111 uses delagation, which will surface incorrect text in notifications.
Proposed fix
- $notification->setRichMessage($l->t('{user} revoked delagation for {account}'),
+ $notification->setRichMessage($l->t('{user} revoked delegation for {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.
| $notification->setRichMessage($l->t('{user} revoked delagation for {account}'), | |
| $notification->setRichMessage($l->t('{user} revoked delegation for {account}'), |
There was a problem hiding this comment.
@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 correct delagation → delegation on Line 111 when you get to it.
| $result = $this->delegationMapper->insert($delegation); | ||
| $this->notify($userId, $currentUserId, $account, true); | ||
| return $result; |
There was a problem hiding this comment.
Isolate notification failures from delegation state changes.
notify() is called after insert/delete and can throw, which can turn a successful delegation mutation into an error response while the DB change is already committed.
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
There was a problem hiding this comment.
The PR description image shows two delegation notifications in a dark-themed panel — one confirming delegation and one confirming revocation of hamza@nextdev.app.
@hamza221, glad we agree! Here's the hardened notify() method ready to apply:
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:
- Wrapping in
try/catch (\Throwable)ensures any exception fromcreateNotification(),get(),getDateTime(), ornotify()is swallowed after the DB write has already committed. - The explicit
: voidreturn type is also added for consistency. - You may optionally log the caught exception at a debug/warning level (e.g. via a
LoggerInterface) so failures are still observable without surfacing to the user.
1 participant
Part of #12403
Unit tests Ai-assisted
Summary by CodeRabbit
New Features
Tests