Skip to content

Rich object string parameters for notifications #2935

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.

Sign up for GitHub

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

Merged
juliusknorr merged 1 commit into from
Apr 16, 2021
Merged

Rich object string parameters for notifications #2935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 70 additions & 3 deletions lib/Notification/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

use OCA\Deck\Db\BoardMapper;
use OCA\Deck\Db\CardMapper;
use OCA\Deck\Db\StackMapper;
use OCP\IURLGenerator;
use OCP\IUserManager;
use OCP\L10N\IFactory;
Expand All @@ -41,6 +42,8 @@ class Notifier implements INotifier {
protected $userManager;
/** @var CardMapper */
protected $cardMapper;
/** @var StackMapper */
protected $stackMapper;
/** @var BoardMapper */
protected $boardMapper;

Expand All @@ -49,12 +52,14 @@ public function __construct(
IURLGenerator $url,
IUserManager $userManager,
CardMapper $cardMapper,
StackMapper $stackMapper,
BoardMapper $boardMapper
) {
$this->l10nFactory = $l10nFactory;
$this->url = $url;
$this->userManager = $userManager;
$this->cardMapper = $cardMapper;
$this->stackMapper = $stackMapper;
$this->boardMapper = $boardMapper;
}

Expand Down Expand Up @@ -100,6 +105,11 @@ public function prepare(INotification $notification, string $languageCode): INot
if (!$boardId) {
throw new AlreadyProcessedException();
}

$card = $this->cardMapper->find($cardId);
$stackId = $card->getStackId();
$stack = $this->stackMapper->find($stackId);
Comment on lines +109 to +111
Copy link
Member Author

@nickvergessen nickvergessen Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a shorter way for this?

Copy link
Member

@juliusknorr juliusknorr Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not as of right now as we always use an entity and don't have any way to map joined results to it currently. I'd say we keep it like this and if we encounter more places where this would be useful we can add a helper to do this in one query.


$initiator = $this->userManager->get($params[2]);
if ($initiator !== null) {
$dn = $initiator->getDisplayName();
Expand All @@ -110,8 +120,22 @@ public function prepare(INotification $notification, string $languageCode): INot
(string) $l->t('The card "%s" on "%s" has been assigned to you by %s.', [$params[0], $params[1], $dn])
);
$notification->setRichSubject(
(string) $l->t('{user} has assigned the card "%s" on "%s" to you.', [$params[0], $params[1]]),
$l->t('{user} has assigned the card {deck-card} on {deck-board} to you.'),
[
'deck-card' => [
'type' => 'deck-card',
'id' => $cardId,
'name' => $params[0],
'boardname' => $params[1],
'stackname' => $stack->getTitle(),
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId . '/card/' . $cardId . '',
],
'deck-board' => [
'type' => 'deck-board',
'id' => $boardId,
'name' => $params[1],
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId,
],
'user' => [
'type' => 'user',
'id' => $params[2],
Expand All @@ -127,9 +151,33 @@ public function prepare(INotification $notification, string $languageCode): INot
if (!$boardId) {
throw new AlreadyProcessedException();
}

$card = $this->cardMapper->find($cardId);
$stackId = $card->getStackId();
$stack = $this->stackMapper->find($stackId);

$notification->setParsedSubject(
(string) $l->t('The card "%s" on "%s" has reached its due date.', $params)
);
$notification->setRichSubject(
$l->t('The card {deck-card} on {deck-board} has reached its due date.'),
[
'deck-card' => [
'type' => 'deck-card',
'id' => $cardId,
'name' => $params[0],
'boardname' => $params[1],
'stackname' => $stack->getTitle(),
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId . '/card/' . $cardId . '',
],
'deck-board' => [
'type' => 'deck-board',
'id' => $boardId,
'name' => $params[1],
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId,
],
]
);
$notification->setLink($this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId . '/card/' . $cardId . '');
break;
case 'card-comment-mentioned':
Expand All @@ -138,6 +186,11 @@ public function prepare(INotification $notification, string $languageCode): INot
if (!$boardId) {
throw new AlreadyProcessedException();
}

$card = $this->cardMapper->find($cardId);
$stackId = $card->getStackId();
$stack = $this->stackMapper->find($stackId);

$initiator = $this->userManager->get($params[2]);
if ($initiator !== null) {
$dn = $initiator->getDisplayName();
Expand All @@ -148,8 +201,16 @@ public function prepare(INotification $notification, string $languageCode): INot
(string) $l->t('%s has mentioned you in a comment on "%s".', [$dn, $params[0]])
);
$notification->setRichSubject(
(string) $l->t('{user} has mentioned you in a comment on "%s".', [$params[0]]),
$l->t('{user} has mentioned you in a comment on {deck-card}.'),
[
'deck-card' => [
'type' => 'deck-card',
'id' => $cardId,
'name' => $params[0],
'boardname' => $params[1],
'stackname' => $stack->getTitle(),
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId . '/card/' . $cardId . '',
],
'user' => [
'type' => 'user',
'id' => $params[2],
Expand Down Expand Up @@ -177,8 +238,14 @@ public function prepare(INotification $notification, string $languageCode): INot
(string) $l->t('The board "%s" has been shared with you by %s.', [$params[0], $dn])
);
$notification->setRichSubject(
(string) $l->t('{user} has shared the board %s with you.', [$params[0]]),
$l->t('{user} has shared {deck-board} with you.'),
Copy link
Member Author

@nickvergessen nickvergessen Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we should keep "… the board …"

Copy link
Member

@juliusknorr juliusknorr Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong opinions here, but I'd say the icon is already enough to indicate that it is deck related. Maybe @jancborchardt can comment about the general wording.

[
'deck-board' => [
'type' => 'deck-board',
'id' => $boardId,
'name' => $params[0],
'link' => $this->url->linkToRouteAbsolute('deck.page.index') . '#/board/' . $boardId,
],
'user' => [
'type' => 'user',
'id' => $params[1],
Expand Down
44 changes: 35 additions & 9 deletions tests/unit/Notification/NotifierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,42 +25,48 @@

use OCA\Deck\Db\BoardMapper;
use OCA\Deck\Db\CardMapper;
use OCA\Deck\Db\StackMapper;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\L10N\IFactory;
use OCP\Notification\INotification;
use PHPUnit\Framework\MockObject\MockObject;

class NotifierTest extends \Test\TestCase {

/** @var IFactory */
/** @var IFactory|MockObject */
protected $l10nFactory;
/** @var IURLGenerator */
/** @var IURLGenerator|MockObject */
protected $url;
/** @var IUserManager */
/** @var IUserManager|MockObject */
protected $userManager;
/** @var CardMapper */
/** @var CardMapper|MockObject */
protected $cardMapper;
/** @var StackMapper|MockObject */
protected $stackMapper;
/** @var BoardMapper */
protected $boardMapper;
/** @var IL10N|MockObject */
protected $l10n;
/** @var Notifier */
protected $notifier;
/** @var IL10N */
protected $l10n;

public function setUp(): void {
parent::setUp();
$this->l10nFactory = $this->createMock(IFactory::class);
$this->url = $this->createMock(IURLGenerator::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->cardMapper = $this->createMock(CardMapper::class);
$this->stackMapper = $this->createMock(StackMapper::class);
$this->boardMapper = $this->createMock(BoardMapper::class);
$this->notifier = new Notifier(
$this->l10nFactory,
$this->url,
$this->userManager,
$this->cardMapper,
$this->stackMapper,
$this->boardMapper
);
$this->l10n = \OC::$server->getL10N('deck');
Expand Down Expand Up @@ -149,7 +155,7 @@ public function testPrepareCardCommentMentioned() {
->with($expectedMessage);
$notification->expects($this->once())
->method('setRichSubject')
->with('{user} has mentioned you in a comment on "Card title".');
->with('{user} has mentioned you in a comment on {deck-card}.');


$this->url->expects($this->once())
Expand Down Expand Up @@ -218,11 +224,25 @@ public function testPrepareCardAssigned($withUserFound = true) {
->with($expectedMessage);
$notification->expects($this->once())
->method('setRichSubject')
->with('{user} has assigned the card "Card title" on "Board title" to you.', [
->with('{user} has assigned the card {deck-card} on {deck-board} to you.', [
'user' => [
'type' => 'user',
'id' => 'otheruser',
'name' => $dn,
],
'deck-card' => [
'type' => 'deck-card',
'id' => '123',
'name' => 'Card title',
'boardname' => 'Board title',
'stackname' => null,
'link' => '#/board/123/card/123',
],
'deck-board' => [
'type' => 'deck-board',
'id' => 123,
'name' => 'Board title',
'link' => '#/board/123',
]
]);

Expand Down Expand Up @@ -288,11 +308,17 @@ public function testPrepareBoardShared($withUserFound = true) {
->with($expectedMessage);
$notification->expects($this->once())
->method('setRichSubject')
->with('{user} has shared the board Board title with you.', [
->with('{user} has shared {deck-board} with you.', [
'user' => [
'type' => 'user',
'id' => 'otheruser',
'name' => $dn,
],
'deck-board' => [
'type' => 'deck-board',
'id' => 123,
'name' => 'Board title',
'link' => '#/board/123',
]
]);

Expand Down