Repository navigation
Conversation
|
Hi @ndo84bw Thank you for the PR, we will review these changes as soon as possible. In the mean time I noticed that your PR includes manually edited translation files. We use a external service for translations, and they should not be included in the PR. |
9ebe4b5 to
553fb78
Compare
|
Thanks for the heads up @SebastianKrupinski! Just force-pushed (
|
|
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.) |
When the organizer modifies a single occurrence of a recurring event, the iMIP notification email body did not indicate which occurrence was affected (issue nextcloud#60451). This adds two new rows to the invitation email when an iTip REQUEST diffs from the previous calendar state: - "Cancelled: <date>" for each EXDATE newly added to the master VEvent that has no corresponding RECURRENCE-ID override. - "Moved: <date> from <oldtime> to <newtime>" (same-day) or "Moved: from <olddate> <oldtime> to <newdate> <newtime>" (cross-day) for each override whose DTSTART differs from its RECURRENCE-ID (or from its previous DTSTART). The new rows reuse the existing addBodyListItem styling (italic gray label + value), matching the look of "When:", "Title:", etc. Also fixes the heading wording for single-occurrence updates: when a move adds a brand-new override VEvent, EventComparisonService cannot match it to an old VEvent and IMipPlugin previously fell through to the "would like to invite you" wording. The recipient is already on the existing series, so we now check the old VCalendar for the same UID and use the "admin updated the event" wording in that case. New user-facing strings are added in the source; translations are left to Transifex (no translation files committed). Implementation notes: - buildBodyData() now accepts optional ?VCalendar parameters so it can diff EXDATE values and sibling RECURRENCE-ID overrides against the previous calendar state. - Sabre's Property\ICalendar\DateTime::getDateTimes() returns \DateTimeImmutable, which is a sibling of \DateTime, not a subclass. Nextcloud's L10N::l() only matches \DateTime via instanceof and casts other input via (int), producing timestamp 1 and rendering as "1970-01-01 00:00:01" for any Immutable. A toMutableDateTime() helper converts before formatting. Assisted-by: ClaudeCode:claude-opus-4-7 Signed-off-by: Nico Donath <ndo84bw@gmx.de>
553fb78 to
4166e81
Compare
|
Your PR description is AI-written, so I'm closing it as per our AI policy: https://github.com/nextcloud/.github/blob/master/AI_POLICY.md#human-written-communication |
|
The policy is newer than my PR. One word, and I’m already justifying my work. It’s a shame that collaboration has only worked in the Calendar app so far. I have a very long list of features related to calendars and appointments, and I’m highly motivated to give something back to Nextcloud. |
|
You are free to just write the description yourself and re-open the PR. |
|
I saw the refactoring in #64942. I want to reopen this pull request #60507 later and build on top of it. I also saw that, since #64635, At the moment, only the primary instance is included in the mail. Could we clarify this now: Do you or the team agree with how I want to change the invitation mails? The pull request description shows the changes for deleting, moving, and renaming an occurrence. |
Summary
When the organizer modifies a single occurrence of a recurring event, the iMIP notification email did not indicate which occurrence was affected - the body only described the series. This PR makes the body convey the same information that Thunderbird already extracts from the ICS attachment.
Three concrete fixes:
Cancelled:row added for eachEXDATEnewly present on the masterVEVENTthat has no matchingRECURRENCE-IDoverride.Moved:row added for each override whoseDTSTARTdiffers from itsRECURRENCE-ID(or from its previousDTSTART). Same-day move renders as<date> from <oldtime> to <newtime>; cross-day move asfrom <olddate> <oldtime> to <newdate> <newtime>.EventComparisonServicecannot match a brand-new overrideVEventto an oldVEvent, so the previous code fell through to the "would like to invite you" wording for single-occurrence moves. We now consult the oldVCalendarfor the sameUIDto detect that case.Implementation notes
IMipService::buildBodyData()gained two optional?VCalendarparameters (new and old). The original two-argument signature is preserved (existing tests untouched).detectRecurrenceChanges(),collectExdates(),findOverrideEvents(),formatCanceledOccurrence(),formatMovedOccurrence(),toMutableDateTime(). One public helperfindMasterEvent()(needed byIMipPluginto detect existing series).Sabre\VObject\Property\ICalendar\DateTime::getDateTimes()returns\DateTimeImmutable, which is a sibling of\DateTime, not a subclass.OC\L10N\L10N::l()atlib/private/L10N/L10N.php:156checksif ($data instanceof \DateTime)and falls through to(int)$datafor anything else - silently casting an Immutable to1and rendering as1970-01-01 00:00:01. A smalltoMutableDateTime()helper converts before formatting. Worth considering as a separate hardening ofIL10N::l()so it accepts\DateTimeInterface, since any future caller wiring Sabre's parsed dates intol()will hit the same trap.Translations
The new format strings (
%1$s at %2$s,from %1$s to %2$s, etc.) are intentionally generic so they can be translated naturally per locale; TRANSLATORS comments are inline with eachl10n->t()call to give context.Test plan
Setup (applies to all four tests):
en, has not accepted the series yet.de, has accepted the series (with Thunderbird).stable33code (Before), once with this PR applied (After).Test 1 - delete a single occurrence (issue Case A & C combined)
Action: User A deletes occurrence on
<DATE>via "Delete this occurrence" in the Nextcloud Calendar web UI.User B (EN, not accepted)
Subject (unchanged)
Body
Expected change After this PR: new
Cancelled: <date> at <time>(B) /Abgesagt: <datum> um <zeit>(C) row appears in the body. Subject and heading already used "updated" wording before the PR for the delete case, so unchanged.Test 2 - move a single occurrence, same day (issue Case B)
Action: User A moves the occurrence on
<DATE>from<oldtime>to<newtime>(same day, time-only change) in the Nextcloud Calendar web UI.User B (EN, not accepted)
Subject
Body
Expected change After this PR:
Moved: <date> from <oldtime> to <newtime>(B) /Verschoben: <datum> von <alt> auf <neu>(C) row appears in the body.Test 3 - move a single occurrence, different day and time (additional, not in original issue)
Action: User A moves the occurrence on
<DATE>to a different day and a different time in the Nextcloud Calendar web UI.User B (EN, not accepted)
Subject
Body
Expected change After this PR: new
Moved: from <olddate> <oldtime> to <newdate> <newtime>(B) /Verschoben: von <altdatum> <altzeit> auf <neudatum> <neuzeit>(C) row.Test 4 - rename a single occurrence (issue Case D)
Action: User A changes the title of the occurrence on
<DATE>to a new title in the Nextcloud Calendar web UI.User B (EN, not accepted)
Subject
Body
Expected change After this PR: heading switches from "would like to invite you" to "updated the event" for User B. No dedicated
Renamed:row - see Scope decisions.Rendered styling (screenshots)
Text-only paste does not convey that the new
Cancelled:/Moved:labels are styled identically to the existingWhen:/Title:rows (italic, gray#777, on a line of their own above the value). The screenshots below show the rendered HTML email.Cancelled row (after Test 1):
Moved row (after Test 2):
Unit tests
Added six cases in
apps/dav/tests/unit/CalDAV/Schedule/IMipServiceTest.php:testBuildBodyDataDetectsCanceledOccurrencemeeting_canceled_occurrencespopulated with correct datetestBuildBodyDataIgnoresPreexistingExdatestestBuildBodyDataDetectsMovedOccurrencemeeting_moved_occurrencespopulated (same-day)testBuildBodyDataDetectsMovedOccurrenceAcrossDaystestBuildBodyDataReturnsNoExtraKeysWhenNothingRecurrenceChangedtestFindMasterEventSkipsOverridesThe l10n stub in these tests intentionally type-hints
\DateTime(not\DateTimeInterface) so any regression of theDateTimeImmutableissue would surface as aTypeErrorrather than silently producing epoch-0 output.Checklist
IMipServiceTeststable33(issue affects 33.x, verified there)AI (if applicable)