User last activity #3286
User last activity #3286
Conversation
… instead of `UserAccountView`
WalkthroughRenames admin user models/assemblers to administration-specific types, adds lastActivity through repository/service/controller/DTOs and frontend schemas, removes a public LLM prompt endpoint and related schemas, refactors admin UI list rendering, and updates E2E tests to assert lastActivity. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as AdministrationController
participant Service as UserAccountService
participant Repo as UserAccountRepository
participant Assembler as UserAccountAdministrationModelAssembler
Client->>Controller: GET /v2/administration/users
Controller->>Service: findAllWithDisabledPaged(pageable, search)
Service->>Repo: findAllWithDisabledPaged(pageable, search)
Repo-->>Service: Page<UserAccountAdministrationView> (includes lastActivity)
Service-->>Controller: Page<UserAccountAdministrationView>
Controller->>Assembler: toModel(view) for each item
Assembler-->>Controller: UserAccountAdministrationModel (avatar links, lastActivity)
Controller-->>Client: PagedModel<UserAccountAdministrationModel>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-07T14:36:39.331ZApplied to files:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
DEVELOPMENT.md (3)
80-86: New "API schema changes" section is helpful but consider clarifying the workflow.The new documentation is clear and useful for developers who modify API schemas. Consider whether you want to add a note about regenerating the schema after significant API model changes (e.g., adding new fields like
lastActivityto administration models). This would help developers avoid missing the schema sync step when adding new features.
82-82: Minor wording suggestion for clarity.The phrase "invoke the webapp schema script" reads slightly awkwardly. Consider rephrasing to "run the webapp schema script" for consistency with other command instructions in the document.
115-115: Use more formal wording in the instructions.Replace "To fix Prettier issues" with "To resolve Prettier issues" for more formal and consistent documentation tone.
webapp/src/views/administration/AdministrationUsers.tsx (1)
82-83: MUI color prop: use text.secondary (v5), not textSecondary.Update Typography color to the v5 token to avoid deprecation warnings.
-<Typography variant="body2" color="textSecondary"> +<Typography variant="body2" color="text.secondary">backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountAdministrationModel.kt (1)
7-7: Prefer Instant/OffsetDateTime over Date in public API.Using java.time types is clearer and timezone‑safe; Jackson maps them well. Consider switching lastActivity to Instant (or OffsetDateTime).
-import java.util.Date +import java.time.Instant @@ - val lastActivity: Date?, + val lastActivity: Instant?,Changing the type will cascade to the assembler and frontend schema; confirm compatibility before applying.
Also applies to: 20-21
backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountAdministrationView.kt (1)
7-18: Be aware ofByteArrayin data class.The
totpKeyproperty usesByteArray?(line 18), which relies on reference equality rather than structural equality in Kotlin data classes. This can lead to unexpected behavior withequals(),hashCode(), andcopy()if instances are compared or used in collections. Since this is a view/DTO, it's likely acceptable, but consider using a different type (e.g.,List<Byte>or a wrapper) if equality semantics become important.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
DEVELOPMENT.md(12 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationController.kt(4 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountAdministrationModel.kt(2 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountAdministrationModelAssembler.kt(1 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModelAssembler.kt(0 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountAdministrationView.kt(1 hunks)backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt(2 hunks)webapp/src/service/apiSchema.generated.ts(3 hunks)webapp/src/views/administration/AdministrationUsers.tsx(3 hunks)
💤 Files with no reviewable changes (1)
- backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModelAssembler.kt
🧰 Additional context used
🧬 Code graph analysis (1)
webapp/src/service/apiSchema.generated.ts (1)
webapp/src/service/billingApiSchema.generated.ts (1)
components(218-1508)
🪛 LanguageTool
DEVELOPMENT.md
[grammar] ~82-~82: Ensure spelling is correct
Context: ... to have backend running and invoke the webapp schema script to update frontend: ```s...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~115-~115: Consider using a different verb for a more formal wording.
Context: ...ins for VS Code, Idea, or WebStorm. To fix Prettier issues and check everything is...
(FIX_RESOLVE)
🔇 Additional comments (10)
webapp/src/service/apiSchema.generated.ts (3)
6243-6257: No issues found —lastActivityis already correctly handled.The codebase already implements the safety pattern requested: it checks for undefined values (line 83 in AdministrationUsers.tsx), falls back to a localized translation for missing data, and correctly parses the ISO date string via
new Date(). No numeric assumptions exist. The type is correctly defined as optional string with date-time format.
6948-6949: No action required - changes are already correctly implemented.The single consumer of the
getUsersendpoint (AdministrationUsers.tsxline 36) already correctly handles thePagedModelUserAccountAdministrationModelresponse type and properly reads thelastActivityfield fromUserAccountAdministrationModel(lines 83, 86). The typing is automatically inferred through the genericuseApiQueryhook, and the schema is correctly defined. No updates are needed.Likely an incorrect or invalid review comment.
4068-4073: Embedded users type alignment looks correct.PagedModelUserAccountInProjectModel.embedded.users now points to UserAccountInProjectModel[]. Sanity-check affected UI/DTO mappers.
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationController.kt (2)
51-59: Wiring to new admin view/model is correct; auth tightened.Assembler/typed PagedResourcesAssembler updates align with the service/repository return type; adding @RequiresSuperAuthentication to list endpoints is appropriate.
Also applies to: 86-89
144-148: Minor doc polish looks good.Updated summary text improves clarity.
backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt (1)
570-575: Return type switch verified as complete and consistent.Verification confirms the Page return type is properly implemented across the entire call chain:
- Repository method returns Page
- Service method returns Page
- Controller correctly handles the new type
No lingering Page expectations found for this method; all call sites are properly aligned with the type change.
webapp/src/views/administration/AdministrationUsers.tsx (1)
86-90: Translation keys are missing from i18n files; refactor suggestion doesn't address root issue.The keys
administration_user_no_activityandadministration_user_last_activitydo not exist inwebapp/src/i18n/en.json(or other locale files). The current code at lines 83–88 already references these exact keys. The suggested refactor uses the same non-existent keys, merely wrapping them in an IIFE without solving the underlying problem.Additionally, the code already guards against missing
lastActivity(!u.lastActivitycheck), and the API schema confirmslastActivityis optional and provided as ISO 8601 when present. The suggested defensive parsing adds complexity without addressing the missing translations.Action: First, add the missing translation keys to all locale files in
webapp/src/i18n/. Then reassess whether additional defensive parsing is necessary (it likely is not, given the existing guard clause and optional API field).Likely an incorrect or invalid review comment.
backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountAdministrationModelAssembler.kt (2)
11-17: LGTM! Clean HATEOAS assembler structure.The component follows Spring HATEOAS best practices with proper type parameters and dependency injection.
18-33: No issues found. The role defaulting behavior is correct and intentional.The implementation defaults
nullrole toUserAccount.Role.USERconsistently. TheUserAccount.rolefield is defined as nullable (Role? = Role.USER) without database constraints, making null values theoretically possible. The defensive defaulting in the assembler aligns with the entity's own default value and is replicated across multiple assemblers (UserAccountAdministrationModelAssemblerandPrivateUserAccountModelAssembler), indicating this is an established pattern in the codebase. This approach is safe and appropriate.backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountAdministrationView.kt (1)
19-22: LGTM! Last activity field properly added.The new
lastActivityfield is correctly typed as nullableDate?, which aligns with the PR objective to track user last activity.
backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt
Show resolved
Hide resolved
JanCizmar
left a comment
JanCizmar
left a comment
There was a problem hiding this comment.
Thanks for working on this! 🙏
I have checked it fast and I have few points.
- I don't think that the feature request specified the need for frontend. However, it's nice to have.
- We need to add backend test
- we need to add e2e tests
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 (2)
webapp/src/websocket-client/WebsocketClient.ts (1)
85-93: Don’t drop subscriptions on disconnect — breaks resubscribe after reconnect.Clearing the array via removeSubscription makes resubscribe() a no-op on the next connect. Keep entries and only null out runtime handles.
Apply:
const onDisconnect = function () { connected = false; - subscriptions.forEach((s) => { - s.unsubscribe = undefined; - s.id = undefined; - removeSubscription(s); - }); + // Keep subscription records so onConnected() can resubscribe them. + subscriptions.forEach((s) => { + s.unsubscribe = undefined; + s.id = undefined; + }); options.onConnectionClose?.(); };webapp/src/views/administration/components/OptionsButton.tsx (1)
93-99: Use currentTarget for menu anchor; target can be the SVG path.Using event.target may anchor to a child element and break positioning.
-<IconButton - onClick={(e) => setAnchor(e.target as HTMLElement)} +<IconButton + onClick={(e) => setAnchor(e.currentTarget as HTMLElement)} data-cy="administration-user-menu" > <DotsVertical /> </IconButton>Optional a11y polish:
-<IconButton +<IconButton + aria-haspopup="menu" + aria-expanded={Boolean(anchor)} + aria-controls="administration-user-menu"And give Menu an id:
-<Menu +<Menu id="administration-user-menu"
🧹 Nitpick comments (1)
webapp/src/websocket-client/WebsocketClient.ts (1)
50-57: Typed parsing is effectively “any” — consider a minimal guard.We parse JSON and cast to Message, then downcast to any. A tiny improvement: type it as WebsocketEvent and narrow at call site to retain some structure without big changes.
-const parsed = JSON.parse(message.body) as Message; -subscription.callback(parsed as any); +const parsed = JSON.parse(message.body) as WebsocketEvent<unknown>; +subscription.callback(parsed as any); // retain current behavior while keeping event shape
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
webapp/src/component/security/UserMenu/MenuHeader.tsx(1 hunks)webapp/src/views/administration/AdministrationUsers.tsx(3 hunks)webapp/src/views/administration/components/OptionsButton.tsx(1 hunks)webapp/src/views/administration/components/RoleSelector.tsx(1 hunks)webapp/src/websocket-client/WebsocketClient.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/src/views/administration/AdministrationUsers.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
webapp/src/views/administration/components/RoleSelector.tsx (2)
webapp/src/service/apiSchema.generated.ts (1)
components(1072-6349)webapp/src/component/UserAccount.tsx (1)
User(10-15)
🔇 Additional comments (4)
webapp/src/component/security/UserMenu/MenuHeader.tsx (1)
6-7: LGTM — type swap aligns with admin model.Union still satisfies usages (id, name, avatar) in this component.
webapp/src/views/administration/components/OptionsButton.tsx (1)
12-12: LGTM — local alias updated to administration model.Matches administration endpoints used below.
webapp/src/views/administration/components/RoleSelector.tsx (1)
10-12: LGTM — roles and user types now sourced from administration model.The satisfies Role usage keeps literals aligned with the union.
If server roles ever change, update both the OpenAPI schema and these literals together.
webapp/src/websocket-client/WebsocketClient.ts (1)
193-200: Narrow actor type to prevent admin field exposure — no internal impact but improves API safety.Current code has no consumers accessing
actororactor.datafields. However, the exportedWebsocketEventtype exposesUserAccountAdministrationModelincluding admin-only fields (disabled, deleted, mfaEnabled, etc.). This is a design-level API exposure risk.The proposed refactor is safe and recommended:
+type ActorUser = Pick< + components['schemas']['UserAccountAdministrationModel'], + 'id' | 'username' | 'name' | 'avatar' +>; export type WebsocketEvent<Data> = { activityId: number; actor: { type: 'user'; - data: components['schemas']['UserAccountAdministrationModel']; + data: ActorUser; }; data: Data; };This change has zero runtime or internal compilation impact (verified: no code accesses
actor.databeyond these fields).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webapp/src/views/administration/AdministrationUsers.tsx (1)
83-105: Consider adding explicit spacing between primary and secondary text rows.The nested
Boxstructure creates a two-row layout, but relies on default Typography margins for spacing. While this likely works, adding explicit spacing (e.g.,gapon the outer Box ormarginTopon the secondary Box) would provide more predictable and maintainable layout control.Example refinement:
- <Box> + <Box sx={{ display: 'flex', flexDirection: 'column', gap: 0.5 }}> <Box> <Typography variant="body1">Alternatively, add margin to the secondary text box:
</Box> - <Box> + <Box sx={{ mt: 0.5 }}> <Typography
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
e2e/cypress/common/administration.ts(1 hunks)e2e/cypress/e2e/administration/base.cy.ts(6 hunks)e2e/cypress/e2e/administration/customerDebug.cy.ts(2 hunks)e2e/cypress/support/dataCyType.d.ts(1 hunks)webapp/src/views/administration/AdministrationUsers.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
e2e/cypress/common/administration.ts (1)
e2e/cypress/common/shared.ts (1)
gcy(28-29)
e2e/cypress/e2e/administration/customerDebug.cy.ts (2)
e2e/cypress/common/administration.ts (2)
visitAdministrationUsers(8-10)getUserListItem(12-16)e2e/cypress/common/shared.ts (1)
gcy(28-29)
e2e/cypress/e2e/administration/base.cy.ts (2)
e2e/cypress/common/administration.ts (3)
visitAdministrationOrganizations(4-6)visitAdministrationUsers(8-10)getUserListItem(12-16)e2e/cypress/common/apiCalls/common.ts (2)
forceDate(529-531)releaseForcedDate(533-535)
🔇 Additional comments (11)
e2e/cypress/support/dataCyType.d.ts (1)
90-90: LGTM!The new data-cy attribute for the user activity feature is properly placed among other administration entries and follows the existing naming convention.
e2e/cypress/e2e/administration/customerDebug.cy.ts (3)
9-9: LGTM!The refactor from
visitAdministrationtovisitAdministrationUsersprovides more specific navigation and aligns with this test's focus on user-related administration actions.Also applies to: 18-18
26-26: LGTM!The debug account button click is correctly implemented using the updated helper functions.
Also applies to: 34-34
38-38: Good addition to verify project creation.The assertion confirms the action succeeded, improving test reliability.
e2e/cypress/common/administration.ts (2)
2-2: LGTM!The refactor to provide separate
visitAdministrationOrganizationsandvisitAdministrationUsersfunctions improves clarity and makes test navigation more explicit.Also applies to: 4-10
12-16: Improved selector specificity.The updated implementation first targets elements with
data-cy="administration-users-list-item"before searching for the userName, reducing the risk of false positives if the username appears elsewhere on the page.e2e/cypress/e2e/administration/base.cy.ts (3)
3-9: LGTM!The new imports support the user last activity test and are all properly utilized.
21-22: LGTM!The navigation updates correctly map tests to their appropriate administration sections (organizations vs. users), improving test clarity.
Also applies to: 52-52, 61-61, 70-70, 98-98
80-95: Consider explicit page reload and verify timezone assumptions.The test logic is sound, but there are a few considerations:
Indirect reload (Line 90): The comment indicates a reload, but clicking the menu item is an indirect navigation. Consider using
cy.reload()or re-callingvisitAdministrationUsers()for more explicit and robust page reloading.Timezone/locale sensitivity (Line 93): The assertion expects "Last activity on August 28, 2023 at 2:00 AM" which assumes a specific timezone and time format. Verify that the test environment has consistent timezone settings, or consider using a more flexible assertion pattern.
Activity recording timing: There's no explicit wait between
createProject(lines 86-89) and reloading (line 90). If activity recording is asynchronous, consider addingcy.wait()orcy.waitForDom()to ensure the activity is persisted before reloading.webapp/src/views/administration/AdministrationUsers.tsx (2)
2-2: LGTM: Imports and hooks are correctly added for the feature.The new imports (
useDateFormatter, Material-UI components) and hooks are necessary for displaying the last activity date in a formatted, localized manner.Also applies to: 4-11, 56-57
95-102: Add validation for empty or invalid date strings.The current check
!u.lastActivityhandles null/undefined but doesn't guard against empty strings or malformed date strings. Ifu.lastActivityis"", it would pass the truthy check butnew Date("")creates an Invalid Date, potentially causingformatDateto fail or display unexpected output.Apply this diff to add robust date validation:
- {!u.lastActivity + {!u.lastActivity || isNaN(new Date(u.lastActivity).getTime()) ? t('administration_user_no_activity') : t('administration_user_last_activity', { date: formatDate(new Date(u.lastActivity), {Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
e2e/cypress/e2e/administration/base.cy.ts (1)
86-90: Consider adding explicit waits for async operations.After creating the project (line 86-89) and reloading the view (line 90), there are no explicit waits to ensure the operations complete. While Cypress has built-in retry logic, adding explicit waits can make the test more robust and clear about its dependencies.
Consider adding waits to ensure operations complete:
createProject({ name: 'just.to.record.activity', languages: [{ name: 'cs', originalName: 'čj', tag: 'cs' }], - }); + }).then(() => { + cy.wait(500); // Ensure activity is recorded + }); gcy('settings-menu-item').contains('Users').click(); + gcy('administration-users-list-item').should('be.visible'); // Ensure page loadedbackend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt (1)
90-90: Redundant login call.The explicit
loginAsUser(userAccount!!.username)is unnecessary because:
userAccountis already set totestData.adminfrom the@BeforeEachsetup- The previous
performAuthGetat line 81 already logged in as admin vialoginAsAdminIfNotLogged()performAuthPostat line 97 won't re-login since a user is already authenticatedRemove line 90 to simplify the test:
val organization = dbPopulator.createOrganization("just.to.record.activity", userAccount!!) - loginAsUser(userAccount!!.username) val projectRequest = CreateProjectRequest(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt(3 hunks)e2e/cypress/e2e/administration/base.cy.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
e2e/cypress/e2e/administration/base.cy.ts (3)
e2e/cypress/common/administration.ts (3)
visitAdministrationOrganizations(4-6)visitAdministrationUsers(8-10)getUserListItem(12-16)e2e/cypress/common/apiCalls/common.ts (2)
forceDate(529-531)releaseForcedDate(533-535)e2e/cypress/common/shared.ts (1)
gcy(28-29)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/testing/AuthorizedControllerTest.kt (4)
performAuthGet(130-133)loginAsUser(59-62)loginAsUser(64-67)performAuthPost(122-128)
🔇 Additional comments (2)
e2e/cypress/e2e/administration/base.cy.ts (2)
3-9: LGTM! Import updates align with the new test and refactored navigation helpers.The new imports support the last activity test (date forcing, project creation) and the more granular navigation helpers improve test organization.
Also applies to: 21-22
52-52: LGTM! Correct use of specialized navigation helpers.The migration from
visitAdministration()to context-specific helpers (visitAdministrationOrganizations()andvisitAdministrationUsers()) improves test clarity and maintainability.Also applies to: 61-61, 70-70, 98-98
.../src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
e2e/cypress/e2e/administration/base.cy.ts (1)
25-25: Consider using a nested describe block for scoped setup/teardown.The current pattern of matching test titles with
Cypress.currentTest.titleis fragile—renaming the test will silently break the date forcing setup.Refactor to use a nested describe block with isolated hooks:
-const activityTest = "can display user's last activity"; - describe('Administration', () => { beforeEach(() => { - if (Cypress.currentTest.title === activityTest) { - forceDate(new Date('2023-08-28T02:30').getTime()); // app displays local time, set this one in local timezone - } administrationTestData.clean(); administrationTestData.generate().then((res) => {}); login('admin@admin.com'); }); afterEach(() => { - if (Cypress.currentTest.title === activityTest) { - releaseForcedDate(); - } administrationTestData.clean(); setProperty('authentication.userCanCreateOrganizations', true); }); // ... existing tests ... + describe('User Activity', () => { + beforeEach(() => { + forceDate(new Date('2023-08-28T02:30').getTime()); // app displays local time, set this one in local timezone + }); + + afterEach(() => { + releaseForcedDate(); + }); + - it(activityTest, () => { + it("can display user's last activity", () => { visitAdministrationUsers(); // ... rest of test }); + }); });Also applies to: 29-31, 38-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
e2e/cypress/e2e/administration/base.cy.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
e2e/cypress/e2e/administration/base.cy.ts (3)
e2e/cypress/common/apiCalls/common.ts (2)
forceDate(529-531)releaseForcedDate(533-535)e2e/cypress/common/administration.ts (3)
visitAdministrationOrganizations(4-6)visitAdministrationUsers(8-10)getUserListItem(12-16)e2e/cypress/common/shared.ts (1)
gcy(28-29)
🔇 Additional comments (3)
e2e/cypress/e2e/administration/base.cy.ts (3)
3-9: LGTM! Imports support the new activity test.All new imports (
createProject,forceDate,releaseForcedDate,setProperty) are properly utilized in the test setup and the new activity test case.
60-60: LGTM! Good refactoring to specialized helpers.Replacing the generic navigation helper with
visitAdministrationOrganizations()andvisitAdministrationUsers()makes the test intent clearer and improves maintainability.Also applies to: 69-69, 78-78, 106-106
88-103: LGTM! Test logic correctly verifies activity tracking.The test properly validates the user activity feature by:
- Confirming initial "No activity yet" state
- Triggering activity via project creation
- Reloading the view and verifying the timestamp appears
The navigation-based reload (line 99) appropriately tests the real user flow.
Anty0
left a comment
Anty0
left a comment
There was a problem hiding this comment.
Good job, looking good. I have some notes, but they should all be easy to fix ^^
.../src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt
Outdated
Show resolved
Hide resolved
.../src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt
Outdated
Show resolved
Hide resolved
backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-07T14:36:39.331Z
Learnt from: Anty0
Repo: tolgee/tolgee-platform PR: 3263
File: backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt:279-290
Timestamp: 2025-10-07T14:36:39.331Z
Learning: In OrganizationRoleService in backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationRoleService.kt, the `removeOrDeactivateUser` method intentionally has two distinct paths: if a user can be removed, it calls `removeUserForReal` (which removes organization roles, project permissions, and evicts caches); if a user is managed by the organization and cannot be removed, it calls `userAccountService.disable(userId)` without any cleanup. This design is intentional—disabling should be reversible so the user stays unmodified (keeping their organization roles and project permissions intact), allowing them to be re-enabled if they return to the organization.
Applied to files:
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt
🧬 Code graph analysis (1)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt (1)
backend/testing/src/main/kotlin/io/tolgee/testing/AuthorizedControllerTest.kt (4)
performAuthGet(130-133)loginAsUser(59-62)loginAsUser(64-67)performAuthPost(122-128)
🔇 Additional comments (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt (2)
4-4: LGTM!The new imports are necessary for the test implementation and are all properly used.
Also applies to: 6-6, 17-17, 23-23
37-40: LGTM!The @AfterEach cleanup ensures
forcedDateis properly reset after each test, preventing state leakage between tests. This addresses the concerns from the previous review.
.../src/test/kotlin/io/tolgee/api/v2/controllers/administration/AdministrationControllerTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webapp/src/views/administration/components/AdministrationUserItem.tsx (1)
33-35: Add defensive coding for consistency with adjacent admin components.The codebase uses
user.name || user.usernamefallback patterns in adjacent administration components (OptionsButton.tsx, MemberItem.tsx). Line 34 lacks this fallback, making it inconsistent. While TypeScript types declare these fields as required, the team applies defensive coding elsewhere in the administration module—follow the same pattern here.Suggested fix:
- {user.name} | {user.username} <Chip size="small" label={user.id} /> + {user.name || user.username} {user.name && user.username && '|'} {user.username} <Chip size="small" label={user.id} />Or simpler, if name is display-preferred:
- {user.name} | {user.username} <Chip size="small" label={user.id} /> + {user.name ? `${user.name} | ${user.username}` : user.username} <Chip size="small" label={user.id} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
e2e/cypress/e2e/administration/base.cy.ts(6 hunks)webapp/src/views/administration/AdministrationUsers.tsx(2 hunks)webapp/src/views/administration/components/AdministrationUserItem.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-03T08:38:27.934Z
Learnt from: Anty0
Repo: tolgee/tolgee-platform PR: 3248
File: backend/data/src/main/kotlin/io/tolgee/service/organization/OrganizationService.kt:157-157
Timestamp: 2025-10-03T08:38:27.934Z
Learning: The SUPPORTER role is read-only and should not be able to create organizations in the name of other users. When checking if a user can create organizations, use `isAdmin()` rather than `isSupporterOrAdmin()` to exclude SUPPORTER from this capability.
Applied to files:
e2e/cypress/e2e/administration/base.cy.ts
🧬 Code graph analysis (3)
webapp/src/views/administration/AdministrationUsers.tsx (1)
webapp/src/views/administration/components/AdministrationUserItem.tsx (1)
AdministrationUserItem(18-62)
e2e/cypress/e2e/administration/base.cy.ts (3)
e2e/cypress/common/administration.ts (3)
visitAdministrationOrganizations(4-6)visitAdministrationUsers(8-10)getUserListItem(12-16)e2e/cypress/common/apiCalls/common.ts (2)
releaseForcedDate(533-535)forceDate(529-531)e2e/cypress/common/shared.ts (1)
gcy(28-29)
webapp/src/views/administration/components/AdministrationUserItem.tsx (4)
webapp/src/component/MfaBadge.tsx (1)
MfaBadge(34-53)webapp/src/views/administration/components/DebugCustomerAccountButton.tsx (1)
DebugCustomerAccountButton(8-39)webapp/src/views/administration/components/RoleSelector.tsx (1)
RoleSelector(14-68)webapp/src/views/administration/components/OptionsButton.tsx (1)
OptionsButton(18-133)
🔇 Additional comments (4)
webapp/src/views/administration/AdministrationUsers.tsx (1)
3-3: Excellent refactoring that addresses previous feedback.The extraction of user item rendering into the
AdministrationUserItemcomponent successfully addresses the suggestion from the previous review to split this into a separate component. This improves code organization and maintainability.Also applies to: 9-9, 66-69
webapp/src/views/administration/components/AdministrationUserItem.tsx (3)
1-16: LGTM - Clean component structure and type definitions.The imports, type alias, and props interface are well-organized and appropriate for the component's needs.
54-59: LGTM - Actions layout and props are correct.The actions section properly renders all user-related controls with appropriate props and spacing.
43-50: Remove the defensive date parsing suggestion—the code is correct as-is.The API schema explicitly defines
lastActivityasstringwith formatdate-time, which means the backend guarantees valid ISO 8601 date strings. TheuseDateFormatter()hook safely usesIntl.DateTimeFormatto format dates, andnew Date(string)correctly parses ISO 8601 strings. The existing null check (!user.lastActivity) already covers the only legitimate edge case. Adding defensive parsing would mask a backend contract violation rather than handle a real frontend issue.Likely an incorrect or invalid review comment.
Anty0
left a comment
Anty0
left a comment
There was a problem hiding this comment.
LGTM (except for the note on the deprecated property). The PR itself needs to wait until we extend the activity; otherwise, the lastActivity would lag behind when the user is not performing any of the tracked activities.
| @Deprecated("See `UserAccount` for details.") | ||
| /** @see [io.tolgee.model.UserAccount.thirdPartyAuthType] */ | ||
| override val accountType: UserAccount.AccountType?, |
There was a problem hiding this comment.
@JanCizmar What do you think about this? I wouldn't mark it as deprecated. If anything, the accountType is duplicated data in the database, but from the API perspective, I wouldn't stop providing the accountType – I wouldn't deprecate it.
There was a problem hiding this comment.
It comes from my possible misunderstanding of:
/**
* This property is redundant - it's value can be derived from other existing properties.
* Kept for legacy reasons.in backend/data/src/main/kotlin/io/tolgee/model/UserAccount.kt.
There was a problem hiding this comment.
Yup, from my perspective, it isn't really deprecated. It can be calculated from other columns rather than stored in its own database column—that's what the comment is trying to say.
3 participants
Requested in #2973.
Based on #3238.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests