-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Calendar events email reminders #3044
Conversation
@tcitworld, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @rullzer and @icewind1991 to be potential reviewers. |
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.
What I dislike about this is all the duplication of notifications we will get with that.
The plan is to have notifications integrated into the mobile and desktop clients as well. So when you have an event coming up, you will have 4 notifications (laptop nextcloud, laptop calendar, phone nextcloud, phone calendar).
That is why I always stayed away from this notifications. Might make sense to have them in the web ui, but I think most people have at least one client running which already gives the notifications....
@nickvergessen |
What is the status here? Can this be reviewed soon? |
@tcitworld What's your schedule for this feature? :) |
11049d2
to
e8c0e67
Compare
Will change this to work on the backend already used for Activities made by @nickvergessen |
Should emails reminders be sent to all |
e8c0e67
to
5554ec6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3044 +/- ##
=============================================
- Coverage 53.77% 31.01% -22.76%
+ Complexity 22746 22368 -378
=============================================
Files 1405 1383 -22
Lines 86898 85091 -1807
Branches 1328 1329 +1
=============================================
- Hits 46727 26394 -20333
- Misses 40171 58697 +18526
|
Did you check any RFC for an answer on that? :) |
@georgehrke @jancborchardt Today, when you set an alarm, you are not automatically added as an attendee, but I guess you would like to get the notifications anyway. Maybe an issue in calendar repo to automatically add yourself as an attendee ? Or another one to add a group as attendee ? |
Also, if someone plz have a look why my notifications aren't displayed. :'( |
Sharing an event with someone should mean they are invited to add themselves as attending. But not set them as attending automatically. |
Would like to discuss more on that on the calendar app repo, maybe a easier way to add yourself as an attendee instead of typing your own username? A checkbox? |
Hi @tcitworld any status on this? It's a killer feature for me and if there's a bounty, I'm willing to pitch in. Also if you need some dev/design help, let me know. |
No bounty yet. Feel free to have a look and add answers to my questions. |
Not sure if there is a little confusion here: We are talking about different kinds of attendees here.
See my comment in #679 (comment)
They should be able to manage their own VAlarm block for each event and hence be able to add themselves for reminders. Not sure about the default here.
Yep, please open one :)
Maybe. It's important to provide an opt-out mechanism here imho. |
@nickvergessen @tcitworld @georgehrke It doesn't look like something for 13, right? |
apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php
Outdated
Show resolved
Hide resolved
$message->setTo([$emailAddress]); | ||
$message->useTemplate($template); | ||
|
||
try { |
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.
While you're at it: it would be great if you could dispatch an event here containing the message. This can be subscribed by other apps so they can modify / extend the message before it is sent out.
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 we do that in a separate PR please? We are planning to release a beta of 17 later this week, so this has to get in if we want it in 17.
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.
Ok sure.
7885830
to
de14dff
Compare
apps/dav/lib/CalDAV/Reminder/NotificationProvider/EmailProvider.php
Outdated
Show resolved
Hide resolved
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.
In general I'm all for merging this. I'm sure there are things we didn't think of yet. But lets get this in before it gets to huge.
I just wonder about the migration change.
* | ||
* @package OCA\DAV\CalDAV\Reminder | ||
*/ | ||
class Backend { |
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.
I'm fine with this for now. But it might make sense to try to eventually convert this to the Entity stuff of the appframework.
if ($diff->invert) { | ||
$title = $this->l10n->t('%s (in %s)', [$title, $diffLabel]); | ||
} else { | ||
$title = $this->l10n->t('%s (%s ago)', [$title, $diffLabel]); |
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.
So i guess we either have to give good hints to the translators. Or we have to somehow fix this later to have proper translations. Because I can already see languages where this looks weird.
/** @var ITimeFactory */ | ||
private $timeFactory; | ||
|
||
public const REMINDER_TYPE_EMAIL = 'EMAIL'; |
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.
If we have the types here. Shouldn't we just reference them from the providers themselfs?
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.
as in have a method getSupportedTypes
?
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.
if you configure email reminder alert to calendar on nextcloud already please send step to configure take me
} | ||
|
||
switch($action) { | ||
case '\OCA\DAV\CalDAV\CalDavBackend::createCalendarObject': |
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.
Ah magic string ;)
But yeah lets do it for now.
Depends on nextcloud/3rdparty#321 |
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.
Looks good.
Lets see if phan is happy and get it in.
Signed-off-by: Thomas Citharel <[email protected]>
…ckground-job Signed-off-by: Georg Ehrke <[email protected]>
Signed-off-by: Georg Ehrke <[email protected]>
Signed-off-by: Georg Ehrke <[email protected]>
…xtcloud 17 Signed-off-by: Georg Ehrke <[email protected]>
…nstead Signed-off-by: Georg Ehrke <[email protected]>
…er, better then not showing reminders at all for now Signed-off-by: Georg Ehrke <[email protected]>
Signed-off-by: Georg Ehrke <[email protected]>
Signed-off-by: Georg Ehrke <[email protected]>
Signed-off-by: Georg Ehrke <[email protected]>
a65c3cf
to
4d28a45
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Is this part of 17.0.1? EDIT: It is. However I don't get notifications also sending emails works. I suppose those are triggered by cronjobs? |
fixes #3979
Okay, this seems ready.
What it looks like:
Email
Notifications
(Browser notifications come as well)
Edited by @georgehrke:
ToDo: