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.

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

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

This was implemented in a separate file - I'm not sure if that is the correct pattern or not.

Also, the migration process was tweaked a bit. I'm not super-familiar with SQL in PHP but I think this will execute as an UPDATE query DB-side which I would hope should perform fairly quickly even on a very large table.

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?

SebastianKrupinski · 2026-08-10T23:49:58Z

@rich0

Yes that is fine, these are custom properties that only exist in our calendar client. We'll make these available only in 35+ and can feature gate the calendar to 35+

rich0 · 2026-08-11T13:02:31Z

I rebased all of this on master and added one commit so that you can see the changes, though honestly it is probably easier to just look at the combined result since it is far simpler now. I will note that the migration versioning now has a conflict but I'm guessing it would make more sense to tweak that once we're ready to merge. The calendar app itself has a similar versioning open item around the version checks as I wasn't sure what is used during development.

Note that this version of server will not work with the calendar default settings in the current version of the app. As-written the app itself is backwards-compatible, so the merge order lib-app-server is safe. This is the simplest component though if you can tolerate breaking the default setting feature.

SebastianKrupinski · 2026-08-11T14:49:15Z

+	 * Encode a single legacy trigger int as [{"trigger":N,"action":"DISPLAY"}]
+	 * for all rows still missing the JSON column value.
+	 */
+	private function migrateLegacyIntColumn(string $legacyColumn, string $jsonColumn): void {


Nice, this is definitely more efficient.

Normally, manually creating json like this would be a no-no but this is the most efficient for a one time migration on a multi-database platform

Works correctly

SebastianKrupinski · 2026-08-11T14:53:58Z

+			->where($qb->expr()->isNotNull($legacyColumn))
+			->andWhere($qb->expr()->isNull($jsonColumn));


Nice. It will continue where it left of when interrupted

SebastianKrupinski · 2026-08-11T15:00:24Z

@rich0

I modified the migration version numbers are the dav app is now 2.0.... and also bumped the app version to trigger the migration

rich0 · 2026-08-11T15:05:01Z

@rich0

I modified the migration version numbers are the dav app is now 2.0.... and also bumped the app version to trigger the migration

Oh thanks. For future reference, is master not the baseline for PRs like these? I could have rebased that on something else.

SebastianKrupinski · 2026-08-11T15:11:08Z

@rich0
I modified the migration version numbers are the dav app is now 2.0.... and also bumped the app version to trigger the migration

Oh thanks. For future reference, is master not the baseline for PRs like these? I could have rebased that on something else.

Master is the reference, but each app (built in or not) has a version. Dav is now on 2.0...

Meaning, migration -> Version(app version)(date)(time in seconds or a random sequential number)

Signed-off-by: Richard Freeman <rich@rich0.org> Assisted-by: Grok:grok-4

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

rich0 mentioned this pull request Aug 11, 2026

feat(dav): store multiple default calendar alarms as JSON rich0/nextcloud-server#6

Open

4 tasks

rich0 force-pushed the upstreampr/pluralcalendardefaults branch 3 times, most recently from fe906cf to 18b39c1 Compare August 11, 2026 12:14

SebastianKrupinski approved these changes Aug 11, 2026

View reviewed changes

SebastianKrupinski force-pushed the upstreampr/pluralcalendardefaults branch from 18b39c1 to 3219465 Compare August 11, 2026 14:58

SebastianKrupinski force-pushed the upstreampr/pluralcalendardefaults branch 2 times, most recently from f6a2367 to bc31776 Compare August 11, 2026 15:04

SebastianKrupinski force-pushed the upstreampr/pluralcalendardefaults branch 2 times, most recently from 2ab8506 to 19b3aef Compare August 11, 2026 17:47

SebastianKrupinski added the AI assisted label Aug 11, 2026

feat(dav): store multiple default calendar alarms as JSON

2920c37

Signed-off-by: Richard Freeman <rich@rich0.org> Assisted-by: Grok:grok-4

SebastianKrupinski force-pushed the upstreampr/pluralcalendardefaults branch from 19b3aef to 2920c37 Compare August 11, 2026 17:51

		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';

		->where($qb->expr()->isNotNull($legacyColumn))
		->andWhere($qb->expr()->isNull($jsonColumn));

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rich0 commented Aug 10, 2026

Uh oh!

SebastianKrupinski commented Aug 10, 2026

Uh oh!

rich0 commented Aug 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SebastianKrupinski commented Aug 11, 2026

Uh oh!

rich0 commented Aug 11, 2026

Uh oh!

SebastianKrupinski commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants