test: custom user status API tests with authentication and permission#39330
test: custom user status API tests with authentication and permission#39330dionisio-bot[bot] merged 1 commit intodevelopfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe custom-user-status end-to-end test suite was expanded to include comprehensive testing for create, update, and delete operations with authentication, authorization, validation, and lifecycle scenarios. Global setup/teardown logic was added for managing unauthorized user accounts and permissions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/custom-user-status.ts (2)
23-30: Consider consolidating duplicatebeforehooks.While Mocha allows multiple
beforehooks, consolidating them improves readability and maintains a single setup flow. The static analyzer flagged this as a duplicate hook.♻️ Proposed consolidation
- before((done) => { - getCredentials(done); - }); - - before(async () => { - unauthorizedUser = await createUser(); - unauthorizedUserCredentials = await login(unauthorizedUser.username, password); - }); + before(async function () { + await new Promise<void>((resolve) => getCredentials(resolve)); + unauthorizedUser = await createUser(); + unauthorizedUserCredentials = await login(unauthorizedUser.username, password); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-user-status.ts` around lines 23 - 30, Consolidate the two before hooks into a single before hook that first runs getCredentials and then creates/logs in the unauthorized user; update the before to be an async function that awaits getCredentials (wrap its callback form in a Promise if needed) and then awaits createUser() and login() to set unauthorizedUser and unauthorizedUserCredentials. Target the getCredentials call and the unauthorizedUser/unauthorizedUserCredentials assignments so the setup runs in one sequential flow.
231-236: Redundant permission restoration inafterhook.The
updatePermission('manage-user-status', ['admin'])call on line 235 is redundant sinceafterEachat line 228 already restores permissions after each test. Theafterhook runs after the lastafterEach, so permissions will already be restored.♻️ Proposed simplification
after(async () => { if (customUserStatusId) { await deleteCustomUserStatus(customUserStatusId); } - await updatePermission('manage-user-status', ['admin']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/end-to-end/api/custom-user-status.ts` around lines 231 - 236, Remove the redundant permission restoration call from the after hook: the afterEach already calls updatePermission('manage-user-status', ['admin']) for every test, so in the after() block (which currently contains deleteCustomUserStatus(customUserStatusId) and updatePermission('manage-user-status', ['admin'])), keep the deleteCustomUserStatus(customUserStatusId) cleanup but delete the updatePermission('manage-user-status', ['admin']) line to avoid duplicate restoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/custom-user-status.ts`:
- Around line 23-30: Consolidate the two before hooks into a single before hook
that first runs getCredentials and then creates/logs in the unauthorized user;
update the before to be an async function that awaits getCredentials (wrap its
callback form in a Promise if needed) and then awaits createUser() and login()
to set unauthorizedUser and unauthorizedUserCredentials. Target the
getCredentials call and the unauthorizedUser/unauthorizedUserCredentials
assignments so the setup runs in one sequential flow.
- Around line 231-236: Remove the redundant permission restoration call from the
after hook: the afterEach already calls updatePermission('manage-user-status',
['admin']) for every test, so in the after() block (which currently contains
deleteCustomUserStatus(customUserStatusId) and
updatePermission('manage-user-status', ['admin'])), keep the
deleteCustomUserStatus(customUserStatusId) cleanup but delete the
updatePermission('manage-user-status', ['admin']) line to avoid duplicate
restoration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6f5aadb-b22f-451a-914c-529c3fff907a
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
🧠 Learnings (14)
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/custom-user-status.ts
🪛 Biome (2.4.4)
apps/meteor/tests/end-to-end/api/custom-user-status.ts
[error] 27-30: Duplicate before hook found.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (5)
apps/meteor/tests/end-to-end/api/custom-user-status.ts (5)
1-8: LGTM!The imports are well-organized and the
Responsetype import improves type safety for the test assertions.
10-13: LGTM!The optional
statusTypeparameter correctly aligns with the API endpoint signature and is properly forwarded in the request payload.
131-216: LGTM!The create endpoint tests provide comprehensive coverage including authentication, authorization, success path with
statusType, duplicate name validation, and invalidstatusTypehandling. TheafterEachhook properly restores permissions and cleans up created statuses, ensuring test isolation.
276-326: LGTM!The update success test properly validates the response and tracks the updated name. The duplicate name and invalid
statusTypetests correctly verify error handling, and the inline cleanup of the secondary status maintains test isolation.
328-410: LGTM!The delete endpoint tests use an appropriate
beforeEach/afterEachpattern that creates a fresh status for each test. The success test correctly setscustomUserStatusId = ''to prevent double-deletion inafterEach. This ensures proper test isolation while covering all critical paths: authentication, authorization, missing parameter, non-existent status, and successful deletion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39330 +/- ##
===========================================
- Coverage 70.89% 70.87% -0.03%
===========================================
Files 3207 3207
Lines 113334 113334
Branches 20538 20531 -7
===========================================
- Hits 80349 80322 -27
- Misses 30940 30959 +19
- Partials 2045 2053 +8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes
Adds new test cases to improve coverage for the
custom-user-statusAPI endpoints.New Test Cases
Create endpoint (
custom-user-status.create):statusTypevalidationUpdate endpoint (
custom-user-status.update):statusTypevalidationDelete endpoint (
custom-user-status.delete):Issue(s)
QA-113
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
This pull request contains no user-facing changes. It extends internal test coverage for custom user status functionality, including authentication, authorization, and validation test scenarios.