feat(calendar): show a descriptive name in ICS subscription feeds

[lucs7]

Add per-subscription calendar name (schedule name, resource name, resource group name, or the current user's own calendar) and pass it to CalendarExportDisplay::Render() so subscribed calendars ics files get a readable NAME/X-WR-CALNAME instead of a generic one in calendar clients.

Assisted-by: Claude:claude-sonnet-5

[Copilot]

🟡 Not ready to approve

The new resource-group calendar naming introduces an avoidable extra repository lookup per request (double-loading the same resource group) which should be addressed to prevent a performance regression.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Adds a per-subscription calendar display name to ICS subscription feeds so clients show a meaningful calendar title (e.g., schedule/resource/resource-group name or “My Calendar”) instead of a generic one.

Changes:

• Derive calendarName during subscription feed generation (schedule/resource/user/resource-group) and pass it through the page layer.
• Extend the ICS subscription rendering path to pass the calendar name into CalendarExportDisplay::Render() so NAME / X-WR-CALNAME can be set.
• Add/expand unit tests covering calendar-name selection for schedule/resource/user/resource-group subscriptions.

File summaries

File	Description
tests/Presenters/CalendarSubscriptionPresenterTest.php	Adds assertions that CalendarName is set appropriately for different subscription selectors.
tests/Application/Schedule/CalendarSubscriptionServiceTest.php	Adds coverage for fetching resource group names by public id (including not-found).
Presenters/CalendarSubscriptionPresenter.php	Computes a subscription-specific calendar name and sets it on the page.
Pages/Export/SubscriptionPage.php	Stores calendarName on the base subscription page and exposes a setter.
Pages/Export/ICalendarSubscriptionPage.php	Extends the subscription page contract with SetCalendarName().
Pages/Export/CalendarSubscriptionPage.php	Passes the stored calendar name to CalendarExportDisplay::Render().
lib/Application/Schedule/CalendarSubscriptionService.php	Adds GetResourceGroupName() to support naming for group subscriptions.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

empty() is used for the resource-group name check, which can incorrectly discard valid names like "0" and prevent the calendar name from being set.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

Presenters/CalendarSubscriptionPresenter.php:116

• empty($resourceGroupName) treats the string '0' as empty, which would incorrectly skip setting the calendar name for a valid group named "0". Use the same null/empty-string check used later for $calendarName to avoid PHP's empty() pitfalls.

        if (!empty($resourceGroupId)) {
            $resourceIds = $this->subscriptionService->GetResourcesInGroup($resourceGroupId);
            $resourceGroupName = $this->subscriptionService->GetResourceGroupName($resourceGroupId);
            if (!empty($resourceGroupName)) {
                $calendarName = $resourceGroupName;
            }

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

A resource-group filter can be treated as “no filter” when the resolved resource list is empty (e.g., schedule+group where the group has no resources), producing an incorrect non-empty feed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

Presenters/CalendarSubscriptionPresenter.php:121

• When a subscription includes both a schedule and a resource group, and the resource group resolves to an empty resource list, the later empty($resourceIds) || in_array(...) check treats that as “no group filter” and will include all schedule reservations instead of returning an empty feed. Track whether a resource-group filter is active separately from whether the resolved list is empty, and short-circuit reservation loading when the filter is active but empty.

        }

        if ($calendarName !== null && $calendarName !== '') {
            $this->page->SetCalendarName($calendarName);
        }

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[lucs7]

the resource resource-group filter was present in the code and is not touched by the PR. I created a separate bug report for later fixing

[Copilot]

🟢 Ready to approve

The change is narrowly scoped, covered by targeted unit tests, and the new group-name lookup is implemented with caching to avoid redundant loads.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 7/7 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[JohnVillalovos]

Can this use 'type-hints' instead of a docblock? Trying to use type-hints where possible, though not always possible.

[JohnVillalovos]

Can this use 'type-hints' instead of a docblock? Trying to use type-hints where possible, though not always possible.

[JohnVillalovos]

Please add type-hints

[JohnVillalovos]

Please add type-hints

[JohnVillalovos]

Claude Code review: f0fa2a4a6 — feat(calendar): show a descriptive name in ICS subscription feeds

What's good

• Correct layering: presenter decides the name, page holds it, display renders
  it. No new coupling introduced.
• LoadResourceGroup() + $groupCache avoids a second
  LoadResourceGroupByPublicId round trip, and
  testGetResourceGroupNameReusesGroupLoadedByGetResourcesInGroup actually
  asserts the repository is hit exactly once. Good test.
• The group-name null/empty guard correctly avoids clobbering an
  already-derived schedule name, with a dedicated test
  (testGetsResourceGroupReservationsKeepsScheduleCalendarNameWhenGroupNameIsNull).
• No new information disclosure. The feed already exposes reservation detail
  behind the subscription key, and the personal feed uses a generic label rather
  than the user's name.
• sabre/vobject escapes and folds the property value, so an admin-authored
  resource name containing commas, semicolons or newlines can't corrupt the ICS.

Findings

1. Precedence makes personal filtered feeds indistinguishable

Presenters/CalendarSubscriptionPresenter.php:103-109

The !empty($userId) block runs after the schedule and resource blocks, so it
overwrites their names. That matters because
Presenters/Calendar/PersonalCalendarPresenter.php:57 calls
ForUser($userSession->UserId, $resourceId, $scheduleId), and
CalendarSubscriptionUrl::__construct() puts all three (uid, sid, rid)
into the generated URL.

Result: a user who subscribes to "my calendar filtered to Room A" and "my
calendar filtered to Room B" gets two feeds both named My Calendar — which is
the exact problem this commit sets out to solve.

A composed name (My Calendar – Room A) or letting the resource/schedule name
win when present would fix it. Not a merge blocker, but it's the one case where
the feature under-delivers, and it's untested — no test covers uid + rid
together.

2. No test asserts the name reaches the ICS output

Nothing under tests/ asserts X-WR-CALNAME for an export or subscription;
the only occurrence is tests/Presenters/Admin/ImportICalPresenterTest.php:108,
which is a parsing fixture. The wiring is tested at the presenter/page boundary,
but the payload never is.

tests/Presenters/CalendarExportPresenterTest.php already instantiates
CalendarExportDisplay and inspects serialized output in five places, so a
one-line assertion there would close the gap cheaply.

3. Minor: naming/consistency in the service

lib/Application/Schedule/CalendarSubscriptionService.php:200

Sibling methods return domain objects (GetResource, GetSchedule, GetUser).
A GetResourceGroup($publicId): ?ResourceGroup would fit the existing shape
better and let callers take what they need, rather than adding a name-specific
accessor to the public interface. Cosmetic; wouldn't block on it.

4. Docblocks used where native type declarations would work

The commit adds @param / @return docblocks on brand-new declarations instead
of native types. CLAUDE.md asks for type declarations on new functions and
methods, and the author already used a native type for the backing property
(protected ?string $calendarName = null, Pages/Export/SubscriptionPage.php:27),
so the two styles are inconsistent within the same commit.

Native types cannot be adopted one line at a time here. PHP rejects partial
adoption in both directions — verified on PHP 8.4.17:

• Interface param untyped, implementation narrowing to ?string →
  Fatal error: Declaration of CA::f(?string $x): void must be compatible with IA::f($x)
• Interface declaring : ?string, implementation omitting it →
  Fatal error: Declaration of CB::g($x) must be compatible with IB::g($x): ?string

So the interface and every implementer must be typed in the same change. The
table below therefore lists the interface and its implementation as separate
rows carrying the same signature — PHP has no type inheritance, so each
declaration stands on its own.

The two halves are not equally optional. The return type is mandatory on the
implementation (second bullet above). The parameter type is not — an untyped
param is mixed, which is wider than string, so contravariance lets the class
load. But PHP checks arguments against the signature of the function actually
invoked, not the interface it satisfies, so a half-typed pair silently stops
enforcing the contract, including for calls made through the interface type:

interface I3 { public function f(string $x): ?string; }
class C3 implements I3 { public function f($x): ?string { var_dump($x); return null; } }

$c->f(123);                              // int(123)  - no error
(function (I3 $i) { $i->f(456); })($c);  // int(456)  - no error, called via the interface

Typed on both sides, the same call is rejected with
TypeError: C4::f(): Argument #1 ($x) must be of type string, array given.
That makes the half-typed form the quietly-broken case rather than a build
failure, which is the reason to insist on both:

Interface	Implementation	Result
f(string $x): ?string	f(string $x): ?string	Correct — enforced at runtime
f(string $x): ?string	f($x): ?string	Loads, but param contract is unenforced
f(string $x): ?string	f(string $x)	Fatal error at declaration

For this commit that is six declarations across four files:

Site	Current	Typed equivalent
Pages/Export/ICalendarSubscriptionPage.php:20-23	@param string $calendarName + untyped	SetCalendarName(?string $calendarName): void
Pages/Export/SubscriptionPage.php:49	SetCalendarName($calendarName): void	SetCalendarName(?string $calendarName): void
tests/Presenters/CalendarSubscriptionPresenterTest.php:302	SetCalendarName($calendarName) (the fake)	SetCalendarName(?string $calendarName): void
lib/Application/Schedule/CalendarSubscriptionService.php:51-55	@param string / @return null|string + untyped	GetResourceGroupName(string $publicResourceGroupId): ?string
lib/Application/Schedule/CalendarSubscriptionService.php:200	untyped	GetResourceGroupName(string $publicResourceGroupId): ?string
lib/Application/Schedule/CalendarSubscriptionService.php:207	untyped (private)	LoadResourceGroup(string $publicResourceGroupId): ?ResourceGroup

This stays inside the commit's own blast radius. Every one of those six is a
+ line introduced by this commit, in a file it already modifies — no
pre-existing declaration has to change. Confirmed by git show HEAD and by
grepping the tree: the only three SetCalendarName declarations that exist are
the interface, SubscriptionPage, and the test fake. AtomSubscriptionPage and
CalendarSubscriptionPage inherit the concrete method without redeclaring it,
and CalendarSubscriptionValidatorTest uses createMock, which generates
type-compatible implementations at runtime.

string rather than ?string is correct for $publicResourceGroupId: the sole
caller guards with !empty($resourceGroupId) before calling
(Presenters/CalendarSubscriptionPresenter.php:111-113), and querystring values
are strings. ?ResourceGroup is safe for the private helper because
LoadResourceGroupByPublicId() returns a ResourceGroup or null
(Domain/Access/ResourceRepository.php:590-603).

Where the docblocks should stay. The neighbouring untyped members —
SetReservations(), GetResourcesInGroup(), GetResource(), GetSchedule(),
GetUser() — are pre-existing declarations with implementers and callers
outside this commit's diff (e.g. CalendarExportPage::SetReservations()).
Typing those is a separate refactor and correctly out of scope here; the
author's "match local style" reasoning holds for them, just not for the six new
declarations above.

Note this interacts with finding #5: typing ICalendarSubscriptionPage widens
the ABI break for out-of-tree implementers beyond the method addition alone.

5. Note, not a defect: interface backwards compatibility

Adding SetCalendarName() to ICalendarSubscriptionPage and
GetResourceGroupName() to ICalendarSubscriptionService breaks any
out-of-tree implementer. Nothing in-tree breaks, and this is consistent with how
the project has evolved these interfaces. Worth a mention only if the
maintainers care about third-party page/service implementations.

6. Unrelated to the commit, but in the working tree

tpl/Export/ical.tpl is untracked and dead after the sabre/vobject migration.
Make sure it doesn't get swept into a commit.

Verdict

Yes, merge it — but raise finding #1 on the PR first.

The change is well-scoped, unit-tested, and passes every CI gate locally. The
precedence question deserves a maintainer decision before merge (a composed name
is a small follow-up either way), and adding one X-WR-CALNAME assertion to
CalendarExportPresenterTest would close the only real coverage gap.

[JohnVillalovos]

Codex Review of f0fa2a4

One non-blocking maintainability finding.

Finding

Low: Use native types for the newly introduced APIs

The new methods rely on PHPDoc where native PHP type declarations can express the contracts directly:

• ICalendarSubscriptionPage::SetCalendarName() documents a string parameter but has no parameter or return type.
• CalendarSubscriptionService::GetResourceGroupName() documents null|string but has no native parameter or return type.
• The new private LoadResourceGroup() helper is also untyped.

With PHP 8.2 as the minimum supported version and the repository guidance to use type hints where possible, these should be declared as:

public function SetCalendarName(string $calendarName): void;
public function GetResourceGroupName(string $publicResourceGroupId): ?string;
private function LoadResourceGroup(string $publicResourceGroupId): ?ResourceGroup;

The concrete SubscriptionPage implementation and the test fake should use the same SetCalendarName() signature. Native declarations make these new contracts enforceable and avoid PHPDoc drifting from the implementation.

Recommendation

Merge upstream after addressing the native-type cleanup above. It is small and does not require a design change.

Review summary

• Correctly selects schedule, resource, user, and resource-group calendar names.
• Resource-group lookup caching avoids duplicate repository calls.
• Calendar names reach both NAME and X-WR-CALNAME in the serialized ICS feed.
• The commit message follows Conventional Commits, has a 65-character header, and includes the required Assisted-by trailer.

Additional non-blocking note

There is no automated page-level test covering the final forwarding call in Pages/Export/CalendarSubscriptionPage.php. The implementation is straightforward, and the direct serialization smoke test confirmed the expected output, so this does not block merging.