fix(dav): show canceled/moved occurrences in iMIP emails#60507
Open
ndo84bw wants to merge 1 commit into
Open
Conversation
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. German translations are included for the new label and date/time format strings (de.js/de.json/de_DE.js/de_DE.json) so DE users see "Abgesagt:" / "Verschoben:" and natural prepositions ("um", "von … auf …") immediately; other languages will be picked up via Transifex. 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. Signed-off-by: Nico Donath <ndo84bw@gmx.de>
ndo84bw
requested review from
a team,
ChristophWurst,
SebastianKrupinski and
tcitworld
as code owners
ndo84bw
requested review from
Altahrim,
nfebe,
provokateurin,
sorbaugh and
susnux
and removed request for
a team
Contributor
|
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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.New rows reuse the existing
addBodyListItemstyling (italic gray label + value on its own line), matchingWhen:,Title:, etc.Scope decisions (intentionally out of scope)
METHOD:REQUEST, so the user's mail client (Thunderbird etc.) keeps showing its own Accept/Decline buttons at the top of the email. Hiding only Nextcloud's body buttons would create inconsistent UI without solving the underlying problem. A proper fix would require Sabre'sTipBrokerto emit aMETHOD:CANCELwithRECURRENCE-IDfor single-occurrence deletions, which is a much larger change with cross-client compatibility risk and belongs in its own PR.Renamed:row for single-occurrence title changes (issue's Case D) is not included. The current behavior already renders the renamed occurrence's title + when in the body and now uses "updated" wording, which the issue notes is "actually useful". The spec under "Expected behavior" only enumerates Canceled and Moved. Happy to add it as a follow-up if maintainers prefer.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
German translations for the new strings (
Cancelled:,Moved:,%1$s at %2$s,%1$s from %2$s to %3$s,from %1$s to %2$s,from %1$s %2$s to %3$s %4$s) are included manually inapps/dav/l10n/de.{js,json}andapps/dav/l10n/de_DE.{js,json}so DE users see them on day one. I'm aware Transifex is the source of truth and the next sync will overwrite these - by then Transifex will have its own translation for the same strings, so the effect is the same. Happy to drop the manual entries if maintainers prefer the strict Transifex-only workflow.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
User C (DE, 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
User C (DE, 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
User C (DE, 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
User C (DE, 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.Locally run against this branch (
apps/dav/tests/unit/CalDAV/Schedule/IMipServiceTest.php):(The runner additionally emits
PHPUnit Deprecations: 1—"Your XML configuration validates against a deprecated schema"referring toapps/dav/tests/unit/phpunit.xml. Pre-existing on master, unrelated to this PR.)php-cs-fixer --dry-runreportsFound 0 of 3 files that can be fixedon the three modified PHP files.Checklist
IMipServiceTeststable33(issue affects 33.x, verified there)AI (if applicable)