Repository navigation
Conversation
9b11091 to
0c31259
Compare
lucs7
changed the title
0c31259 to
a7f21b1
Compare
Every reservation email attached an ICS with METHOD:REQUEST, so
Outlook offered to add the event again when a reservation was
cancelled. Clients also differ on what they accept: Outlook only
recognises REQUEST, and a REQUEST needs an ATTENDEE to follow RFC 5546.
Add an IcsMethod enum and let each email class pick its method:
CANCEL (STATUS:CANCELLED) for deletion emails and REQUEST for
everything else. Feeds and exports keep PUBLISH.
REQUEST and CANCEL name the site's default from address as the
organizer and whoever receives the email as the only attendee: the
owner, an invitee, a guest or a share recipient. The attendee needs
action and no RSVP is requested, because invitations are answered
through the links in the email.
The ICS no longer carries a SEQUENCE. Thunderbird did not match a
cancellation to its event when SEQUENCE was set, and a missing one
counts as 0, so DTSTAMP orders the messages.
A series without BookedBy() (from BuildSeries()) now falls back to the
owner's session, with a timezone, instead of failing in the summary
formatting. The log for a missing default from address is kept.
The owner's update email used the subject meant for administrators
("Notification: Reservation Updated for ..."). Use the one for the
owner, like the created and deleted emails do.
Assisted-by: Claude:claude-sonnet-5-5
a7f21b1 to
d13c5f7
Compare
JohnVillalovos
left a comment
JohnVillalovos
left a comment
There was a problem hiding this comment.
This review was written by Codex (OpenAI’s AI coding assistant).
[P1] Preserve an incrementing SEQUENCE for cancellation messages
Pages/Export/CalendarExportDisplay.php, lines 70–73
Switching deletion emails to METHOD:CANCEL is appropriate, but removing SEQUENCE makes those messages noncompliant with RFC 5546 §3.2.5, which requires a sequence number and requires it to increase when cancelling an event. A later DTSTAMP does not replace that requirement. Clients enforcing it may reject the cancellation and leave the reservation on the recipient’s calendar.
Please emit a monotonically increasing revision for each event, including cancellations. Replace the assertion requiring SEQUENCE to be absent with coverage for a create → update → cancel lifecycle. The reported Thunderbird behavior deserves investigation, but dropping a required property is not a portable fix.
The commit message and PR description also say feeds and exports “keep PUBLISH,” although the previous implementation emitted REQUEST; please describe this as a change.
Validation against d13c5f72a: all 1,884 PHPUnit tests completed without failures, with warnings/deprecations and skipped/incomplete tests. PHPStan reported zero errors. I did not directly test calendar clients.
JohnVillalovos
left a comment
JohnVillalovos
left a comment
There was a problem hiding this comment.
This review was written by Claude Code (Anthropic's AI coding assistant).
Thanks for this. Sending CANCEL for deletions and naming the recipient as ATTENDEE is the right direction, and the data-driven test over every email type is a good addition. I agree with the earlier review about SEQUENCE and the "keep PUBLISH" wording. This review covers the remaining points and doesn't repeat those.
Checked against d13c5f72a: the full PHPUnit suite passes (1884 tests), PHPStan reports 0 errors, and php-cs-fixer is clean. I didn't test any calendar clients.
Must fix
1. Split the unrelated fixes into their own commits. Two independent bug fixes ride along with the iTip change:
- the
BookedBy()null fallback inReservationEmailMessage::PopulateIcsAttachment(). This is a real crash fix for series loaded throughBuildSeries(), such as the approval email. - the
ReservationUpdatedEmailsubject change, from the admin subject to the owner subject.
Both are good changes. As separate fix: commits they can be reviewed, bisected and reverted on their own.
2. Document every behaviour change in the commit message and PR description. Besides the PUBLISH point from the earlier review, these changes aren't mentioned:
- Feeds and
.icsdownloads now putSENT-BY="mailto:<default from>"onORGANIZER. - The Outlook workaround from
a886d25b4is removed. When the viewer owned the reservation,ORGANIZERused to be rewritten touser-noreply@domain. Now feeds and downloads use the owner's real address. WithMETHOD:PUBLISHthis is probably fine, but the PR says Outlook wasn't retested. Please check that importing a downloaded.icsas the reservation owner still works in Outlook.
The sentence "Outlook only recognises REQUEST" is also misleading, because this PR relies on Outlook handling CANCEL. I think the point is that Outlook doesn't treat a PUBLISH attachment as an invitation. Please reword it.
Should fix
3. Pages/Export/CalendarExportDisplay.php:60 and :103: !empty($res->OrganizerSentBy) is used as a hidden flag meaning "the owner is visible and a default from address is set". An explicit property, or passing the attendee into Render(), would make the organizer/attendee decision easier to follow.
4. Pages/Export/CalendarExportDisplay.php:102: emails always set AttendeeEmail, so only the tests reach the $res->AttendeeEmail ?? $res->OrganizerEmail fallback. Making the owner the attendee by default would hide a caller that forgot to set one. Please remove the fallback, or document when it applies.
5. Empty email.default.from.address: a REQUEST then goes out with the owner as ORGANIZER and no ATTENDEE. That breaks the RFC 5546 rule this PR cites, and it's the "you are the organizer" situation the removed workaround guarded against. The setting has a default value, so this is rare. Still, the "missing default from address" log only fires in the privacy-hidden branch, not this one.
6. ReservationEmailMessage.php: $display->Render([$icsView], null, $method) passes a positional null. Project guidelines ask for named arguments: Render(reservations: [$icsView], method: $method).
7. tests/lib/Email/Messages/ReservationEmailMessageTest.php:71: emailTypes() doesn't list ParticipantDeletedEmail. That's the most common CANCEL path, because every participant gets one when a reservation is deleted. It inherits CANCEL through InviteeRemovedEmail, so the test would pass. Please add it so a change to that inheritance is caught.
Nits
- The tests call the protected method two ways: through
(fn () => $this->PopulateIcsAttachment(...))->call($message), and through the existingTestReservationEmailMessage::PopulateIcsAttachmentForTest(). Please pick one. - The
foreachoverrequire_onceat the top ofReservationEmailMessageTest.phpis unusual for this codebase. CalendarExportPresenterTestuses the fully qualified#[\PHPUnit\Framework\Attributes\DataProvider], whileReservationEmailMessageTestimports it withuse.PARTSTAT=NEEDS-ACTIONfor participants who have already accepted, andROLE=REQ-PARTICIPANTfor share recipients, aren't accurate.ACCEPTEDandNON-PARTICIPANTwould describe them better.- With
ORGANIZERset to the site address, invitees no longer see who booked the reservation in their calendar entry. This is a reasonable trade-off, but please mention it in the description.
Follow-up issues (not blockers, they predate this PR)
- Invitees and share recipients get a
REQUESTbut never aCANCELwhen the whole reservation is deleted.DeleteReservationNotificationServicedoesn't notify them, so their calendar entries stay behind. - Declining or leaving through the LibreBooking links sends no
CANCEL, so the event stays in that user's calendar. - Creating or deleting a recurring series only sends an ICS for the current instance.
- Even with
RSVP=FALSE, Outlook and Gmail show Accept/Decline buttons. A reply goes to the no-reply mailbox and LibreBooking never sees it. The email template could tell people to use the links instead.
| $event->add('ORGANIZER', 'mailto:' . $res->OrganizerEmail, $organizerParams); | ||
| } | ||
| $event->add('STATUS', $method === IcsMethod::CANCEL ? 'CANCELLED' : ($res->IsPending ? 'TENTATIVE' : 'CONFIRMED')); | ||
| $event->add('SUMMARY', $res->Summary); |
| } | ||
|
|
||
| $attendeeEmail = $res->AttendeeEmail ?? $res->OrganizerEmail; | ||
| if ($method !== IcsMethod::PUBLISH && !empty($res->OrganizerSentBy) && !empty($attendeeEmail)) { |
Summary
Reservation emails always attached an ICS with
METHOD:REQUEST, soOutlook offered to add the event again on cancel. Clients disagree on
what they accept, and a
REQUESTneeds anATTENDEE(RFC 5546).Changes
IcsMethod. Deletion emails sendCANCEL, all other emails sendREQUEST. Feeds and exports keepPUBLISH.recipient (owner, invitee, guest or share address), with
PARTSTAT=NEEDS-ACTIONandRSVP=FALSE.BookedBy()falls back to the owner's sessioninstead of failing.
Testing
attendee, status on cancel).
and Thunderbird. Not retested in Outlook with the final layout.
Closes: #1701
🤖 Generated with Claude Code