chore!: Remove deprecated hideRoomsWithNoActivity param from admin/engagement-dashboard/channels/list
#36886
Conversation
…/engagement-dashboard/channels/list`
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 2f3ec5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 38 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #36886 +/- ##
=================================================
- Coverage 66.37% 65.52% -0.85%
=================================================
Files 3312 3216 -96
Lines 111671 110191 -1480
Branches 20940 20592 -348
=================================================
- Hits 74121 72203 -1918
- Misses 34872 35407 +535
+ Partials 2678 2581 -97
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
WalkthroughMajor-version bump of @rocket.chat/meteor; removal of the hideRoomsWithNoActivity parameter from the engagement-dashboard channels list API, client usage, and tests. Server-side logic now exclusively uses Analytics aggregation in findChannelsWithNumberOfMessages, dropping Rooms-based logic and the auxiliary function. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Admin UI (Client)
participant API as REST API (/v1/engagement-dashboard/channels/list)
participant SVC as Channels Service
participant AN as Analytics
rect rgba(227,242,253,0.5)
note over UI,API: New flow (no hideRoomsWithNoActivity)
UI->>API: GET list?start&end&offset&count
API->>SVC: findChannelsWithNumberOfMessages({ start, end, options })
SVC->>AN: findRoomsByTypesWithNumberOfMessagesBetweenDate(start, end, options)
AN-->>SVC: Aggregation result ({ channels, total } or empty)
SVC-->>API: { channels, total }
API-->>UI: 200 OK { channels, total, offset, count }
end
sequenceDiagram
autonumber
participant UI as Admin UI (Client)
participant API as REST API (old)
participant SVC as Channels Service (old)
participant RM as Rooms (old)
participant AN as Analytics (old)
rect rgba(253,236,200,0.4)
note over UI,API: Old flow (deprecated flag)
UI->>API: GET list?...&hideRoomsWithNoActivity=true|false
API->>SVC: findChannelsWithNumberOfMessages({ start, end, hideRoomsWithNoActivity, options })
alt hideRoomsWithNoActivity
SVC->>AN: Analytics aggregation
else
SVC->>RM: Rooms-based query path
end
SVC-->>API: { channels, total }
API-->>UI: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (1)
151-170: Contradiction with next test: empty rooms excluded here but included later.This test asserts empty rooms are NOT returned, while the very next test expects the same empty room to be present with zero counts. Pick one behavior (default seems “hide empties”) and drop/adjust the other test to avoid flakiness.
Apply one of:
- Keep this test, remove the next “empty room counts 0” test.
- Or invert this assertion if product decision is to include empties by default.
apps/meteor/ee/server/api/engagementDashboard/channels.ts (1)
54-68: Validate that start <= end (prevent negative “last week” windows).Currently only ISO format is validated. If start > end,
diffBetweenDaysInclusivemay produce a negative span, breaking last-week bounds silently.Apply:
- const { start, end } = this.queryParams; - const { offset, count } = await getPaginationItems(this.queryParams); - - const { channels, total } = await findChannelsWithNumberOfMessages({ - start: mapDateForAPI(start), - end: mapDateForAPI(end), + const { start, end } = this.queryParams; + const { offset, count } = await getPaginationItems(this.queryParams); + + const startDate = mapDateForAPI(start); + const endDate = mapDateForAPI(end); + if (startDate.getTime() > endDate.getTime()) { + return API.v1.failure('start must be less than or equal to end'); + } + + const { channels, total } = await findChannelsWithNumberOfMessages({ + start: startDate, + end: endDate, options: { offset, count }, });apps/meteor/ee/server/lib/engagementDashboard/channels.ts (1)
35-38: Off-by-one: last-week window is 1 day too long — fix startOfLastWeekdiffBetweenDaysInclusive(end, start) = moment(end).diff(start, 'days') + 1 (daysBetweenDates). With endOfLastWeek = moment(start).subtract(1, 'days') and startOfLastWeek = moment(endOfLastWeek).subtract(daysBetweenDates, 'days'), the derived last-week span becomes daysBetweenDates + 1. Make both windows equal-length by computing startOfLastWeek as either:
- moment(start).subtract(daysBetweenDates, 'days').toDate()
or- moment(endOfLastWeek).subtract(daysBetweenDates - 1, 'days').toDate()
Location: apps/meteor/ee/server/lib/engagementDashboard/channels.ts (around lines 35–38).
🧹 Nitpick comments (5)
.changeset/warm-boats-care.md (1)
5-5: Add explicit BREAKING section and migration note.Call out the removed query param in a “BREAKING CHANGE” block with brief migration guidance (clients must stop sending
hideRoomsWithNoActivity; server ignores unknown keys but typings will fail). Consider linking to admin REST docs and release notes.Apply:
Removes the deprecated `hideRoomsWithNoActivity` param from `admin/engagement-dashboard/channels/list` + +BREAKING CHANGE: +- The endpoint `admin/engagement-dashboard/channels/list` no longer accepts the `hideRoomsWithNoActivity` query parameter. +- Clients must remove this parameter; default behavior hides rooms with no activity.apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (4)
172-205: Remove or gate the “empty room returns with zero counts” test.Given the parameter was removed and default behavior is to hide rooms with no activity, this test likely no longer applies.
Apply:
- it('should correctly count messages in an empty room', async () => { - ... - });
207-241: Update test title: stale reference to a removed param.Drop “when the param is provided” from the description; the endpoint no longer accepts that flag.
Apply:
- it('should correctly count messages diff compared to last week when the param is provided and there are messages in a room', async () => { + it('should correctly count messages diff compared to last week when there are messages in a room', async () => {
276-309: Duplicate test block; remove one to reduce noise and runtime.This test is duplicated at Lines 310–342 with identical assertions.
Apply:
- it('should correctly count messages from last week and diff when moving to the next week', async () => { - ... - });
119-119: Fix typo in test name.“succesfuly” → “successfully”.
- it('should succesfuly return results', async () => { + it('should successfully return results', async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/warm-boats-care.md(1 hunks)apps/meteor/client/views/admin/engagementDashboard/channels/useChannelsList.ts(0 hunks)apps/meteor/ee/server/api/engagementDashboard/channels.ts(2 hunks)apps/meteor/ee/server/lib/engagementDashboard/channels.ts(1 hunks)apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/views/admin/engagementDashboard/channels/useChannelsList.ts
🔇 Additional comments (2)
apps/meteor/ee/server/lib/engagementDashboard/channels.ts (1)
40-48: Happy path LGTM; single Analytics codepath simplifies behavior.Switching to Analytics aggregation and returning empty arrays when no results is clean and predictable.
apps/meteor/ee/server/api/engagementDashboard/channels.ts (1)
13-31: Typings update looks correct — confirm no callers still passhideRoomsWithNoActivityAutomated search failed: ripgrep returned "No files were searched"; verification couldn't be completed.
Run locally and confirm no matches:
#!/bin/bash rg -nC2 --hidden --no-ignore -S 'hideRoomsWithNoActivity' || true rg -nC2 --hidden --no-ignore -S 'engagement-dashboard/channels/list' || trueChanged: apps/meteor/ee/server/api/engagementDashboard/channels.ts (lines 13-31)
|
Done here #36951 |
Proposed changes (including videos or screenshots)
Removes the deprecated
hideRoomsWithNoActivityparam fromadmin/engagement-dashboard/channels/listendpointIssue(s)
ARCH-1757
Steps to test or reproduce
Further comments
Summary by CodeRabbit