-
Notifications
You must be signed in to change notification settings - Fork 282
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
Add notification on card assignment to user, fixes #475 #545
Add notification on card assignment to user, fixes #475 #545
Conversation
lib/Notification/Notifier.php
Outdated
} | ||
$notification->setParsedSubject( | ||
(string) $l->t('The card "%s" on "%s" has been assigned to you by %s.', $params) | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a rich subject as well, similar to https://github.com/nextcloud/deck/pull/545/files#diff-52c5c003756b8dcc9c5318257107346dL95? This way the user will also be linked properly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Thank you very much for your pull request @steav, looks good besides the inline comment. Can you also remove the changes to the language files. We are managing translations via transifex and those strings will be pulled from there: https://www.transifex.com/nextcloud/nextcloud |
@steav Can you also add a sign-off message to the commits as described in https://github.com/nextcloud/deck#sign-your-work ? |
Thanks you a lot @juliushaertl for your advices and your awesome work! |
@steav Can you also have a look at the failing tests and add some basic ones for the new methods? |
lib/Service/CardService.php
Outdated
|
||
/* Notify user about the card assignment */ | ||
$card = $this->cardMapper->find($cardId); | ||
$this->notificationHelper->sendCardAssigned($card, $userId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only send a notification if the user assigns somebody else. If you assign yourself, there is no need for a notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@steav Really nice work done here. Tested and works fine. I just have one small enhancement commented inline. After that it is good to go. 😉 |
Signed-off-by: Steav <[email protected]>
Signed-off-by: Steav <[email protected]>
Signed-off-by: Steav <[email protected]>
Signed-off-by: steav <[email protected]>
Signed-off-by: steav <[email protected]>
Signed-off-by: steav <[email protected]>
Signed-off-by: steav <[email protected]>
e2e832d
to
476b15e
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Julius Härtl <[email protected]>
merged. 🚀 Thank you very much for your contribution @steav Further pull requests are always welcome. 😉 |
No description provided.