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 sends duplicate notifications if there is an additional attendee [$250] #21370

Closed
dinosmm opened this issue Jun 11, 2020 · 40 comments · Fixed by #34909
Closed

Calendar sends duplicate notifications if there is an additional attendee [$250] #21370

dinosmm opened this issue Jun 11, 2020 · 40 comments · Fixed by #34909
Assignees
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: caldav Related to CalDAV internals feature: dav feature: emails

Comments

@dinosmm
Copy link

dinosmm commented Jun 11, 2020

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.

Steps to reproduce

The calendar normally sends one notification (whether push or email) around the specified time for events that do not have attendees.
This also works correctly for shared calendars, every user with whom the calendar & event is shared receives one notification.
However, if I add an event to a non-shared calendar and add an attendee, both I and the attendee will receive two notifications for each notification event. This happens both with push and email notifications.
Is this a known issue? A cursory search here in Github didn't come up with anything relating to calendar notifications.

  • Create calendar event on non-shared calendar
  • Add a notification for x minutes before event, set to email
  • Add an attendee that has an email address
  • Ensure 'send email' is checked on attendee

Expected behaviour:

Organiser and attendee each receive one single email notification.

Actual behaviour:

Organiser and attendee each receive TWO email notifications, at the same time.
This also happens if notifications are pushed to an installed Nextcloud app on a mobile device.

Setup:

Ubuntu 18.04.3 LTS
Nextcloud 18.0.4 via Snap
No security or setup warnings on admin panel

@mshayden
Copy link

Has anyone addressed this issue? I have reproduced this issue. I created a recurring meeting with 8 attendees with an email reminder set up one hour in advance. All 8 of us get 8 emails an hour before each meeting. This isn't the kind of behaviour I would expect.

@kesselb
Copy link
Contributor

kesselb commented Jul 12, 2020

cc @nextcloud/calendar

@kesselb kesselb added 0. Needs triage Pending check for reproducibility or if it fits our roadmap bug labels Jul 12, 2020
@tobyheywood
Copy link

I to am facing this issue too.
I see this for meetings in shared calendars and using the deck app.
The notifications are duplicated and can be seen in the WebUI notification area twice as well as via the push mechanism in the nextcloud app.

I'm running nextcloud 18.0.8. Wouldupgrade to 19.02 but waiting for there to be an upgrade to an associated app to ensure compatibility.

@jrobertp
Copy link

jrobertp commented Sep 4, 2020

I too observed this bug. The calendar event entry in the database appears to get duplicated for each attendee with a nextcloud login, so as to appear in their personal calendars. The calendar reminder entries get duplicated as well. And then each reminder triggers an email to all attendees.

@floatingpurr
Copy link

Same problem here. Any fix or workaround?

@c42759
Copy link

c42759 commented Jan 7, 2021

I have the same problem...
only happens for events where I have more people as attendees.

In my case I have a weekly meeting where are me and 2 more people, this event has 4 notifications:

  • Pop notification 1 day earlier
  • Pop notification 2 hours earlier
  • Email notification 1 day earlier
  • Email notification 2 hours earlier

For pop notifications, it's all ok, but for the email notification, I receive 3 emails with the same information.

@verbeckii
Copy link

Same problem with duplicate email event notifications
Is there any fix?

@j-ed
Copy link
Contributor

j-ed commented Jan 25, 2021

Is there any fix?

If a fix would be available this issue ticket would have been closed already 😉

@XueSheng-GIT
Copy link

I'm on a freshly installed NC21 and did encounter the same issue. The number of emails send is a multiple of the number of participants (12 participants means that the reminder is send to each person 12 times). You can imagine that the participants were a bit confused ;).

If there isn't a fix yet, maybe someone found a workaround!?

@RodSeq
Copy link

RodSeq commented Mar 10, 2021

Same issue here with NC 20.0.8 and Calendar App 2.1.3.

Hope this annoying bug can be solved in the next Nextcloud release.

@dinosmm
Copy link
Author

dinosmm commented Mar 10, 2021

I can confirm the issue persists for me as well with Nextcloud 20.0.7 and Calendar 2.1.3.
I saw someone mentioned cc'ing this to nextcloud/calendar, but I can't see it there, should I post a copy of this there?

@m4us1ne
Copy link

m4us1ne commented Mar 10, 2021

@dinosmm i did that, i wasnt sure where the issue came from, but after a bit of digging in the reminder mechanism i realized that its an issue with the server and not the calendar-app. the 250USD bounty from this issue was transfered onto #21370

@j-ed
Copy link
Contributor

j-ed commented Mar 10, 2021

@dinosmm @m4us1ne Due to the fact that the core function is part of the server package and has nothing to do with the Calendar app at all, how should it help to create a new issue in an other repository too? Additionally it won't change anything nor does it help in any way to confirm that the issue still persists in version XYZ, because this can easily been seen by checking the status of this issue which is still "Open" 😉

