Skip to content

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

Open
rich0 wants to merge 1 commit into
nextcloud:masterfrom
rich0:upstreampr/pluralcalendardefaults
Open

feat(dav): store multiple default calendar alarms as JSON#61832
rich0 wants to merge 1 commit into
nextcloud:masterfrom
rich0:upstreampr/pluralcalendardefaults

Conversation

@rich0

@rich0 rich0 commented Jul 5, 2026

Copy link
Copy Markdown

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

AI (if applicable)

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

@rich0

rich0 commented Jul 5, 2026

Copy link
Copy Markdown
Author

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 rich0 force-pushed the upstreampr/pluralcalendardefaults branch 2 times, most recently from 1cda052 to 1e99678 Compare July 5, 2026 15:37
@rich0 rich0 marked this pull request as ready for review July 5, 2026 15:38
@rich0 rich0 requested review from come-nc, leftybournes, provokateurin and salmart-dev and removed request for a team July 5, 2026 15:38
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 rich0 force-pushed the upstreampr/pluralcalendardefaults branch from 1e99678 to 24485e2 Compare July 6, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant