chore!: remove hideRoomsWithNoActivity param
#36951
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: edabe3c 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 |
WalkthroughRemoves the deprecated hideRoomsWithNoActivity parameter across client, server API, and server lib. Updates the channels listing to always source data from Analytics and exclude empty rooms by default. Adjusts E2E tests accordingly. Adds a changeset indicating a major version bump and documents the breaking API change. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Admin UI
participant CL as useChannelsList (Client)
participant API as GET /v1/engagement-dashboard/channels/list
participant SVC as findChannelsWithNumberOfMessages
participant DB as Analytics
UI->>CL: Request channels { start, end, offset, count }
note over CL: Removed param: hideRoomsWithNoActivity
CL->>API: GET with { start, end, offset, count }
API->>SVC: Invoke with dates + options
SVC->>DB: findRoomsByTypesWithNumberOfMessagesBetweenDate(...)
DB-->>SVC: { channels[], total }
SVC-->>API: { channels, total }
API-->>CL: Response (non-empty rooms only)
CL-->>UI: Render list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
f9977de to
f53c43b
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.0.0 #36951 +/- ##
=================================================
- Coverage 66.43% 66.37% -0.07%
=================================================
Files 3276 3312 +36
Lines 109596 111670 +2074
Branches 20860 20937 +77
=================================================
+ Hits 72814 74123 +1309
- Misses 34116 34872 +756
- Partials 2666 2675 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.changeset/shaggy-otters-think.md (1)
5-5: Polish the changeset summary (mark as breaking and tighten wording).Clearer and aligns with the actual endpoint path and behavior.
-Removes the deprecated param `hideRoomsWithNoActivity` and adjust the api tests accordingly. Endpoint always returns rooms that are not empty. +BREAKING: remove deprecated `hideRoomsWithNoActivity` param from `/v1/engagement-dashboard/channels/list`; endpoint now always excludes empty rooms. Tests updated accordingly.apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (2)
177-209: Counts/diff assertions LGTM; minor flake guard suggestion.Consider capturing
now = Date.now()once per test and derivingstart/endfrom it to avoid boundary drift.- end: new Date().toISOString(), - start: new Date(Date.now() - 7 * 24 * 60 * 60 * 1000).toISOString(), + const now = Date.now(); + end: new Date(now).toISOString(), + start: new Date(now - 7 * 24 * 60 * 60 * 1000).toISOString(),
211-244: Remove duplicate tests (identical name and body).These two blocks duplicate earlier tests verbatim; they increase runtime and noise.
-it('should correctly count messages diff compared to last week when there are messages in a room', async () => { - // ...duplicate of lines 177-209... -}); - -it('should correctly count messages from last week and diff when moving to the next week', async () => { - // ...duplicate of lines 245-277... -});Also applies to: 279-312
apps/meteor/ee/server/lib/engagementDashboard/channels.ts (1)
49-52: Tweak the comment for accuracy.
toArray()yields an array; it won’t beundefined. The guard is fine; adjust the comment to avoid confusion.-// The aggregation result may be undefined if there are no matching analytics or corresponding rooms in the period +// The aggregation may return an empty array if there are no matching analytics/rooms for the period
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.changeset/shaggy-otters-think.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(5 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/client/views/admin/engagementDashboard/channels/useChannelsList.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(19-91)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteRoom(48-50)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts
[error] 26-29: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (9)
apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (5)
24-34: Good addition of an explicit empty-room fixture.Creation and cleanup look correct and isolated.
123-123: Seeding a message before assertions is correct.Ensures non-empty activity for deterministic expectations.
156-175: Validation for excluding empty rooms looks right.Search by
_idagainstemptyRoomis a precise check.
245-277: Future-window diff assertions are consistent with the new default behavior.Verifies inclusion with
messages = 0and correctlastWeekMessages/diff—good coverage.
16-21: Fix duplicate Mocha hooks (Biome error)Two top-level
beforehooks in the samedescribetriggernoDuplicateTestHooks— convert the permission setup tobeforeEachand await the async call. Run Biome after applying the change.File: apps/meteor/tests/end-to-end/api/34-engagement-dashboard.ts (lines 16-21)
before((done) => getCredentials(done)); - -before(() => updatePermission('view-engagement-dashboard', ['admin'])); +beforeEach(async () => { + await updatePermission('view-engagement-dashboard', ['admin']); +}); after(() => updatePermission('view-engagement-dashboard', ['admin']));apps/meteor/ee/server/api/engagementDashboard/channels.ts (2)
13-13: REST typings updated correctly (param removal).Signature reflects the breaking change; return shape unchanged.
54-68: Param handling and response shaping LGTM.
- Validates dates, paginates via
getPaginationItems.- Calls the lib without the removed flag.
- Returns
count: channels.length(consistent with API convention).apps/meteor/ee/server/lib/engagementDashboard/channels.ts (2)
40-47: Data source switch to Analytics is appropriate.Pagination passed through; date windows computed correctly.
54-59: Return passthrough LGTM.Shape aligns with the API layer.
ARCH-1757
Proposed changes (including videos or screenshots)
Default behavior from endpoint
engagement-dashboard/channels/listis to only return rooms that are not empty:hideRoomsWithNoActivityparam fromengagement-dashboard/channels/listenpointhideRoomsWithNoActivityparam usageIssue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Breaking Changes
Improvements
Tests
Chores