@m4us1ne
Copy link

m4us1ne commented Mar 10, 2021

@j-ed i did not knew about this issue, only after digging into the code. thats when i discovered where the error is and found this issue here

@DJaeger DJaeger changed the title Calendar sends duplicate notifications if there is an additional attendee Calendar sends duplicate notifications if there is an additional attendee [$250] Apr 1, 2021
@christlang
Copy link

christlang commented Apr 10, 2021

The problem in a picture

image

When the event gets a reminder in every calendar and in every reminder it is send to all attendees.

So, what is the problem:

  • Should only the owner have a reminder?
  • Should a reminder not send E-Mails to other nextcloud-users? (poor outsiders for events with 30 participants inside an organisation and only one outside that will get an e-mail from every insider...)
  • Should a reminder never send an E-Mail reminder to other participants?

Workaround at the moment:

  • owner of a event should not use reminders
  • owner can also remove reminders for all nextcloud-participants
  • if a participant is removing the reminder it is only its own

Todo:

  • is this only an E-Mail or also a Notification-Problem?
    image
  • The PushProvider.php is used for that
    • there is a config that can disable push at all ($this->config->getAppValue('dav', 'sendEventRemindersPush', 'no'))
    • occ config:app:set dav sendEventRemindersPush --value yes

@christlang
Copy link

there is also an option to disable event reminders at all

// EventReminderJob.php
		if ($this->config->getAppValue('dav', 'sendEventReminders', 'yes') !== 'yes') {
			return;
		}

can be disabled with

occ config:app:set dav sendEventReminders --value no

@dinosmm
Copy link
Author

dinosmm commented Apr 10, 2021

Surely the expected and desired behaviour should be that only one email reminder is sent to each participant on the day and time a reminder is set to be sent?
(unless I misunderstood your 'what is the problem' section @christlang )

@christlang
Copy link

so the reminder of the owner of the event should send E-Mails to non-next-cloud-users and the reminders of nextcloud-participants should inform that participants?

@dinosmm
Copy link
Author

dinosmm commented Apr 10, 2021

Apologies as I may not fully understand how it works programmatically.
The creator of the event sets the reminders for the event and adds attendees who may be nextcloud-users or outsiders.
I would expect reminders for nextcloud-users are then either a) initiated by the owner's calendar (in which case the owner's calendar should send one reminder to owner and each attendee) or b) copied to attendees' calendars and initiated from their own calendars (in which case the owner's calendar only sends reminders to the owner, one for each reminder set, and attendees' calendars handle each attendee's reminders).

For outsiders, the owner's calendar will have to initiate reminders.

Does the current design somehow preclude a single reminder being sent to each attendee? (In other words is it a deeper/more complicated change that is required to be made to the db design?)

@christlang
Copy link

I'm still learning how all is connected. I would expect, that the change doesn't need a db change at all. The information should already be available ;)

@christlang
Copy link

possible fix #26496 or at least a mitigation of the problem. Because the nextcloud-users still get a mail from their own reminder and one from the organizer.

@dinosmm
Copy link
Author

dinosmm commented Apr 10, 2021

possible fix #26496 or at least a mitigation of the problem. Because the nextcloud-users still get a mail from their own reminder and one from the organizer.

Nice one! Hope it can be successfully merged.

Does that mean that nextcloud-users can then go to the event in their own calendars and change/remove their own reminder? With this fix, would the recommended action for nextcloud-users be to manually remove reminders from events they are invited to?

@EhiOnime
Copy link

EhiOnime commented May 6, 2021

OK I think I found the solution to this problem
single-notifications.txt
by changing the SQL query in Reminder/Backend.php and adding a GroupBy clause to ensure that even if there are multiple entries in the database, only a single one is returned:

Diff Patch is attached it should be applied in your nextcloud/apps/dav directory

single-notifications.txt

@verbeckii
Copy link

OK I think I found the solution to this problem
single-notifications.txt
by changing the SQL query in Reminder/Backend.php and adding a GroupBy clause to ensure that even if there are multiple entries in the database, only a single one is returned:

Diff Patch is attached it should be applied in your nextcloud/apps/dav directory

single-notifications.txt

sorry, can you explain in a little more detail how can we use your fix?

@meonkeys
Copy link

meonkeys commented Sep 2, 2021

I'm seeing this with NC21 and calendar v2.3.3

@stefan123t
Copy link

@meonkeys did you verify this solution with your other issue ?

@meonkeys
Copy link

meonkeys commented Nov 7, 2021

Oh cool! I am now running nc v22.2.0 and calendar v2.3.4, so I should have this fix now, correct? I'll test again.

Does this fix existing events? (I'm guessing it does not). I'll try creating a new event to test.

And thank you, I totally forgot about this likely dup when I filed the other issue. I repled at #29434 (comment)

@meonkeys
Copy link

Done! Thanks. 👍

@dinosmm
Copy link
Author

dinosmm commented May 16, 2022

I have had this fix applied to my setup since it was published here and it was working, but just yesterday I added an event with an additional non-NC attendee and I got a duplicate email notification for that event. Has something been done to annul this fix? I checked and my /var/www/nextcloud/apps/dav/lib/CalDAV/Reminder/Backend.php still contains the additional groupBy line.

@miaulalala
Copy link
Contributor

Investigative results:

  1. The Notification Provider sends to ATTENDEES of the VEVENT not VALARM
  • check how our frontend creates ICSs for invitations, do we use VALARM properly?
  • Check other calendar implementations
  1. Depending on what we find in 1) Group by UID anyway, and bulk process the notifications. Only process once, and remove all related reminders. Caveat: recurrences, and recurrence exceptions.

@tcitworld could you please elaborate on #3044 a bit if you have time and remember?

Why did you choose VEVENT attendees, do you remember?

@miaulalala
Copy link
Contributor

miaulalala commented Oct 21, 2022

We do not write ATTENDEES in the VALARM:

ACTION:DISPLAY
TRIGGER;RELATED=START:-PT15H
END:VALARM

Google:

BEGIN:VALARM
ACTION:EMAIL
DESCRIPTION:This is an event reminder
SUMMARY:Alarm notification
ATTENDEE:mailto:miaugenau@gmail.com
TRIGGER:-P0DT2H0M0S
END:VALARM

Thunderbird offers no way to create an email notification, neither does the integrated Calendar on Ubuntu.

@miaulalala
Copy link
Contributor

miaulalala commented Oct 21, 2022

Processing happens in \OCA\DAV\CalDAV\Reminder\ReminderService::getRemindersForVAlarm

The duplications come from \OCA\DAV\CalDAV\Reminder\NotificationProvider\EmailProvider::getAllEMailAddressesFromEvent in the send()

@miaulalala
Copy link
Contributor

miaulalala commented Oct 21, 2022

Ok, current solution I would like to see:

  • Add a proper VALARM to our frontent in accordance with the RFC.
  • On that note, send an upstream PR to Sabre to extend the VALARM Validation rules to include the ATTENDEE property (and anything else that might be missing).
  • As some attendees might be external, we cannot insert reminders into the DB table, as that needs an existing calendar. So, rather than messing with the current creation process, which as far as I can tell does not do anything explicitly wrong anyway, \OCA\DAV\CalDAV\Reminder\NotificationProvider\EmailProvider::getAllEMailAddressesFromEvent could be rewritten to get the emails from the VALARM property instead of the VEVENT as soon as our frontend supports that.
  • Check the CalendarObjectReminderUpdaterListener and make sure only reminders from ORGANIZERs are processed if it is a scheduling event. If my VEVENT is an event where I am an ATTENDEE, the reminder should not be written UNLESS I explicitly created one on the event. Here, it is important to discern between individual alarms (No ATTENDEE properties) and scheduling alarms (Yes ATTENDEE properties)

As an example, imagine the following scenario and why the ATTENDEE should not hold a VALARM: I invite user Bob, who has a Nextcloud Calendar, from a different solution like Google. I specify an email reminder 15 minutes before the event. Bob adds the event via iMIP or other scheduling mechanisms. The email notification, if we were to send one from Nextcloud, would be sent twice - once from the ORGANIZER via Google Calendar, and once from the Nextcloud CalDav backend. It is not the responsibility of the CalDav Backend to send a reminder UNLESS Bob has added a reminder himself. This would be a VALARM without ATTENDEEs and should trigger the creation of a DB entry anyway.

@tcitworld
Copy link
Member

Why did you choose VEVENT attendees, do you remember?

Georg authored most of the code related to extracting emails from the event data but I'm guessing we already used vevent attendees (for invitations, notifications of changes, free/busy) and it would make the calendar app more complex (having to listen on vevent attendees changes to update them in valarm as well) - it would make no sense to have two different set of UIs to maintain that list.

Still, it would be a good idea to sync the two and consider only VALARM attendees in the future in server. Well, you just posted pretty much the same thing. :)

send an upstream PR to Sabre to extend the VALARM Validation rules to include the ATTENDEE property

Only when the action is EMAIL, yes. I'm not sure we can have this kind of logic with the current implementation: https://github.com/nextcloud/3rdparty/blob/3095d4062823f3f913d594f9ff313010ed55cd74/sabre/vobject/lib/Component/VAlarm.php#L110-L138

I think the above post looks just right.

@miaulalala
Copy link
Contributor

@st3iny when you touch the CalDAV backend, please backport all the way to 22 :)

@st3iny st3iny self-assigned this Oct 28, 2022
@st3iny st3iny added 2. developing Work in progress and removed 1. to develop Accepted and waiting to be taken care of labels Oct 28, 2022
@st3iny st3iny added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Nov 4, 2022
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 bug feature: caldav Related to CalDAV internals feature: dav feature: emails
Projects