feat(calendar): show a descriptive name in ICS subscription feeds - #1624
feat(calendar): show a descriptive name in ICS subscription feeds#1624lucs7 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟡 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
calendarNameduring 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()soNAME/X-WR-CALNAMEcan 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.
9625594 to
3893065
Compare
There was a problem hiding this comment.
🟡 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$calendarNameto avoid PHP'sempty()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.
3893065 to
ace8ed2
Compare
There was a problem hiding this comment.
🟡 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.
|
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 |
There was a problem hiding this comment.
🟢 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.
28b2798 to
5593996
Compare
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
5593996 to
f0fa2a4
Compare
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