nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.33k stars 4.06k forks source link

Users should be able to choose whether reminders are generated for events in the birthday calendar #1505

Open nursoda opened 8 years ago

nursoda commented 8 years ago

I'm not sure if it's correct in "server" component - but it does not only affect calendar or contacts, so it's either both - or "server", thus I report it here. Feel free to point me to re-insert it somewhere else.

Steps to reproduce Import a vcf file that contains data that contains birthday data.

Actual behaviour All birthday events in calendar are created with reminder (valarm).

Expected behaviour The admin should be able to set not to create any EVENT for contacts -or- The user should be able to opt not to create any EVENT for imported contacts -or- The admin should be able to set not to create any REMINDER for contacts -or- The user should be able to opt not to create remindes (valarms) while importing (contacts).

Server configuration Debian jessie fully patched, Apache 2.4.10 prefork, PHP 5.6.24, MySQL 5.5.52, Nextcloud 10.0 (stable) freshly installed via Webinstaller.

tcitworld commented 2 years ago

@tsvallender Hello, Many thanks for being interested. The part you're looking for is in the following file, I've highlighted where the alarm is being added: https://github.com/nextcloud/server/blob/4475658cc9f2aa57f7abe11f194d7d700d97592b/apps/dav/lib/CalDAV/BirthdayService.php#L291-L295

My personal opinion would be for this config to be handled in the Groupware user settings alongside availability settings (new in Nextcloud 23), instead of the calendar settings like said above.

We could use the setting introduced in https://github.com/nextcloud/calendar/pull/3001 (basic setting for all calendars) as a default, but I would prefer birthday calendars having their own setting.

Unrelated: The default alarm setting over CalDAV was removed for some reason from the draft-daboo-valarm-extensions draft, which became RFC 9074, however some projects seem to use it anyway. I'm tracking this in the wiki if anyone's curious.

tsvallender-zz commented 2 years ago

@tcitworld Thank you for the help! I'm not a PHP guy but I'm going to set up a dev environment and see how far I can get with this.

cneukom commented 2 years ago

Hi all,

I have started working on this issue (related PR: https://github.com/nextcloud/server/pull/33251).

The next step is adding the configuration option to the UI.

My personal opinion would be for this config to be handled in the groupware user settings alongside availability settings (new in Nextcloud 23), instead of the calendar settings like said above.

In my opinion, such a setting should be located close to the "Enable birthday calendar" option. So, I would propose one of the following:

  1. Place the "Birthday reminder" select box in the calendar settings right below the "Enable birthday calendar" option.
  2. Provide both settings ("Enable birthday calendar" and "Birthday reminder") in the groupware user settings and link to them from the calendar app settings. Here we could either:
    1. keep the option "Enable birthday calendar" in the calendar settings to provide a quick way of en-/disabling it,
    2. or we could remove it there and only provide the link to the groupware user settings.

I have a slight preference for the option 2.ii. What do you guys think? @tcitworld

nursoda commented 2 years ago

Thanks for your effort! I vote not to implement 2i as this leads to different howtos and users might expect it to be different and need to experiment instead of knowing straight away. I' also vote for 2ii, seems very clean. Also allows for (yet undeveloped) other apps to provide birthday calendar functionality. Only keeping it with the calendar app as in 1 seems wrong as it is a server feature. But there definitely must remain a link or at least an explanation that there is such feature and what may be configured where.

insilmaril commented 2 years ago

Suggestion looks good, thanks!

b-pfl commented 2 years ago

Just as an additional thought: As it turns out, the reminders are now also sent for regular calendar events, which is annoying if you use CalDav synchronization on your devices. So if there is a way to disable reminders, this should ideally be possible for all different types of reminders. I also tend more towards 2ii. Thanks for starting your work on this issue!

cneukom commented 2 years ago

Hey all,

So I have implemented option 2ii (see https://github.com/nextcloud/server/pull/33397 and https://github.com/nextcloud/calendar/pull/4402), hoping to resolve this issue :)

Just as an additional thought: As it turns out, the reminders are now also sent for regular calendar events, which is annoying if you use CalDav synchronization on your devices. So if there is a way to disable reminders, this should ideally be possible for all different types of reminders.

Hmm, I'm not sure, if I understand you correctly. You are probably referring to the feature discussed in https://github.com/nextcloud/calendar/issues/629... But as I see, there is already the option "No reminder", and it's chosen by default... Also, as stated earlier in this thread, reminders for regular events are usually created by the client, so it might be the case, that one of your clients adds these reminders (and you could probably disable them there).

nursoda commented 2 years ago

May all changes be made by just patching / without building? If not, I'll currently only be able to test a (test) release version.

cneukom commented 2 years ago

No, unfortunately, testing the changes in the calendar app requires you to build the calendar app. However, the only changes in the calendar app was the renaming of the link and the removal of the "Enable birthday calendar" checkbox in the settings panel. All the rest should be testable by checking out the feature branch of the server component (for checking the generated birthday events, you can use the latest version of the calendar app).

Naginreed commented 2 years ago

@tsvallender Hello, Many thanks for being interested. The part you're looking for is in the following file, I've highlighted where the alarm is being added:

https://github.com/nextcloud/server/blob/4475658cc9f2aa57f7abe11f194d7d700d97592b/apps/dav/lib/CalDAV/BirthdayService.php#L291-L295

Did i unterstand that correctly that the workaround so far is to:

  1. get rid of that 5 lines containing the $alarm string in the apps/dav/lib/CalDAV/BirthdayService.php
  2. save the BirthdayService.php file
  3. Go To "Calendar App" -> "Settings & Import" -> "Enable birthday calendar" and Uncheck the Box
  4. Check the "Enable birthday calendar" again to let the server newly build the Birthday Events without Alarms
  5. Resync on the other devices/apps which are read-only anyway for the birthday calendar? (therefore there should be no backsyncs or old entries left)

So far i got no errors or strange behavior on the server, caldav sync in outlook and DAVx5 Sync on Android and all the events now dont have a reminder. I get that this has to be done every update until the settings are permanently implemented, but anyone noticed any issues so far with a workaround like this?

fblz commented 1 year ago

It is almost one year since the patch was created by @cneukom. The patch itself was not merged but part of it seems to have made it into the dav app as of NC25. https://github.com/nextcloud/server/blob/v27.0.0/apps/dav/lib/CalDAV/BirthdayService.php#L455 now queries a user preference instead of being hard coded. Despite this, I was unable to find a corresponding UI element to configure the birthdayCalendarReminderOffset preference.

Am I overlooking something here?

For the time being, I set the preference via SQL: insert into oc_preferences (appid, configkey, configvalue, userid) values ('dav','birthdayCalendarReminderOffset','-PT0M','<USERNAME_HERE>');

With (some) possible values like this.

If you want to keep the alarm, but don't ring at 00:00 AM After every upgrade, in BirthdayService.php Change the line:

$alarm->add($vCal->createProperty('TRIGGER', '-PT0M', ['VALUE' => 'DURATION']));

To ring at 9AM on day of birthday: $alarm->add($vCal->createProperty('TRIGGER', 'PT9H', ['VALUE' => 'DURATION']));

To ring 1 day before (9AM): $alarm->add($vCal->createProperty('TRIGGER', '-PT15H', ['VALUE' => 'DURATION']));

2 days before (9AM) $alarm->add($vCal->createProperty('TRIGGER', '-P1DT15H', ['VALUE' => 'DURATION']));

1 week before (9AM) $alarm->add($vCal->createProperty('TRIGGER', '-P6DT15H', ['VALUE' => 'DURATION']));

Gezzo42 commented 1 year ago

I was able to remove the reminder by using the following command with Nextcloud 25: sudo -u www-data php occ user:setting <username> dav birthdayCalendarReminderOffset "" Perhaps it's a bit easier as the variant using directly the database.

dgsiegel commented 10 months ago

I was able to remove the reminder by using the following command with Nextcloud 25: sudo -u www-data php occ user:setting <username> dav birthdayCalendarReminderOffset "" Perhaps it's a bit easier as the variant using directly the database.

Works great even on Nextcloud 28. Be sure to recreate the Birthday calendar (e.g. disable and re-enable it), as existing entries won't be updated.

fblz commented 5 months ago

The question is, when will the frontend component get the update?

UlyssesZh commented 3 weeks ago

sudo -u www-data php occ user:setting <username> dav birthdayCalendarReminderOffset ""

This does not seem to work with Nextcloud 30.0.0? I re-enabled the birthday calendar after running this, but the reminder is still at 9:00 on every birthday.


Edit: Never mind. It still works. Somehow I have to refresh the webpage.

Larsn-Rinnstroem commented 2 weeks ago

After using that command and re-enabling, the birthday calendar is a shared calendar from me with myself instead of being a stand-alone on the list with the other calendars. Is that on purpose?

UlyssesZh commented 2 weeks ago

Just refresh the webpage, and the "shared" label will disappear.