Skip to content

Compute notification parsed subject from rich subject when possible #34807

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
come-nc merged 8 commits into from
Dec 19, 2022
Merged

Compute notification parsed subject from rich subject when possible #34807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
323963e
Compute parsed from rich subject and message in Notification
come-nc Oct 25, 2022
db995d4
Same thing for Activity
come-nc Oct 25, 2022
d9151ff
Get rid of all useless calls to setParsedSubject now that setRichSubj…
come-nc Oct 25, 2022
2a3090d
Throw instead of using a bogus value if name field is missing
come-nc Nov 21, 2022
cf8d97c
Revert changes to systemtags application
come-nc Nov 21, 2022
8a04bf5
Apply the same changes to richToParsed in Notification
come-nc Nov 21, 2022
f676076
Throw on missing type of rich subject parameter
come-nc Dec 8, 2022
e553cd1
Fix comments and dav tests with Notifications and Activity changes
come-nc Dec 19, 2022
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
12 changes: 4 additions & 8 deletions apps/comments/lib/Activity/Provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@
use OCP\Comments\NotFoundException;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\L10N\IFactory;

class Provider implements IProvider {

protected IFactory $languageFactory;
protected ?IL10N $l = null;
protected IUrlGenerator $url;
Expand Down Expand Up @@ -97,14 +95,12 @@ protected function parseShortVersion(IEvent $event): IEvent {

if ($event->getSubject() === 'add_comment_subject') {
if ($subjectParameters['actor'] === $this->activityManager->getCurrentUserId()) {
$event->setParsedSubject($this->l->t('You commented'))
->setRichSubject($this->l->t('You commented'), []);
$event->setRichSubject($this->l->t('You commented'), []);

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method t on possibly null value
} else {
$author = $this->generateUserParameter($subjectParameters['actor']);
$event->setParsedSubject($this->l->t('%1$s commented', [$author['name']]))
->setRichSubject($this->l->t('{author} commented'), [
'author' => $author,
]);
$event->setRichSubject($this->l->t('{author} commented'), [

Check notice

Code scanning / Psalm

PossiblyNullReference

Cannot call method t on possibly null value
'author' => $author,
]);
}
} else {
throw new \InvalidArgumentException();
Expand Down
19 changes: 0 additions & 19 deletions apps/comments/lib/Notification/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,13 @@
use OCP\Comments\NotFoundException;
use OCP\Files\IRootFolder;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\L10N\IFactory;
use OCP\Notification\AlreadyProcessedException;
use OCP\Notification\INotification;
use OCP\Notification\INotifier;

class Notifier implements INotifier {

protected IFactory $l10nFactory;
protected IRootFolder $rootFolder;
protected ICommentsManager $commentsManager;
Expand Down Expand Up @@ -147,9 +145,7 @@ public function prepare(INotification $notification, string $languageCode): INot
}
[$message, $messageParameters] = $this->commentToRichMessage($comment);
$notification->setRichSubject($subject, $subjectParameters)
->setParsedSubject($this->richToParsed($subject, $subjectParameters))
->setRichMessage($message, $messageParameters)
->setParsedMessage($this->richToParsed($message, $messageParameters))
->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/comment.svg')))
->setLink($this->url->linkToRouteAbsolute(
'comments.Notifications.view',
Expand Down Expand Up @@ -205,19 +201,4 @@ public function commentToRichMessage(IComment $comment): array {
}
return [$message, $messageParameters];
}

public function richToParsed(string $message, array $parameters): string {
$placeholders = $replacements = [];
foreach ($parameters as $placeholder => $parameter) {
$placeholders[] = '{' . $placeholder . '}';
if ($parameter['type'] === 'user') {
$replacements[] = '@' . $parameter['name'];
} elseif ($parameter['type'] === 'file') {
$replacements[] = $parameter['path'];
} else {
$replacements[] = $parameter['name'];
}
}
return str_replace($placeholders, $replacements, $message);
}
}
26 changes: 8 additions & 18 deletions apps/comments/tests/Unit/Notification/NotifierTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,13 @@
use OCP\Files\Node;
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;
use Test\TestCase;

class NotifierTest extends TestCase {

/** @var Notifier */
protected $notifier;
/** @var IFactory|MockObject */
Expand Down Expand Up @@ -135,10 +133,8 @@ public function testPrepareSuccess() {
->method('getSubjectParameters')
->willReturn(['files', '678']);
$this->notification
->expects($this->once())
->method('setParsedSubject')
->with($message)
->willReturnSelf();
->expects($this->never())
->method('setParsedSubject');
$this->notification
->expects($this->once())
->method('setRichSubject')
Expand All @@ -150,10 +146,8 @@ public function testPrepareSuccess() {
->with('Hi {mention-user1}!', ['mention-user1' => ['type' => 'user', 'id' => 'you', 'name' => 'Your name']])
->willReturnSelf();
$this->notification
->expects($this->once())
->method('setParsedMessage')
->with('Hi @Your name!')
->willReturnSelf();
->expects($this->never())
->method('setParsedMessage');
$this->notification
->expects($this->once())
->method('setIcon')
Expand Down Expand Up @@ -256,10 +250,8 @@ public function testPrepareSuccessDeletedUser() {
->method('getSubjectParameters')
->willReturn(['files', '678']);
$this->notification
->expects($this->once())
->method('setParsedSubject')
->with($message)
->willReturnSelf();
->expects($this->never())
->method('setParsedSubject');
$this->notification
->expects($this->once())
->method('setRichSubject')
Expand All @@ -271,10 +263,8 @@ public function testPrepareSuccessDeletedUser() {
->with('Hi {mention-user1}!', ['mention-user1' => ['type' => 'user', 'id' => 'you', 'name' => 'Your name']])
->willReturnSelf();
$this->notification
->expects($this->once())
->method('setParsedMessage')
->with('Hi @Your name!')
->willReturnSelf();
->expects($this->never())
->method('setParsedMessage');
$this->notification
->expects($this->once())
->method('setIcon')
Expand Down
18 changes: 2 additions & 16 deletions apps/dav/lib/CalDAV/Activity/Provider/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,9 @@
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;

abstract class Base implements IProvider {

/** @var IUserManager */
protected $userManager;

Expand All @@ -58,20 +56,8 @@ public function __construct(IUserManager $userManager, IGroupManager $groupManag
$this->url = $urlGenerator;
}

/**
* @param IEvent $event
* @param string $subject
* @param array $parameters
*/
protected function setSubjects(IEvent $event, $subject, array $parameters) {
$placeholders = $replacements = [];
foreach ($parameters as $placeholder => $parameter) {
$placeholders[] = '{' . $placeholder . '}';
$replacements[] = $parameter['name'];
}

$event->setParsedSubject(str_replace($placeholders, $replacements, $subject))
->setRichSubject($subject, $parameters);
protected function setSubjects(IEvent $event, string $subject, array $parameters): void {
$event->setRichSubject($subject, $parameters);
}

/**
Expand Down
16 changes: 1 addition & 15 deletions apps/dav/lib/CardDAV/Activity/Provider/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,9 @@
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;

abstract class Base implements IProvider {

/** @var IUserManager */
protected $userManager;

Expand All @@ -60,20 +58,8 @@ public function __construct(IUserManager $userManager,
$this->url = $urlGenerator;
}

/**
* @param IEvent $event
* @param string $subject
* @param array $parameters
*/
protected function setSubjects(IEvent $event, string $subject, array $parameters): void {
$placeholders = $replacements = [];
foreach ($parameters as $placeholder => $parameter) {
$placeholders[] = '{' . $placeholder . '}';
$replacements[] = $parameter['name'];
}

$event->setParsedSubject(str_replace($placeholders, $replacements, $subject))
->setRichSubject($subject, $parameters);
$event->setRichSubject($subject, $parameters);
}

/**
Expand Down
8 changes: 2 additions & 6 deletions apps/dav/tests/unit/CalDAV/Activity/Provider/BaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,11 @@
use OCP\IGroupManager;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use PHPUnit\Framework\MockObject\MockObject;
use Test\TestCase;

class BaseTest extends TestCase {

/** @var IUserManager|MockObject */
protected $userManager;

Expand Down Expand Up @@ -85,10 +83,8 @@ public function testSetSubjects(string $subject, array $parameters, string $pars
->method('setRichSubject')
->with($subject, $parameters)
->willReturnSelf();
$event->expects($this->once())
->method('setParsedSubject')
->with($parsedSubject)
->willReturnSelf();
$event->expects($this->never())
->method('setParsedSubject');

$this->invokePrivate($this->provider, 'setSubjects', [$event, $subject, $parameters]);
}
Expand Down
9 changes: 0 additions & 9 deletions apps/federatedfilesharing/lib/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,6 @@ public function prepare(INotification $notification, string $languageCode): INot
$params[3] = $remoteInitiator['name'] . '@' . $remoteInitiator['server'];
$params[4] = $remoteOwner['name'] . '@' . $remoteOwner['server'];

$notification->setParsedSubject(
$l->t('You received "%3$s" as a remote share from %4$s (%1$s) (on behalf of %5$s (%2$s))', $params)
);

$notification->setRichSubject(
$l->t('You received {share} as a remote share from {user} (on behalf of {behalf})'),
[
Expand All @@ -128,11 +124,6 @@ public function prepare(INotification $notification, string $languageCode): INot
$remoteOwner = $this->createRemoteUser($params[0]);
$params[3] = $remoteOwner['name'] . '@' . $remoteOwner['server'];

$notification->setParsedSubject(
$l->t('You received "%3$s" as a remote share from %4$s (%1$s)', $params)
);


$notification->setRichSubject(
$l->t('You received {share} as a remote share from {user}'),
[
Expand Down
3 changes: 1 addition & 2 deletions apps/files/lib/Activity/FavoriteProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ protected function setSubjects(IEvent $event, $subject) {
'link' => $this->url->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $subjectParams['id']]),
];

$event->setParsedSubject(str_replace('{file}', $parameter['path'], $subject))
->setRichSubject($subject, ['file' => $parameter]);
$event->setRichSubject($subject, ['file' => $parameter]);
}
}
17 changes: 2 additions & 15 deletions apps/files/lib/Activity/Provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,10 @@
use OCP\Files\NotFoundException;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\L10N\IFactory;

class Provider implements IProvider {

/** @var IFactory */
protected $languageFactory;

Expand Down Expand Up @@ -306,19 +304,8 @@ private function isHiddenFile(string $filename): bool {
return strlen($filename) > 0 && $filename[0] === '.';
}

protected function setSubjects(IEvent $event, $subject, array $parameters) {
$placeholders = $replacements = [];
foreach ($parameters as $placeholder => $parameter) {
$placeholders[] = '{' . $placeholder . '}';
if ($parameter['type'] === 'file') {
$replacements[] = $parameter['path'];
} else {
$replacements[] = $parameter['name'];
}
}

$event->setParsedSubject(str_replace($placeholders, $replacements, $subject))
->setRichSubject($subject, $parameters);
protected function setSubjects(IEvent $event, string $subject, array $parameters): void {
$event->setRichSubject($subject, $parameters);
}

/**
Expand Down
Loading