Skip to content
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

Compute notification parsed subject from rich subject when possible #34807

Merged
merged 8 commits into from
Dec 19, 2022

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Oct 25, 2022

Let’s avoid duplicated code and translation strings and ease application developpers lives.

@come-nc come-nc added the 3. to review Waiting for reviews label Oct 25, 2022
@come-nc come-nc self-assigned this Oct 25, 2022
@ChristophWurst
Copy link
Member

Could you please elaborate on the problem and the solution? :)

@@ -97,14 +95,12 @@

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
->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
@come-nc
Copy link
Contributor Author

come-nc commented Oct 25, 2022

Could you please elaborate on the problem and the solution? :)

Both activity event and notification expects applications to give both a rich subject and a parsed subject, and same thing for message.
But the parsed version can be computed from the rich one in most instances, and it’s already done in several part of our code, with duplicated code.

Also, when doing this:

$event->setParsedSubject($this->l->t('%1$s commented', [$author['name']]))
->setRichSubject($this->l->t('{author} commented'), ['author' => $author]);

Two strings are created for translators to translate, while they contain the same translatable text.

This PR attempts to remove the duplicated code and strings by providing a suitable fallback in notification and activity event directly.

@come-nc come-nc added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 25, 2022
@come-nc
Copy link
Contributor Author

come-nc commented Oct 25, 2022

(putting this back to developing, I’ll look into Joas feedback and special cases next week)

@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 21, 2022
@come-nc
Copy link
Contributor Author

come-nc commented Nov 29, 2022

/rebase

@come-nc come-nc added this to the Nextcloud 26 milestone Nov 29, 2022
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…ect is able to do it

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@nextcloud-command nextcloud-command force-pushed the fix/compute-notification-parsed-subject branch from 5ce21e5 to 8a04bf5 Compare November 29, 2022 14:36
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Dec 13, 2022

@CarlSchwan @ChristophWurst @miaulalala I need a codeowner approval here 🙏

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🤞

@come-nc
Copy link
Contributor Author

come-nc commented Dec 19, 2022

There were 4 failures:
263	
264	1) OCA\Comments\Tests\Unit\Notification\NotifierTest::testPrepareSuccess
265	Expectation failed for method name is "setParsedSubject" when invoked 1 time(s).
266	Method was expected to be called 1 times, actually called 0 times.
267	
268	2) OCA\Comments\Tests\Unit\Notification\NotifierTest::testPrepareSuccessDeletedUser
269	Expectation failed for method name is "setParsedSubject" when invoked 1 time(s).
270	Method was expected to be called 1 times, actually called 0 times.
271	
272	3) OCA\DAV\Tests\unit\CalDAV\Activity\Provider\BaseTest::testSetSubjects with data set #0 ('abc', array(), 'abc')
273	Expectation failed for method name is "setParsedSubject" when invoked 1 time(s).
274	Method was expected to be called 1 times, actually called 0 times.
275	
276	4) OCA\DAV\Tests\unit\CalDAV\Activity\Provider\BaseTest::testSetSubjects with data set #1 ('{actor} created {calendar}', array(array('abc'), array('xyz')), 'abc created xyz')
277	Expectation failed for method name is "setParsedSubject" when invoked 1 time(s).
278	Method was expected to be called 1 times, actually called 0 times.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc merged commit cbbb071 into master Dec 19, 2022
@come-nc come-nc deleted the fix/compute-notification-parsed-subject branch December 19, 2022 09:46
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish pending documentation This pull request needs an associated documentation update and removed 3. to review Waiting for reviews labels Dec 19, 2022
@DaphneMuller
Copy link

hello @come-nc ,
Thank you for your work on this pull request! This ticket seems to have the tag 'missing documentation', is there any chance you could clarify what documentation is missing? Is this for admins or for app developers?

@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Feb 21, 2023
@come-nc come-nc mentioned this pull request Aug 1, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants