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

Calendar events email reminders #3044

Merged
merged 10 commits into from Aug 15, 2019
Merged

Calendar events email reminders #3044

merged 10 commits into from Aug 15, 2019

Conversation

@tcitworld
Copy link
Member

@tcitworld tcitworld commented Jan 12, 2017

fixes #3979

Okay, this seems ready.

What it looks like:

Email

Capture d’écran du 2019-03-16 18-37-02

Notifications

Capture d’écran du 2019-03-16 18-59-26
(Browser notifications come as well)


Edited by @georgehrke:

ToDo:

  • support recurring events
  • support repeat / duration
@mention-bot
Copy link

@mention-bot mention-bot commented Jan 12, 2017

@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.

Loading

apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
Loading
apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
Loading
apps/dav/lib/CalDAV/CalDavBackend.php Outdated Show resolved Hide resolved
Loading
Copy link
Member

@nickvergessen nickvergessen left a comment

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....

Loading

apps/dav/appinfo/app.php Outdated Show resolved Hide resolved
Loading
apps/dav/appinfo/app.php Outdated Show resolved Hide resolved
Loading
@georgehrke
Copy link
Member

@georgehrke georgehrke commented Feb 26, 2017

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
The calendar standard defines different kinds of alarms. Audio, display and email..
E-Mail alarms should obviously only be triggered when the user actually asked for an email :)

Loading

@rullzer
Copy link
Member

@rullzer rullzer commented Apr 3, 2017

What is the status here? Can this be reviewed soon?

Loading

@rullzer rullzer added this to the Nextcloud 13 milestone May 23, 2017
@georgehrke
Copy link
Member

@georgehrke georgehrke commented Jun 2, 2017

@tcitworld What's your schedule for this feature? :)

Loading

@tcitworld tcitworld force-pushed the dav-email-reminders branch from 11049d2 to e8c0e67 Jun 21, 2017
@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Jun 30, 2017

Will change this to work on the backend already used for Activities made by @nickvergessen

Loading

@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Jul 13, 2017

Should emails reminders be sent to all ATTENDEES or just nextcloud users ?
cc @georgehrke @jancborchardt @nickvergessen

Loading

@tcitworld tcitworld force-pushed the dav-email-reminders branch from e8c0e67 to 5554ec6 Jul 13, 2017
@codecov
Copy link

@codecov codecov bot commented Jul 13, 2017

Codecov Report

Merging #3044 into master will decrease coverage by 22.75%.
The diff coverage is 0%.

@@              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
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/CalDAV/Reminder/Notifier.php 0% <0%> (ø) 4 <4> (?)
apps/dav/lib/RootCollection.php 0% <0%> (-95.66%) 1 <0> (ø)
apps/dav/lib/CalDAV/CalDavBackend.php 0% <0%> (-86.04%) 223 <9> (+5)
apps/dav/lib/CalDAV/Reminder/ReminderJob.php 0% <0%> (ø) 8 <8> (?)
apps/dav/lib/AppInfo/Application.php 0% <0%> (-46.16%) 14 <2> (+1)
apps/dav/lib/CalDAV/Reminder/ReminderService.php 0% <0%> (ø) 6 <6> (?)
apps/dav/appinfo/app.php 0% <0%> (-26.32%) 0 <0> (ø)
apps/user_ldap/lib/Migration/UUIDFixUser.php 0% <0%> (-100%) 1% <0%> (ø)
...ib/private/Files/Config/UserMountCacheListener.php 0% <0%> (-100%) 2% <0%> (ø)
apps/comments/lib/AppInfo/Application.php 0% <0%> (-100%) 1% <0%> (ø)
... and 503 more

Loading

@georgehrke
Copy link
Member

@georgehrke georgehrke commented Jul 13, 2017

Should emails reminders be sent to all ATTENDEES or just nextcloud users ?

Did you check any RFC for an answer on that? :)

Loading

@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Jul 17, 2017

  When the action is "EMAIL", the alarm MUST include a "DESCRIPTION"
  property, which contains the text to be used as the message body,
  a "SUMMARY" property, which contains the text to be used as the
  message subject, and one or more "ATTENDEE" properties, which
  contain the email address of attendees to receive the message.  It
  can also include one or more "ATTACH" properties, which are
  intended to be sent as message attachments.  When the alarm is
  triggered, the email message is sent.

  [...]

  In an EMAIL alarm, the intended alarm effect is for an email
  message to be composed and delivered to all the addresses
  specified by the "ATTENDEE" properties in the "VALARM" calendar
  component.  The "DESCRIPTION" property of the "VALARM" calendar
  component MUST be used as the body text of the message, and the
  "SUMMARY" property MUST be used as the subject text.  Any "ATTACH"
  properties in the "VALARM" calendar component SHOULD be sent as
  attachments to the message.

@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.
What about people with whom the calendar is shared (a group, for instance) ? Should they automatically get the notifications too while not manually added to ATTENDEES (they can be a lot) ?

Maybe an issue in calendar repo to automatically add yourself as an attendee ? Or another one to add a group as attendee ?

Loading

@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Jul 17, 2017

Also, if someone plz have a look why my notifications aren't displayed. :'(

Loading

@jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Jul 17, 2017

Sharing an event with someone should mean they are invited to add themselves as attending. But not set them as attending automatically.

Loading

@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Jul 17, 2017

add themselves as attending

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?
Would also be great if someone could tell me how Apple's iCal handles this issue for instance.

Loading

@pixelipo
Copy link
Contributor

@pixelipo pixelipo commented Oct 13, 2017

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.

Loading

@tcitworld
Copy link
Member Author

@tcitworld tcitworld commented Oct 13, 2017

No bounty yet. Feel free to have a look and add answers to my questions.

Loading

@georgehrke
Copy link
Member

@georgehrke georgehrke commented Nov 11, 2017

Not sure if there is a little confusion here:

We are talking about different kinds of attendees here.
There can be attendees in VEvents, the one you see in the event editor, and attendees inside VAlarm blocks. The latter one are for sending out e-mail notifications. You can be an attendee inside the VAlarm block without being an actual attendee of the event.

What about people with whom the calendar is shared (a group, for instance) ?

See my comment in #679 (comment)

Should they automatically get the notifications too while not manually added to ATTENDEES (they can be a lot) ?

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.
If u are an actual attendee of the event, you should get a notification too.

Maybe an issue in calendar repo to automatically add yourself as an attendee ?

Yep, please open one :)

Or another one to add a group as attendee ?

Maybe. It's important to provide an opt-out mechanism here imho.

Loading

@MorrisJobke
Copy link
Member

@MorrisJobke MorrisJobke commented Nov 14, 2017

@nickvergessen @tcitworld @georgehrke It doesn't look like something for 13, right?

Loading

@MorrisJobke MorrisJobke removed this from the Nextcloud 13 milestone Nov 14, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 14 milestone Nov 14, 2017
CalDAV 18 automation moved this from In progress to Review in progress Aug 12, 2019
$message->setTo([$emailAddress]);
$message->useTemplate($template);

try {
Copy link
Member

@fancycode fancycode Aug 13, 2019

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.

Loading

Copy link
Member

@georgehrke georgehrke Aug 13, 2019

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.

Loading

Copy link
Member

@fancycode fancycode Aug 13, 2019

Choose a reason for hiding this comment

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

Ok sure.

Loading

@georgehrke georgehrke force-pushed the dav-email-reminders branch from 7885830 to de14dff Aug 13, 2019
Copy link
Member

@rullzer rullzer left a comment

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.

Loading

Loading
*
* @package OCA\DAV\CalDAV\Reminder
*/
class Backend {
Copy link
Member

@rullzer rullzer Aug 13, 2019

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.

Loading

if ($diff->invert) {
$title = $this->l10n->t('%s (in %s)', [$title, $diffLabel]);
} else {
$title = $this->l10n->t('%s (%s ago)', [$title, $diffLabel]);
Copy link
Member

@rullzer rullzer Aug 13, 2019

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.

Loading

/** @var ITimeFactory */
private $timeFactory;

public const REMINDER_TYPE_EMAIL = 'EMAIL';
Copy link
Member

@rullzer rullzer Aug 13, 2019

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?

Loading

Copy link
Member

@georgehrke georgehrke Aug 13, 2019

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?

Loading

Copy link

@remmesa remmesa Sep 10, 2019

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

Loading

}

switch($action) {
case '\OCA\DAV\CalDAV\CalDavBackend::createCalendarObject':
Copy link
Member

@rullzer rullzer Aug 13, 2019

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.

Loading

apps/dav/lib/Migration/Version1004Date20170825134824.php Outdated Show resolved Hide resolved
Loading
@georgehrke georgehrke removed this from the Nextcloud 18 milestone Aug 15, 2019
@georgehrke georgehrke added this to the Nextcloud 17 milestone Aug 15, 2019
@georgehrke georgehrke removed this from Review in progress in CalDAV 18 Aug 15, 2019
@rullzer
Copy link
Member

@rullzer rullzer commented Aug 15, 2019

Loading

Copy link
Member

@rullzer rullzer left a comment

Looks good.
Lets see if phan is happy and get it in.

Loading

tcitworld and others added 10 commits Aug 15, 2019
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
…ckground-job

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
…xtcloud 17

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
…nstead

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
…er, better then not showing reminders at all for now

Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
Signed-off-by: Georg Ehrke <developer@georgehrke.com>
@rullzer rullzer force-pushed the dav-email-reminders branch from a65c3cf to 4d28a45 Aug 15, 2019
@rullzer rullzer merged commit 6ef7ba2 into master Aug 15, 2019
2 of 3 checks passed
Loading
@rullzer rullzer deleted the dav-email-reminders branch Aug 15, 2019
@remmesa

This comment was marked as off-topic.

@remmesa

This comment was marked as off-topic.

@Mic92
Copy link

@Mic92 Mic92 commented Dec 4, 2019

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?

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment