feat(dav): store multiple default calendar alarms as JSON by rich0 · Pull Request #61832 · nextcloud/server

rich0 · 2026-07-05T15:03:11Z

Summary

This PR is hand-written (other than the commit comment below, which was edited by me), but the code changes were created by AI.

I am a novice at PHP and new to Nextcloud development so I invite scrutiny. I noted questions about nextcloud dev conventions and the design below. Everything was reviewed and tested by me with both the stable server and the new app+lib, plus the stable app and the new server.

The intent of this feature is to extend the recent default calendar reminder feature by allowing multiple reminders to be set as well as the notification type for each. Only relative reminder intervals are supported as I didn't think absolute reminders made much sense in a template. This requires changes in server, cdav-library, and the calendar app.

The server revisions are backwards-compatible with the current stable app, and the app revisions are backwards-compatible with the current stable server. Within the server component, updates from legacy apps will update the notification time on the first defined alarm if there are multiple alarms, and legacy apps will be able to read the notification time on the first alarm. This should generally result in the new properties taking precedence if both are getting updated consistently.

The data migration itself does not have unit testing coverage, but I think this is the norm. As long as the schema changes are made, both the read and write paths in the server should migrate data as it is used if the migration did not complete.

The calendar app has a dependency on the updated cdav-library, which is not reflected in package.json as I'm not sure what the convention is for bumping these revisions in tandem.

Related to:
nextcloud/calendar#8567
nextcloud/cdav-library#1066

Commit comment:
Add default_alarms_pday/fday TEXT columns and CalDAV properties default-alarms-part-day/full-day for alarm templates with trigger and DISPLAY|EMAIL action. Legacy integer columns remain in sync for NC34 clients. Includes migration from existing single-int defaults.

Assisted-by: Grok:grok-4

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes (n/a? - only visible changes are in app)
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes) (n/a)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

The content of this PR was partly or fully generated using AI

rich0 · 2026-07-05T15:06:43Z

The checklist mentions documentation. A quick glance suggests the calendar screenshots are already dated after the initial feature was added in v34, so they definitely could use a refresh after this change as well. I do not know if a focused team typically does this or if this is solicited from contributors. Let me know what the usual approach is and I'll see if I can chip in if needed. The UI for the app was borrowed from the regular event notifications UI so it will be very consistent.

Add default_alarms_pday/fday TEXT columns and CalDAV properties default-alarms-part-day/full-day for alarm templates with trigger and DISPLAY|EMAIL action. Legacy integer columns remain in sync for NC34 clients. Includes migration from existing single-int defaults. Assisted-by: Grok:grok-4 Signed-off-by: Richard Freeman <rich@rich0.org>

rich0 · 2026-07-06T11:03:13Z

I pushed a fix for the psalm error.

rich0 · 2026-07-15T12:24:39Z

Since this checklist asks for a screenshot, here is the screenshot of the final feature, though the UI component is actually implemented in the calendar app:

ChristophWurst · 2026-07-16T12:46:44Z

Thank you for the PR

github-actions · 2026-07-21T02:14:25Z

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

SebastianKrupinski · 2026-08-10T02:07:45Z

 		'{http://apple.com/ns/ical/}calendar-color' => ['calendarcolor', 'string'],
 		'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}deleted-at' => ['deleted_at', 'int'],
 		'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarm-part-day' => ['default_alarm_pday', 'int'],
 		'{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarm-full-day' => ['default_alarm_fday', 'int'],


Since you are changing the property name to "default-alarms-part-day" all the code using "default_alarm_pday" needs to be removed. Otherwise after this PR is merged all the old code is just dead code.

SebastianKrupinski · 2026-08-10T02:08:15Z

+	public const DEFAULT_ALARMS_PART_DAY_PROPERTY = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarms-part-day';
+	public const DEFAULT_ALARMS_FULL_DAY_PROPERTY = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarms-full-day';


This should be part of the property map below.

SebastianKrupinski · 2026-08-10T02:15:12Z

+					case self::DEFAULT_ALARMS_PART_DAY_PROPERTY:
+						$encoded = DefaultCalendarAlarms::validateAndEncode($propertyValue);
+						$newValues['default_alarms_pday'] = $encoded;
+						$newValues['default_alarm_pday'] = DefaultCalendarAlarms::legacyIntFromJson($encoded);


Remove all the legacy encodings.

SebastianKrupinski · 2026-08-10T02:16:00Z

+	/**
+	 * Returns the JSON string exposed on CalDAV, synthesizing from legacy int when needed.
+	 */
+	public static function formatForCalDav(?string $json, ?int $legacyInt): ?string {


Remove all the legacy conversion code.

SebastianKrupinski · 2026-08-10T02:17:21Z

You need a second migration step to remove the old fields

SebastianKrupinski · 2026-08-10T02:19:46Z

+		while (($row = $result->fetchAssociative()) !== false) {
+			$id = (int)$row['id'];
+
+			if ($row['default_alarms_pday'] === null && $row['default_alarm_pday'] !== null) {
+				$this->updateAlarmsColumn($id, 'default_alarms_pday', DefaultCalendarAlarms::encodeFromLegacyInt((int)$row['default_alarm_pday']));
+			}
+
+			if ($row['default_alarms_fday'] === null && $row['default_alarm_fday'] !== null) {
+				$this->updateAlarmsColumn($id, 'default_alarms_fday', DefaultCalendarAlarms::encodeFromLegacyInt((int)$row['default_alarm_fday']));
+			}
+		}


I get this is necessary but this is also a very expensive operation, you need to imaging this migration running on a table with a million rows to update. It should take 10min+

SebastianKrupinski · 2026-08-10T02:23:21Z

 		}

+		if (isset($properties[self::DEFAULT_ALARMS_PART_DAY_PROPERTY])) {
+			$encoded = DefaultCalendarAlarms::validateAndEncode($properties[self::DEFAULT_ALARMS_PART_DAY_PROPERTY]);


We should not be validating data in the storage backend, the job of the storage backed in to store and retrieve data, not to make sure that that data is correct.

rich0 · 2026-08-10T10:25:01Z

@SebastianKrupinski Before I make those changes I just wanted to confirm that it was acceptable that the server-side not be backwards-compatible with any clients that added the new property from the last release. That was the reason for preserving all that legacy code, and also not putting the new fields in the property map (to allow for the conversion functionality).

The other reason for it was to allow the data migration to be optional or run while the server is live, since it is expensive. I'm not sure if that is something that Nextcloud can actually take advantage of, but it was part of the design.

I'm more than happy to make those changes once you confirm this is ok. While I'm at it, the calendar app and library components also have similar compatibility code for older Nextcloud versions - do you want me to also remove that, or do we want the clients to remain backwards-compatible?

This was referenced Jul 5, 2026

feat(calendar): plural default calendar reminders nextcloud/calendar#8567

Open

feat: parse and persist plural default calendar alarms nextcloud/cdav-library#1066

Open

rich0 force-pushed the upstreampr/pluralcalendardefaults branch 2 times, most recently from 1cda052 to 1e99678 Compare July 5, 2026 15:37

rich0 marked this pull request as ready for review July 5, 2026 15:38

rich0 requested review from a team, ChristophWurst, SebastianKrupinski and tcitworld as code owners July 5, 2026 15:38

rich0 requested review from come-nc, leftybournes, provokateurin and salmart-dev and removed request for a team July 5, 2026 15:38

rich0 force-pushed the upstreampr/pluralcalendardefaults branch from 1e99678 to 24485e2 Compare July 6, 2026 09:43

susnux added the community pull requests from community label Jul 6, 2026

come-nc removed their request for review July 6, 2026 13:39

ChristophWurst added enhancement feature: dav feature: caldav Related to CalDAV internals 3. to review Waiting for reviews labels Jul 16, 2026

github-actions Bot added the feedback-requested label Jul 21, 2026

SebastianKrupinski requested changes Aug 10, 2026

View reviewed changes

		public const DEFAULT_ALARMS_PART_DAY_PROPERTY = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarms-part-day';
		public const DEFAULT_ALARMS_FULL_DAY_PROPERTY = '{' . \OCA\DAV\DAV\Sharing\Plugin::NS_NEXTCLOUD . '}default-alarms-full-day';

Uh oh!

Uh oh!

Conversation

rich0 commented Jul 5, 2026

Summary

Checklist

AI (if applicable)

Uh oh!

rich0 commented Jul 5, 2026

Uh oh!

rich0 commented Jul 6, 2026

Uh oh!

rich0 commented Jul 15, 2026

Uh oh!

ChristophWurst commented Jul 16, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

rich0 commented Aug 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants