feat(dav): store multiple default calendar alarms as JSON#61832
Conversation
|
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. |
1cda052 to
1e99678
Compare
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>
1e99678 to
24485e2
Compare
|
I pushed a fix for the psalm error. |
|
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
added
enhancement
feature: dav
feature: caldav
|
Thank you for the PR |
|
Hello there, 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.) |
3 participants
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
3. to review, feature component)stable32)AI (if applicable)