Update writers & auditors to be objects #29
Conversation
Replace simple string arrays for writers and auditors with rich user objects containing name, email, username, and avatar fields. This provides better user experience for display purposes while maintaining backward compatibility for access control systems. Changes: - Add UserInfo type with name, email, username, avatar fields - Update Goa API design and regenerate types - Update domain models and service layer converters - Maintain username extraction for OpenFGA access control - Update tests for new structure Breaking Change: API consumers must update to use new user object format instead of simple username strings for writers and auditors fields. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
Update root-project-setup and load_mock_data scripts to use the new UserInfo objects instead of simple strings for writers and auditors. Changes: - Update root-project-setup to parse structured user data with backward compatibility - Support both "username" and "username:name:email:avatar" formats - Update load_mock_data to generate UserInfo objects with mock data - Add helper functions for creating test user objects Environment Variable Formats: - Simple (backward compatible): ROOT_PROJECT_WRITERS="user1,user2" - Structured: ROOT_PROJECT_WRITERS="user1:John Doe:[email protected]:avatar.jpg,user2:Jane Smith:[email protected]:" 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
Complete the test file updates to support the new UserInfo structure for writers and auditors fields. All tests now use proper UserInfo objects instead of simple strings. Changes: - Fix NATS repository tests to use UserInfo objects - Update service converter tests with helper functions - Fix project operations tests - Ensure all test fixtures use realistic user data - All tests now pass successfully This completes the migration to the new UserInfo structure across the entire codebase including all test files. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
Update the Helm chart to support both simple and structured formats for root project writers and auditors. The chart now correctly handles the new UserInfo structure while maintaining backward compatibility. Changes: - Update Chart.yaml version from 0.4.4 to 0.5.0 - Fix deployment template to properly quote environment variables - Update values.yaml with structured format examples and documentation - Add support for mixed format usage (simple + structured in same array) Environment Variable Formats Supported: - Simple: "username1,username2" - Structured: "user:Name:[email protected]:avatar.jpg,user2:Name2:email2:" - Mixed: "simple_user,structured:Full Name:[email protected]:" The root-project-setup script automatically detects and handles both formats. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Jordan Evans <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a UserInfo type and replaces writer/auditor string lists with structured UserInfo across API DSL, domain models, converters, service operations, tests, Helm charts/values, mock data, and root-project setup parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as Project API (projsvc)
participant Svc as Service Layer
participant Repo as Repository/DB
participant NATS as Publisher
Client->>API: CreateProject(payload with writers/auditors as UserInfo[])
API->>Svc: Convert payload → domain (UserInfo[] via convertUsersFromAPI)
Svc->>Repo: Save project/settings
Repo-->>Svc: Saved project
Note over Svc: extractUsernames(UserInfo[]) → []string for downstream messages
Svc->>NATS: Publish access/index updates (usernames only)
NATS-->>Svc: Ack
Svc-->>API: Project created (UserInfo[] preserved)
API-->>Client: Response (writers/auditors as UserInfo[])
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
🔇 Additional comments (3)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This pull request transforms the project API to use richer user profile information by introducing a new UserInfo type and updating project auditors and writers from simple username strings to detailed user objects containing name, email, username, and avatar information.
Key changes include:
- Introduction of a new
UserInfotype with comprehensive user profile data - Conversion of
auditorsandwritersfrom string arrays toUserInfoobject arrays across all API layers - Addition of backward compatibility support in environment parsing for root project configuration
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/domain/models/project.go | Adds new UserInfo struct with profile fields and updates ProjectSettings to use UserInfo arrays |
| internal/service/converters.go | Implements conversion functions between domain and API UserInfo types |
| internal/service/project_operations.go | Updates to extract usernames from UserInfo objects for access control |
| api/project/v1/design/types.go | Defines UserInfo DSL type and updates attribute functions to use UserInfo arrays |
| scripts/root-project-setup/main.go | Adds parseUserInfo function supporting both simple and structured user formats |
| api/project/v1/gen/http/project_service/server/types.go | Updates generated HTTP types to use UserInfo request/response bodies |
| charts/lfx-v2-project-service/values.yaml | Updates configuration format to support structured user information |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| parts := strings.Split(trimmed, ":") | ||
| user := models.UserInfo{ | ||
| Username: parts[0], | ||
| } | ||
| if len(parts) > 1 { | ||
| user.Name = parts[1] | ||
| } | ||
| if len(parts) > 2 { | ||
| user.Email = parts[2] | ||
| } | ||
| if len(parts) > 3 { | ||
| user.Avatar = parts[3] | ||
| } |
There was a problem hiding this comment.
Potential panic if parts[0] is accessed when parts slice is empty. The code should check len(parts) > 0 before accessing parts[0].
Copilot uses AI. Check for mistakes.
| // createTestUserInfo creates a UserInfo for testing purposes | ||
| func createTestUserInfo(username, name, email, avatar string) models.UserInfo { | ||
| return models.UserInfo{ | ||
| Name: name, | ||
| Email: email, | ||
| Username: username, | ||
| Avatar: avatar, | ||
| } | ||
| } | ||
|
|
||
| // createTestAPIUserInfo creates an API UserInfo for testing purposes | ||
| func createTestAPIUserInfo(username, name, email, avatar string) *projsvc.UserInfo { | ||
| return &projsvc.UserInfo{ | ||
| Name: &name, | ||
| Email: &email, | ||
| Username: &username, | ||
| Avatar: &avatar, | ||
| } | ||
| } |
There was a problem hiding this comment.
Test utility functions should be moved to a test file or testing package instead of being included in production code. These functions are only used in tests but are compiled into the production binary.
| // createTestUserInfo creates a UserInfo for testing purposes | |
| func createTestUserInfo(username, name, email, avatar string) models.UserInfo { | |
| return models.UserInfo{ | |
| Name: name, | |
| Email: email, | |
| Username: username, | |
| Avatar: avatar, | |
| } | |
| } | |
| // createTestAPIUserInfo creates an API UserInfo for testing purposes | |
| func createTestAPIUserInfo(username, name, email, avatar string) *projsvc.UserInfo { | |
| return &projsvc.UserInfo{ | |
| Name: &name, | |
| Email: &email, | |
| Username: &username, | |
| Avatar: &avatar, | |
| } | |
| } |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/load_mock_data/main.go (1)
37-45: Fix ProjectResponse to match structured auditors/writers payloadThe service now returns auditors and writers as user objects, but
ProjectResponsestill expects[]string. When the script decodes the API response this mismatch triggersjson: cannot unmarshal object into Go struct field … of type string, breaking the loader.Update these fields to the new shape:
type ProjectResponse struct { ID *string `json:"id,omitempty"` Slug *string `json:"slug,omitempty"` Name *string `json:"name,omitempty"` Description *string `json:"description,omitempty"` Public *bool `json:"public,omitempty"` ParentUID *string `json:"parent_uid,omitempty"` - Auditors []string `json:"auditors,omitempty"` - Writers []string `json:"writers,omitempty"` + Auditors []*projectservice.UserInfo `json:"auditors,omitempty"` + Writers []*projectservice.UserInfo `json:"writers,omitempty"` }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (11)
api/project/v1/gen/http/cli/lfx_v2_project_service/cli.gois excluded by!**/gen/**api/project/v1/gen/http/openapi.jsonis excluded by!**/gen/**api/project/v1/gen/http/openapi.yamlis excluded by!**/gen/**api/project/v1/gen/http/openapi3.jsonis excluded by!**/gen/**api/project/v1/gen/http/openapi3.yamlis excluded by!**/gen/**api/project/v1/gen/http/project_service/client/cli.gois excluded by!**/gen/**api/project/v1/gen/http/project_service/client/encode_decode.gois excluded by!**/gen/**api/project/v1/gen/http/project_service/client/types.gois excluded by!**/gen/**api/project/v1/gen/http/project_service/server/encode_decode.gois excluded by!**/gen/**api/project/v1/gen/http/project_service/server/types.gois excluded by!**/gen/**api/project/v1/gen/project_service/service.gois excluded by!**/gen/**
📒 Files selected for processing (14)
api/project/v1/design/types.go(1 hunks)charts/lfx-v2-project-service/Chart.yaml(1 hunks)charts/lfx-v2-project-service/templates/deployment.yaml(1 hunks)charts/lfx-v2-project-service/values.yaml(1 hunks)cmd/project-api/service_endpoint_project_test.go(3 hunks)internal/domain/models/project.go(2 hunks)internal/domain/models/project_test.go(1 hunks)internal/infrastructure/nats/repository_test.go(1 hunks)internal/service/converters.go(4 hunks)internal/service/converters_test.go(4 hunks)internal/service/project_operations.go(3 hunks)internal/service/project_operations_test.go(1 hunks)scripts/load_mock_data/main.go(3 hunks)scripts/root-project-setup/main.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure code formatting and linting pass (make fmt, make lint, make check)
Files:
internal/service/project_operations.gointernal/domain/models/project_test.gointernal/service/converters.goscripts/root-project-setup/main.gocmd/project-api/service_endpoint_project_test.goapi/project/v1/design/types.gointernal/service/converters_test.goscripts/load_mock_data/main.gointernal/service/project_operations_test.gointernal/domain/models/project.gointernal/infrastructure/nats/repository_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Place unit tests alongside implementation with the same filename and *_test.go suffix
Each production function must have exactly one corresponding TestFunction test (table with multiple cases allowed)
Use table-driven tests for coverage (subtests over a single TestFunction)
Mock all external dependencies (e.g., repositories, message builders) in unit tests
Focus unit tests on exported package functions
Files:
internal/domain/models/project_test.gocmd/project-api/service_endpoint_project_test.gointernal/service/converters_test.gointernal/service/project_operations_test.gointernal/infrastructure/nats/repository_test.go
cmd/project-api/service*.go
📄 CodeRabbit inference engine (CLAUDE.md)
cmd/project-api/service*.go: Implement Goa-generated service interfaces in cmd/project-api/service*.go
Enforce ETag concurrency via If-Match on PUT/DELETE handlers
Files:
cmd/project-api/service_endpoint_project_test.go
api/project/v1/design/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Define and update the REST API using Goa v3 in api/project/v1/design/ (Design First)
Files:
api/project/v1/design/types.go
🧬 Code graph analysis (8)
internal/domain/models/project_test.go (3)
api/project/v1/design/types.go (1)
UserInfo(226-243)internal/domain/models/project.go (1)
UserInfo(12-17)api/project/v1/gen/project_service/service.go (1)
UserInfo(431-440)
internal/service/converters.go (3)
api/project/v1/design/types.go (1)
UserInfo(226-243)internal/domain/models/project.go (1)
UserInfo(12-17)api/project/v1/gen/project_service/service.go (1)
UserInfo(431-440)
cmd/project-api/service_endpoint_project_test.go (4)
api/project/v1/design/types.go (1)
UserInfo(226-243)internal/domain/models/project.go (1)
UserInfo(12-17)api/project/v1/gen/project_service/service.go (1)
UserInfo(431-440)pkg/misc/misc.go (1)
StringPtr(7-9)
api/project/v1/design/types.go (2)
internal/domain/models/project.go (1)
UserInfo(12-17)api/project/v1/gen/project_service/service.go (1)
UserInfo(431-440)
internal/service/converters_test.go (2)
pkg/misc/misc.go (1)
StringPtr(7-9)internal/domain/models/project.go (2)
UserInfo(12-17)ProjectSettings(46-55)
scripts/load_mock_data/main.go (2)
api/project/v1/design/types.go (1)
UserInfo(226-243)internal/domain/models/project.go (1)
UserInfo(12-17)
internal/service/project_operations_test.go (2)
api/project/v1/design/types.go (1)
UserInfo(226-243)internal/domain/models/project.go (1)
UserInfo(12-17)
internal/domain/models/project.go (2)
api/project/v1/design/types.go (1)
UserInfo(226-243)api/project/v1/gen/project_service/service.go (1)
UserInfo(431-440)
🪛 YAMLlint (1.37.1)
charts/lfx-v2-project-service/templates/deployment.yaml
[error] 69-69: syntax error: expected , but found ','
(syntax)
[warning] 71-71: too few spaces after comma
(commas)
⏰ 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). (2)
- GitHub Check: Build and Test
- GitHub Check: MegaLinter
| value: "{{ .Values.rootProject.writers | join "," }}" | ||
| - name: ROOT_PROJECT_AUDITORS | ||
| value: {{ .Values.rootProject.auditors | join "," }} | ||
| value: "{{ .Values.rootProject.auditors | join "," }}" |
There was a problem hiding this comment.
Fix quoting: current template fails lint/render
Wrapping {{ .Values.rootProject.* | join "," }} in raw double quotes introduces unescaped quotes inside a YAML double-quoted scalar, triggering the yamllint syntax error (expected <block end>, but found ',') and breaking chart linting. Replace the manual quoting with Helm’s quote helper after the join so the rendered value is correctly quoted.
- value: "{{ .Values.rootProject.writers | join "," }}"
+ value: {{ .Values.rootProject.writers | join "," | quote }}
- value: "{{ .Values.rootProject.auditors | join "," }}"
+ value: {{ .Values.rootProject.auditors | join "," | quote }}📝 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.
| value: "{{ .Values.rootProject.writers | join "," }}" | |
| - name: ROOT_PROJECT_AUDITORS | |
| value: {{ .Values.rootProject.auditors | join "," }} | |
| value: "{{ .Values.rootProject.auditors | join "," }}" | |
| value: {{ .Values.rootProject.writers | join "," | quote }} | |
| - name: ROOT_PROJECT_AUDITORS | |
| value: {{ .Values.rootProject.auditors | join "," | quote }} |
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 69-69: syntax error: expected , but found ','
(syntax)
[warning] 71-71: too few spaces after comma
(commas)
🤖 Prompt for AI Agents
In charts/lfx-v2-project-service/templates/deployment.yaml around lines 69 to
71, the current template wraps the join output in raw double quotes which can
inject unescaped quotes and break YAML; replace the manual quoting by piping the
joined string into Helm's quote helper (e.g. .Values.rootProject.auditors | join
"," | quote) so the rendered scalar is safely quoted; apply the same pattern to
any other similar lines (e.g. writers) to ensure lint/render passes.
| apiUsers := make([]*projsvc.UserInfo, len(users)) | ||
| for i, user := range users { | ||
| apiUsers[i] = &projsvc.UserInfo{ | ||
| Name: &user.Name, | ||
| Email: &user.Email, | ||
| Username: &user.Username, | ||
| Avatar: &user.Avatar, | ||
| } | ||
| } | ||
| return apiUsers |
There was a problem hiding this comment.
Don’t take addresses of the range variable
Here we take &user.Name (and friends) where user is the loop variable. Because Go reuses the same variable each iteration, every pointer ends up referencing the last element—tests with multiple writers/auditors will return duplicates.
- apiUsers := make([]*projsvc.UserInfo, len(users))
- for i, user := range users {
- apiUsers[i] = &projsvc.UserInfo{
- Name: &user.Name,
- Email: &user.Email,
- Username: &user.Username,
- Avatar: &user.Avatar,
- }
- }
+ apiUsers := make([]*projsvc.UserInfo, len(users))
+ for i := range users {
+ user := users[i]
+ apiUsers[i] = &projsvc.UserInfo{
+ Name: misc.StringPtr(user.Name),
+ Email: misc.StringPtr(user.Email),
+ Username: misc.StringPtr(user.Username),
+ Avatar: misc.StringPtr(user.Avatar),
+ }
+ }📝 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.
| apiUsers := make([]*projsvc.UserInfo, len(users)) | |
| for i, user := range users { | |
| apiUsers[i] = &projsvc.UserInfo{ | |
| Name: &user.Name, | |
| Email: &user.Email, | |
| Username: &user.Username, | |
| Avatar: &user.Avatar, | |
| } | |
| } | |
| return apiUsers | |
| apiUsers := make([]*projsvc.UserInfo, len(users)) | |
| for i := range users { | |
| user := users[i] | |
| apiUsers[i] = &projsvc.UserInfo{ | |
| Name: misc.StringPtr(user.Name), | |
| Email: misc.StringPtr(user.Email), | |
| Username: misc.StringPtr(user.Username), | |
| Avatar: misc.StringPtr(user.Avatar), | |
| } | |
| } | |
| return apiUsers |
🤖 Prompt for AI Agents
internal/service/converters.go around lines 383 to 392: the loop takes addresses
of the range variable (e.g., &user.Name) which is reused each iteration causing
all pointers to reference the last element; change to take addresses of the
actual slice element (e.g., &users[i].Name, &users[i].Email, &users[i].Username,
&users[i].Avatar) or assign a local copy per-iteration (u := users[i]; use
&u.Name etc.) so each pointer refers to the correct element.
| // Structured format: username:name:email:avatar | ||
| parts := strings.Split(trimmed, ":") | ||
| user := models.UserInfo{ | ||
| Username: parts[0], | ||
| } | ||
| if len(parts) > 1 { | ||
| user.Name = parts[1] | ||
| } | ||
| if len(parts) > 2 { | ||
| user.Email = parts[2] | ||
| } | ||
| if len(parts) > 3 { | ||
| user.Avatar = parts[3] | ||
| } | ||
| users = append(users, user) |
There was a problem hiding this comment.
Structured user parsing drops avatar URLs
Using strings.Split treats every : as a separator. Avatar values like https://… are truncated to "https", so the structured env format breaks immediately. Please switch to strings.SplitN(..., 4) (and trim parts) so we preserve the full avatar URL and keep backwards compatibility.
- if strings.Contains(trimmed, ":") {
- // Structured format: username:name:email:avatar
- parts := strings.Split(trimmed, ":")
+ if strings.Contains(trimmed, ":") {
+ // Structured format: username:name:email:avatar
+ parts := strings.SplitN(trimmed, ":", 4)
user := models.UserInfo{
- Username: parts[0],
+ Username: strings.TrimSpace(parts[0]),
}
if len(parts) > 1 {
- user.Name = parts[1]
+ user.Name = strings.TrimSpace(parts[1])
}
if len(parts) > 2 {
- user.Email = parts[2]
+ user.Email = strings.TrimSpace(parts[2])
}
if len(parts) > 3 {
- user.Avatar = parts[3]
+ user.Avatar = strings.TrimSpace(parts[3])
}
users = append(users, user)📝 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.
| // Structured format: username:name:email:avatar | |
| parts := strings.Split(trimmed, ":") | |
| user := models.UserInfo{ | |
| Username: parts[0], | |
| } | |
| if len(parts) > 1 { | |
| user.Name = parts[1] | |
| } | |
| if len(parts) > 2 { | |
| user.Email = parts[2] | |
| } | |
| if len(parts) > 3 { | |
| user.Avatar = parts[3] | |
| } | |
| users = append(users, user) | |
| if strings.Contains(trimmed, ":") { | |
| // Structured format: username:name:email:avatar | |
| parts := strings.SplitN(trimmed, ":", 4) | |
| user := models.UserInfo{ | |
| Username: strings.TrimSpace(parts[0]), | |
| } | |
| if len(parts) > 1 { | |
| user.Name = strings.TrimSpace(parts[1]) | |
| } | |
| if len(parts) > 2 { | |
| user.Email = strings.TrimSpace(parts[2]) | |
| } | |
| if len(parts) > 3 { | |
| user.Avatar = strings.TrimSpace(parts[3]) | |
| } | |
| users = append(users, user) |
🤖 Prompt for AI Agents
In scripts/root-project-setup/main.go around lines 95 to 109, the current
parsing uses strings.Split which splits on every ':' and truncates avatar URLs;
change the split to use strings.SplitN(trimmed, ":", 4) and trim whitespace on
each returned part so the fourth part preserves the full avatar URL while
maintaining backwards compatibility for fewer fields.
Signed-off-by: Jordan Evans <[email protected]>
This pull request updates the project API to include richer user profile information for project auditors and writers, replacing simple username lists with detailed user objects. The changes introduce a new
UserInfotype and update CLI examples to reflect the new structure.API Type Enhancements:
UserInfotype intypes.goto represent user profiles withname,email,username, andavatarattributes.auditorsandwritersattributes in project-related DSL functions to use arrays ofUserInfoinstead of arrays of usernames, along with updated example data to match the new structure.CLI Example Updates:
cli.goto showauditorsandwritersas arrays of user objects, providing realistic sample data for each field. [1] [2] [3]