feat(user): add support for super_admin role, service account update and soft-delete#1903
Open
subratsahilgupta wants to merge 2 commits into
Open
feat(user): add support for super_admin role, service account update and soft-delete#1903subratsahilgupta wants to merge 2 commits into
subratsahilgupta wants to merge 2 commits into
Conversation
Contributor
WalkthroughThis PR adds a ChangesUser Name Field and Service Account Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant UserService
participant UserRepository
participant EntClient
Client->>Handler: PUT /users/{id} + UpdateUserRequest
Handler->>UserService: UpdateUser(ctx, id, request)
UserService->>UserRepository: Get user by id
UserRepository->>EntClient: Query user
EntClient-->>UserRepository: User (check type)
UserRepository-->>UserService: domain.User
UserService->>UserService: Validate ServiceAccount type
UserService->>UserRepository: Update name + audit fields
UserRepository->>EntClient: Update mutation
EntClient-->>UserRepository: updated ent.User
UserRepository-->>UserService: domain.User
UserService-->>Handler: dto.UpdateUserResponse
Handler-->>Client: 200 + UserResponse
Client->>Handler: DELETE /users/{id}
Handler->>UserService: DeleteUser(ctx, id)
UserService->>UserRepository: Get user by id
UserRepository-->>UserService: domain.User
UserService->>UserService: Validate ServiceAccount type
UserService->>UserRepository: Delete (soft-archive)
UserRepository->>EntClient: Update status=archived
EntClient-->>UserRepository: affected count
UserRepository-->>UserService: nil or error
UserService-->>Handler: nil or error
Handler-->>Client: 204 No Content or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: no go files to analyze: running 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/testutil/inmemory_user_store.go (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStore keyed by
InMemoryUserStore.Createstores users inr.users[user.Email]; if service accounts are created with an emptyErrAlreadyExistsand makingGetByEmail("")lookups ambiguous.
internal/service/user_test.gohas service-account test cases, but it recreatess.userRepo = testutil.NewInMemoryUserStore()per subtest, so the current tests may not cover the “multiple service accounts in one store instance” scenario. Consider keying byID(or otherwise using a non-empty unique key for service accounts) instead of🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutil/inmemory_user_store.go` around lines 30 - 34, The store currently keys users by email in InMemoryUserStore.Create (r.users[user.Email]) which collides for service accounts with empty Email; change the store to key by a non-empty unique field (use user.ID) and update all related methods (e.g., InMemoryUserStore.Create, GetByEmail, any direct r.users accesses) to maintain a map keyed by ID while keeping a secondary email->ID index or scanning for GetByEmail lookups so GetByEmail("") and multiple service accounts work correctly; ensure Create still checks uniqueness (by ID and/or email) and that tests referencing GetByEmail continue to behave as expected.
🧹 Nitpick comments (3)
internal/repository/ent/user.go (1)
110-112: 💤 Low valueName cannot be cleared via Update.
Because
SetNameis only applied whenuser.Name != "", an explicit request to clear a service account's display name (empty string) is silently ignored. If clearing is a supported operation, distinguish "unset" from "set to empty" (e.g., a*stringor an explicit flag) at the service/DTO boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/repository/ent/user.go` around lines 110 - 112, The update currently ignores explicit requests to clear Name because it only calls updateQuery.SetName when user.Name != "", so change the service/DTO to distinguish "unset" vs "set to empty" (e.g., make Name a *string or add an explicit flag), then update this code to check for nil rather than empty string: if the DTO's Name pointer is non-nil call updateQuery.SetName(*user.Name) (which allows setting an empty string), otherwise leave the field unchanged; ensure you update any callers and converters to populate nil vs pointer values accordingly.internal/testutil/inmemory_user_store.go (1)
112-124: 💤 Low value
Deletediverges from prod soft-delete semantics (idempotency/tenant).The ent repository only archives a
publisheduser (scoped by tenant) and returnsErrNotFoundon a second delete. This in-memoryDeletere-archives unconditionally and ignores tenant, so a second call returnsnilinstead ofErrNotFound. If tests assert delete idempotency or tenant isolation, behavior will differ from production.♻️ Align with prod guard
for key, u := range r.users { - if u.ID == id { + if u.ID == id && u.Status == types.StatusPublished { u.Status = types.StatusArchived r.users[key] = u return nil } } return ierr.NewError("user not found").Mark(ierr.ErrNotFound)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutil/inmemory_user_store.go` around lines 112 - 124, In InMemoryUserStore.Delete, make behavior match production: while holding r.mu, locate the user by id and tenant (check u.ID == id and u.Tenant == ctxTenant), only archive if u.Status == types.StatusPublished (set u.Status = types.StatusArchived and write back to r.users), and return nil; if no matching published user is found (either missing, wrong tenant, or already archived), return ierr.NewError("user not found").Mark(ierr.ErrNotFound) so a second delete returns ErrNotFound; keep the existing locking and use the same error type.internal/service/user_test.go (1)
231-262: 💤 Low valueConsider adding test coverage for new service account operations.
The test file only covers metadata merge for the authenticated user. The new
UpdateUserwith explicit ID (service-account path) andDeleteUsermethod lack test coverage. Consider adding table-driven tests for:
- Updating a service account by explicit ID
- Rejecting update when ID points to a non-service-account user
- Deleting a service account
- Rejecting deletion of non-service-account users
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/service/user_test.go` around lines 231 - 262, Add table-driven tests to internal/service/user_test.go covering service-account paths for s.userService.UpdateUser and s.userService.DeleteUser: create service-account and non-service-account users via s.userRepo.Create, then test UpdateUser with an explicit ID (ensure successful update only for service-account and rejected for non-service-account) using dto.UpdateUserRequest with Metadata and verify merged fields, and test DeleteUser by ID (successful for service-account and rejected for non-service-account). Place tests alongside TestUpdateUser_MetadataMerge in the UserServiceSuite and reuse ctx/baseModel setup to assert success responses and error cases for each table entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/api/v1/user.go`:
- Around line 185-205: The Swagger annotation for DeleteUser is inconsistent
with the handler: update the comment on the DeleteUser method to reflect the
actual 204 response by replacing "`@Success` 200 {object} dto.UpdateUserResponse"
with a 204 annotation (e.g. "`@Success` 204 {string} string \"No Content\"" or
simply "`@Success` 204 \"No Content\"") so the doc no longer claims a response
body; keep the handler implementation (UserHandler.DeleteUser) unchanged.
In `@internal/service/user.go`:
- Around line 360-371: DeleteUser currently passes an empty id to
userRepo.GetByID which yields a confusing "not found" error; add the same
empty-id handling used in UpdateUser: if id == "" resolve the target id from the
request context (the context user ID retrieval used by UpdateUser) and use that
resolved id for GetByID/Delete, or if your codebase prefers explicit validation
return a clear validation error when no context user is present. Update the
logic in DeleteUser (referencing DeleteUser, UpdateUser, and GetByID) to either
resolve empty id from ctx before calling GetByID or return a marked validation
error rather than calling GetByID with an empty string.
---
Outside diff comments:
In `@internal/testutil/inmemory_user_store.go`:
- Around line 30-34: The store currently keys users by email in
InMemoryUserStore.Create (r.users[user.Email]) which collides for service
accounts with empty Email; change the store to key by a non-empty unique field
(use user.ID) and update all related methods (e.g., InMemoryUserStore.Create,
GetByEmail, any direct r.users accesses) to maintain a map keyed by ID while
keeping a secondary email->ID index or scanning for GetByEmail lookups so
GetByEmail("") and multiple service accounts work correctly; ensure Create still
checks uniqueness (by ID and/or email) and that tests referencing GetByEmail
continue to behave as expected.
---
Nitpick comments:
In `@internal/repository/ent/user.go`:
- Around line 110-112: The update currently ignores explicit requests to clear
Name because it only calls updateQuery.SetName when user.Name != "", so change
the service/DTO to distinguish "unset" vs "set to empty" (e.g., make Name a
*string or add an explicit flag), then update this code to check for nil rather
than empty string: if the DTO's Name pointer is non-nil call
updateQuery.SetName(*user.Name) (which allows setting an empty string),
otherwise leave the field unchanged; ensure you update any callers and
converters to populate nil vs pointer values accordingly.
In `@internal/service/user_test.go`:
- Around line 231-262: Add table-driven tests to internal/service/user_test.go
covering service-account paths for s.userService.UpdateUser and
s.userService.DeleteUser: create service-account and non-service-account users
via s.userRepo.Create, then test UpdateUser with an explicit ID (ensure
successful update only for service-account and rejected for non-service-account)
using dto.UpdateUserRequest with Metadata and verify merged fields, and test
DeleteUser by ID (successful for service-account and rejected for
non-service-account). Place tests alongside TestUpdateUser_MetadataMerge in the
UserServiceSuite and reuse ctx/baseModel setup to assert success responses and
error cases for each table entry.
In `@internal/testutil/inmemory_user_store.go`:
- Around line 112-124: In InMemoryUserStore.Delete, make behavior match
production: while holding r.mu, locate the user by id and tenant (check u.ID ==
id and u.Tenant == ctxTenant), only archive if u.Status == types.StatusPublished
(set u.Status = types.StatusArchived and write back to r.users), and return nil;
if no matching published user is found (either missing, wrong tenant, or already
archived), return ierr.NewError("user not found").Mark(ierr.ErrNotFound) so a
second delete returns ErrNotFound; keep the existing locking and use the same
error type.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcaf667c-663b-4cdc-bb45-40b83a7cd55d
📒 Files selected for processing (20)
ent/migrate/schema.goent/mutation.goent/runtime.goent/schema/user.goent/user.goent/user/user.goent/user/where.goent/user_create.goent/user_update.gointernal/api/dto/user.gointernal/api/router.gointernal/api/v1/user.gointernal/config/rbac/roles.jsoninternal/domain/user/model.gointernal/domain/user/repository.gointernal/rbac/rbac.gointernal/repository/ent/user.gointernal/service/user.gointernal/service/user_test.gointernal/testutil/inmemory_user_store.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/testutil/inmemory_user_store.go (1)
55-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Updatestill drops the newNamefield.This store is part of the service-test support for the PR, but
Updateonly persists metadata/audit fields. With the new usernamesupport added across the service contract, tests backed by this double won't exercise repository semantics forNameupdates reliably.Suggested fix
if u.ID == updatedUser.ID && u.TenantID == updatedUser.TenantID { + u.Name = updatedUser.Name if updatedUser.Metadata != nil { u.Metadata = updatedUser.Metadata } u.UpdatedBy = updatedUser.UpdatedBy🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/testutil/inmemory_user_store.go` around lines 55 - 63, The Update method in inmemory_user_store.go currently only persists metadata and audit fields, dropping changes to the new Name field; modify the Update function (the loop over r.users comparing u.ID and u.TenantID) to also copy updatedUser.Name into the stored user (u.Name = updatedUser.Name) when a new name is provided (e.g., non-empty or non-nil per your User type semantics), just like how Metadata is handled, then write r.users[key] = u and return.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/testutil/inmemory_user_store.go`:
- Around line 116-118: The lookup of tenant context drops the ok flag so a
missing types.CtxTenantID is treated like an empty/ wildcard tenant; change the
lookup to capture the boolean (tenantID, ok :=
ctx.Value(types.CtxTenantID).(string)) and require ok to be true before allowing
the wildcard branch in the loop that iterates r.users (where the check is u.ID
== id && ...). In other words, only allow deletion when the tenant context
exists (ok == true) and either tenantID == "" (explicit empty tenant) or
u.TenantID == tenantID; if ok is false, do not match and do not perform
cross-tenant deletes.
---
Outside diff comments:
In `@internal/testutil/inmemory_user_store.go`:
- Around line 55-63: The Update method in inmemory_user_store.go currently only
persists metadata and audit fields, dropping changes to the new Name field;
modify the Update function (the loop over r.users comparing u.ID and u.TenantID)
to also copy updatedUser.Name into the stored user (u.Name = updatedUser.Name)
when a new name is provided (e.g., non-empty or non-nil per your User type
semantics), just like how Metadata is handled, then write r.users[key] = u and
return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9684c9f7-b8a3-469d-8f76-49e90b98816b
📒 Files selected for processing (3)
internal/api/v1/user.gointernal/service/user.gointernal/testutil/inmemory_user_store.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/service/user.go
- internal/api/v1/user.go
Comment on lines
+116
to
+118
| tenantID, _ := ctx.Value(types.CtxTenantID).(string) | ||
| for key, u := range r.users { | ||
| if u.ID == id && (tenantID == "" || u.TenantID == tenantID) { |
Contributor
There was a problem hiding this comment.
Missing tenant context currently turns into a cross-tenant delete.
Line 116 drops the ok result from the context lookup, so a missing types.CtxTenantID is treated the same as an intentionally empty tenant and the wildcard branch on Line 118 becomes active. That means service tests can forget to propagate tenant context and still pass deletion across tenants, which weakens the multitenant contract this double is supposed to enforce.
Suggested fix
- tenantID, _ := ctx.Value(types.CtxTenantID).(string)
+ tenantID, ok := ctx.Value(types.CtxTenantID).(string)
+ if !ok {
+ return ierr.NewError("tenant ID not found in context").Mark(ierr.ErrValidation)
+ }
for key, u := range r.users {
if u.ID == id && (tenantID == "" || u.TenantID == tenantID) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tenantID, _ := ctx.Value(types.CtxTenantID).(string) | |
| for key, u := range r.users { | |
| if u.ID == id && (tenantID == "" || u.TenantID == tenantID) { | |
| tenantID, ok := ctx.Value(types.CtxTenantID).(string) | |
| if !ok { | |
| return ierr.NewError("tenant ID not found in context").Mark(ierr.ErrValidation) | |
| } | |
| for key, u := range r.users { | |
| if u.ID == id && (tenantID == "" || u.TenantID == tenantID) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/testutil/inmemory_user_store.go` around lines 116 - 118, The lookup
of tenant context drops the ok flag so a missing types.CtxTenantID is treated
like an empty/ wildcard tenant; change the lookup to capture the boolean
(tenantID, ok := ctx.Value(types.CtxTenantID).(string)) and require ok to be
true before allowing the wildcard branch in the loop that iterates r.users
(where the check is u.ID == id && ...). In other words, only allow deletion when
the tenant context exists (ok == true) and either tenantID == "" (explicit empty
tenant) or u.TenantID == tenantID; if ok is false, do not match and do not
perform cross-tenant deletes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Security
Chores