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 7 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);
}
}
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
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
33 changes: 9 additions & 24 deletions apps/files/lib/Notification/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
use OCP\Notification\INotifier;

class Notifier implements INotifier, IDismissableNotifier {

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

Expand Down Expand Up @@ -151,7 +150,6 @@ public function handleTransferownershipRequest(INotification $notification, stri
'name' => $sourceUser->getDisplayName(),
],
])
->setParsedSubject(str_replace('{user}', $sourceUser->getDisplayName(), $l->t('Incoming ownership transfer from {user}')))
->setRichMessage(
$l->t("Do you want to accept {path}?\n\nNote: The transfer process after accepting may take up to 1 hour."),
[
Expand All @@ -160,20 +158,17 @@ public function handleTransferownershipRequest(INotification $notification, stri
'id' => $param['targetUser'] . '::' . $param['nodeName'],
'name' => $param['nodeName'],
]
])
->setParsedMessage(str_replace('{path}', $param['nodeName'], $l->t("Do you want to accept {path}?\n\nNote: The transfer process after accepting may take up to 1 hour.")));
]);

return $notification;
}

public function handleTransferOwnershipFailedSource(INotification $notification, string $languageCode): INotification {
public function handleTransferOwnershipFailedSource(INotification $notification, string $languageCode): INotification {
$l = $this->l10nFactory->get('files', $languageCode);
$param = $notification->getSubjectParameters();

$targetUser = $this->getUser($param['targetUser']);
$notification->setRichSubject($l->t('Ownership transfer failed'))
->setParsedSubject($l->t('Ownership transfer failed'))

->setRichMessage(
$l->t('Your ownership transfer of {path} to {user} failed.'),
[
Expand All @@ -187,19 +182,16 @@ public function handleTransferOwnershipFailedSource(INotification $notification,
'id' => $targetUser->getUID(),
'name' => $targetUser->getDisplayName(),
],
])
->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $targetUser->getDisplayName()], $l->t('Your ownership transfer of {path} to {user} failed.')));
]);
return $notification;
}

public function handleTransferOwnershipFailedTarget(INotification $notification, string $languageCode): INotification {
public function handleTransferOwnershipFailedTarget(INotification $notification, string $languageCode): INotification {
$l = $this->l10nFactory->get('files', $languageCode);
$param = $notification->getSubjectParameters();

$sourceUser = $this->getUser($param['sourceUser']);
$notification->setRichSubject($l->t('Ownership transfer failed'))
->setParsedSubject($l->t('Ownership transfer failed'))

->setRichMessage(
$l->t('The ownership transfer of {path} from {user} failed.'),
[
Expand All @@ -213,20 +205,17 @@ public function handleTransferOwnershipFailedTarget(INotification $notification,
'id' => $sourceUser->getUID(),
'name' => $sourceUser->getDisplayName(),
],
])
->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $sourceUser->getDisplayName()], $l->t('The ownership transfer of {path} from {user} failed.')));
]);

return $notification;
}

public function handleTransferOwnershipDoneSource(INotification $notification, string $languageCode): INotification {
public function handleTransferOwnershipDoneSource(INotification $notification, string $languageCode): INotification {
$l = $this->l10nFactory->get('files', $languageCode);
$param = $notification->getSubjectParameters();

$targetUser = $this->getUser($param['targetUser']);
$notification->setRichSubject($l->t('Ownership transfer done'))
->setParsedSubject($l->t('Ownership transfer done'))

->setRichMessage(
$l->t('Your ownership transfer of {path} to {user} has completed.'),
[
Expand All @@ -240,20 +229,17 @@ public function handleTransferOwnershipDoneSource(INotification $notification,
'id' => $targetUser->getUID(),
'name' => $targetUser->getDisplayName(),
],
])
->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $targetUser->getDisplayName()], $l->t('Your ownership transfer of {path} to {user} has completed.')));
]);

return $notification;
}

public function handleTransferOwnershipDoneTarget(INotification $notification, string $languageCode): INotification {
public function handleTransferOwnershipDoneTarget(INotification $notification, string $languageCode): INotification {
$l = $this->l10nFactory->get('files', $languageCode);
$param = $notification->getSubjectParameters();

$sourceUser = $this->getUser($param['sourceUser']);
$notification->setRichSubject($l->t('Ownership transfer done'))
->setParsedSubject($l->t('Ownership transfer done'))

->setRichMessage(
$l->t('The ownership transfer of {path} from {user} has completed.'),
[
Expand All @@ -267,8 +253,7 @@ public function handleTransferOwnershipDoneTarget(INotification $notification,
'id' => $sourceUser->getUID(),
'name' => $sourceUser->getDisplayName(),
],
])
->setParsedMessage(str_replace(['{path}', '{user}'], [$param['nodeName'], $sourceUser->getDisplayName()], $l->t('The ownership transfer of {path} from {user} has completed.')));
]);

return $notification;
}
Expand Down
20 changes: 2 additions & 18 deletions apps/files_sharing/lib/Activity/Providers/Base.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@
use OCP\Federation\ICloudIdManager;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\IUser;
use OCP\IUserManager;
use OCP\L10N\IFactory;

abstract class Base implements IProvider {

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

Expand Down Expand Up @@ -124,24 +122,10 @@ abstract protected function parseShortVersion(IEvent $event);
abstract protected function parseLongVersion(IEvent $event, IEvent $previousEvent = null);

/**
* @param IEvent $event
* @param string $subject
* @param array $parameters
* @throws \InvalidArgumentException
*/
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
10 changes: 1 addition & 9 deletions apps/files_sharing/lib/Notification/Notifier.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,6 @@ protected function parseShareExpiration(IShare $share, INotification $notificati

$notification
->setParsedSubject($l->t('Share will expire tomorrow'))
->setParsedMessage($l->t('One or more of your shares will expire tomorrow'))
->setRichMessage(
$l->t('Your share of {node} will expire tomorrow'),
[
Expand Down Expand Up @@ -232,14 +231,7 @@ protected function parseShareInvitation(IShare $share, INotification $notificati
throw new \InvalidArgumentException('Invalid subject');
}

$placeholders = $replacements = [];
foreach ($subjectParameters as $placeholder => $parameter) {
$placeholders[] = '{' . $placeholder . '}';
$replacements[] = $parameter['name'];
}

$notification->setParsedSubject(str_replace($placeholders, $replacements, $subject))
->setRichSubject($subject, $subjectParameters)
$notification->setRichSubject($subject, $subjectParameters)
->setIcon($this->url->getAbsoluteURL($this->url->imagePath('core', 'actions/share.svg')));

$acceptAction = $notification->createAction();
Expand Down
Loading