-
-
Notifications
You must be signed in to change notification settings - Fork 302
feat: UserAccount's lastActivity in /v2/administration/users #3238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughBackend shifts user listing from entity to view (UserAccountView), adding deleted/disabled timestamps and lastActivity throughout repository, service, assembler, and model. API/webapp schema updated (lastActivity added; LLM prompt removed, getInfo_4 response type changed). UI now shows users’ last activity. Docs add auth config and schema-refresh instructions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin User
participant UI as Webapp (AdministrationUsers)
participant API as AdministrationController
participant SVC as UserAccountService
participant REPO as UserAccountRepository
participant DB as DB/CTE
Admin->>UI: Open Users page
UI->>API: GET /v2/administration/users (paged)
API->>SVC: findAllWithDisabledPaged(search, pageable)
SVC->>REPO: findAllWithDisabledPaged(search, pageable)
REPO->>DB: Query with lastActivity CTE + projections
DB-->>REPO: Page<UserAccountView> (includes deletedAt, disabledAt, lastActivity)
REPO-->>SVC: Page<UserAccountView>
SVC-->>API: Page<UserAccountView>
API->>API: Assemble to UserAccountModel (maps lastActivity)
API-->>UI: Paged UserAccountModel list
UI->>UI: Render user info + “Last Activity”
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (1)
backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt (1)
262-296
: Paging with HQL CTE likely breaks count query derivation; add explicit countQuerySpring Data often fails to derive a count for JPQL/HQL queries using WITH CTEs and DTO projections, causing runtime errors on pageable endpoints. Provide a countQuery that omits the CTE and just counts UserAccount.
@Query( - """ + value = """ with lastActivityCTE as ( select ar.authorId as authorId, max(ar.timestamp) as lastActivity from ActivityRevision ar group by ar.authorId ) select new io.tolgee.dtos.queryResults.UserAccountView( userAccount.id, userAccount.username, userAccount.name, case when ev is not null then coalesce(ev.newEmail, userAccount.username) else null end, userAccount.avatarHash, userAccount.accountType, userAccount.thirdPartyAuthType, userAccount.role, userAccount.isInitialUser, userAccount.totpKey, userAccount.deletedAt, userAccount.disabledAt, la.lastActivity ) from UserAccount userAccount left join userAccount.emailVerification ev left join lastActivityCTE la on la.authorId = userAccount.id where ((lower(userAccount.name) like lower(concat('%', cast(:search as text),'%')) or lower(userAccount.username) like lower(concat('%', cast(:search as text),'%'))) or cast(:search as text) is null) and userAccount.deletedAt is null - """, + """, + countQuery = """ + select count(userAccount) + from UserAccount userAccount + where ( + (lower(userAccount.name) like lower(concat('%', cast(:search as text), '%')) + or lower(userAccount.username) like lower(concat('%', cast(:search as text), '%'))) + or cast(:search as text) is null + ) and userAccount.deletedAt is null + """ ) fun findAllWithDisabledPaged( search: String?, pageable: Pageable, ): Page<UserAccountView>If HQL CTE support varies across DBs/dialects, consider the alternative below.
🧹 Nitpick comments (10)
backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModel.kt (1)
7-7
: API surface OK; consider java.time types instead of DateAdding lastActivity is fine and aligns with the UI. For future-proofing, consider java.time.Instant/OffsetDateTime across API models to avoid timezone ambiguity. Not blocking.
Also applies to: 18-18
DEVELOPMENT.md (2)
65-73
: Docs: warn about default credentials and committing real email
- Make it explicit that initial-password must be changed immediately.
- Recommend using a non-personal placeholder email for local dev to avoid committing PII to VCS.
80-86
: Docs: add tip to re-generate schema on backend changesGood callout. Consider also mentioning to commit the regenerated schema to avoid CI drift.
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/AdministrationController.kt (2)
42-46
: Typos in public API docs“Managees” → “Manages”. Minor but user-facing.
- description = - "**Only for self-hosted instances** \n\n" + - "Managees global Tolgee Platform instance data e.g., user accounts and organizations.", + description = + "**Only for self-hosted instances** \n\n" + + "Manages global Tolgee Platform instance data, e.g., user accounts and organizations.",
141-146
: Typos in endpoint description“Geneate” → “Generate”; tighten phrasing.
- @Operation( - summary = "Geneate user's JWT token", - description = - "Generates a JWT token for the user with provided ID. This is useful, when need to debug of the " + - "user's account. Or when an operation is required to be executed on behalf of the user.", - ) + @Operation( + summary = "Generate user's JWT token", + description = + "Generates a JWT token for the user with the provided ID. Useful for debugging a user's account or executing an operation on their behalf.", + )webapp/src/views/administration/AdministrationUsers.tsx (1)
74-87
: Localize strings and guard against invalid date
- Hardcoded “Last Activity”/“No activity yet” should be translated.
- If backend ever sends an unparsable value, new Date(...).toLocaleString() yields “Invalid Date”.
- <Typography + <Typography variant="caption" color="text.secondary" sx={{ fontStyle: 'italic', display: 'block' }} > - {u.lastActivity - ? `Last Activity: ${new Date(u.lastActivity).toLocaleString()}` - : "No activity yet" - } + {u.lastActivity ? (() => { + const d = new Date(u.lastActivity as string); + return `${t('administration_users_last_activity', 'Last activity')}: ${ + isNaN(d.getTime()) ? t('date_invalid', 'Invalid date') : d.toLocaleString() + }`; + })() : t('administration_users_no_activity', 'No activity yet')} </Typography>If desired, wrap the date with a element for better semantics.
backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt (2)
262-296
: Simpler, portable alternative: correlated subquery instead of CTEThis avoids CTE, keeps JPQL portable, and will let Spring derive the count (you can still keep the explicit countQuery).
- with lastActivityCTE as ( - select ar.authorId as authorId, max(ar.timestamp) as lastActivity - from ActivityRevision ar - group by ar.authorId - ) select new io.tolgee.dtos.queryResults.UserAccountView( userAccount.id, userAccount.username, userAccount.name, case when ev is not null then coalesce(ev.newEmail, userAccount.username) else null end, userAccount.avatarHash, userAccount.accountType, userAccount.thirdPartyAuthType, userAccount.role, userAccount.isInitialUser, userAccount.totpKey, userAccount.deletedAt, userAccount.disabledAt, - la.lastActivity + (select max(ar.timestamp) from ActivityRevision ar where ar.authorId = userAccount.id) ) from UserAccount userAccount - left join userAccount.emailVerification ev - left join lastActivityCTE la on la.authorId = userAccount.id + left join userAccount.emailVerification ev
262-296
: Indexing advice for lastActivity lookupEnsure ActivityRevision has an index on (authorId, timestamp desc) to keep max(timestamp) lookups fast.
backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountView.kt (2)
6-6
: Avoid wildcard import.Import the specific type to keep imports tidy and avoid unintended symbol pulls.
-import java.util.* +import java.util.Date
19-21
: Prefer java.time.Instant for new timestamps.Using Instant avoids Date’s timezone pitfalls and aligns with modern Java/Kotlin time APIs. If changing types is feasible across layers, migrate these three fields and the factory accordingly.
- val deletedAt: Date?, - val disabledAt: Date?, - val lastActivity: Date? + val deletedAt: Instant?, + val disabledAt: Instant?, + val lastActivity: Instant? ... - fun fromEntity(entity: UserAccount, lastActivity: Date?): UserAccountView { + fun fromEntity(entity: UserAccount, lastActivity: Instant?): UserAccountView { ... - deletedAt = entity.deletedAt, - disabledAt = entity.disabledAt, - lastActivity = lastActivity, + deletedAt = entity.deletedAt?.toInstant(), + disabledAt = entity.disabledAt?.toInstant(), + lastActivity = lastActivity,If keeping Date for now, ensure consistent UTC serialization in the API model.
Also applies to: 24-24, 36-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
DEVELOPMENT.md
(1 hunks)backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/AdministrationController.kt
(2 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModel.kt
(2 hunks)backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModelAssembler.kt
(2 hunks)backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountView.kt
(3 hunks)backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt
(3 hunks)backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt
(1 hunks)webapp/src/service/apiSchema.generated.ts
(1 hunks)webapp/src/views/administration/AdministrationUsers.tsx
(2 hunks)
🔇 Additional comments (9)
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/AdministrationController.kt (1)
6-6
: Swapping to UserAccountView in assembler is consistent with service/repo changesLooks correct; Spring will inject PagedResourcesAssembler regardless of generic. No issues spotted.
Also applies to: 55-55
backend/data/src/main/kotlin/io/tolgee/service/security/UserAccountService.kt (1)
572-574
: No remaining usages expecting Page Only the service definition, repository signature, and a single call in AdministrationController.kt were found—and that call already handles Page correctly.backend/data/src/main/kotlin/io/tolgee/repository/UserAccountRepository.kt (1)
149-153
: Projection update OKAdding totpKey/deletedAt/disabledAt/lastActivity placeholders is consistent with UserAccountView’s ctor.
backend/data/src/main/kotlin/io/tolgee/dtos/queryResults/UserAccountView.kt (1)
24-24
: No UserAccountView.fromEntity usages found Signature change is safe; no two-argument calls remain.backend/api/src/main/kotlin/io/tolgee/hateoas/userAccount/UserAccountModelAssembler.kt (4)
14-18
: Type switch to UserAccountView looks correct.Generics and toModel signature align with the new view source.
19-19
: Avatar nullability—verify service behavior.Ensure avatarService.getAvatarLinks handles null avatarHash without throwing and returns sensible defaults.
22-25
: Field mapping LGTM, including lastActivity and deleted/disabled flags.Defaults and boolean derivations are sensible.
Also applies to: 27-32
18-18
:isMfaEnabled
extension resolves correctly forUserAccountView
. Verified thatUserAccountView
implementsIUserAccount
(which extendsIMfa
) and providestotpKey
, so theisMfaEnabled
extension onIMfa
applies as expected.webapp/src/service/apiSchema.generated.ts (1)
5928-5930
: lastActivity usage is safe; confirm ISO 8601 format
In AdministrationUsers.tsx (lines 82–83), the UI uses a truthy check before callingnew Date(u.lastActivity)
, preventing undefined errors, and there’s no sorting logic onlastActivity
. Ensure the backend returns an ISO 8601 date-time string (e.g.,2025-09-08T14:22:00Z
).
Hey, please run prettier to fix the frontend issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Thanks for the PR! ^^
As @dzienisz mentioned, please run npm run eslint -- --fix && npm run tsc
in the webapp
directory to fix linting errors.
The PR looks fine, but @JanCizmar can you take a look at the changes made to UserAccountView
? I don't like that it adds the lastActivity
field, but then leaves it empty when created outside of the one specific query. I'd prefer having a separate view specifically for the admin page that includes the lastActivity
. What's your opinion, @JanCizmar?
|
||
## API schema changes | ||
|
||
After you change the API schema, you need to run the webapp schema script to update Frontend: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should also mention that the backend must be running for the schema
script to work.
Summary by CodeRabbit