Match API response format to chrome service#26
Merged
Hyperkid123 merged 2 commits intoRedHatInsights:masterfrom Aug 5, 2025
Merged
Match API response format to chrome service#26Hyperkid123 merged 2 commits intoRedHatInsights:masterfrom
Hyperkid123 merged 2 commits intoRedHatInsights:masterfrom
Conversation
Reviewer's GuideThis PR aligns API list responses with the Chrome service format by wrapping list endpoints in a data/meta structure, implements dashboardType filtering with auto-creation of missing templates, updates OpenAPI and server handlers accordingly, introduces a Permission schema for widget mappings, and adds a detailed widget migration guide. Sequence diagram for GET / with dashboardType filtering and auto-creationsequenceDiagram
actor User
participant API as API Server
participant Service as Service Layer
participant DB as Database
User->>API: GET /?dashboardType=typeX
API->>Service: GetUserTemplates(userId, dashboardType=typeX)
Service->>DB: Query templates for userId and typeX
alt No templates found
Service->>Service: ForkBaseTemplate(typeX, userId)
Service->>Service: ChangeDefaultTemplate(newTemplateId, userId)
Service-->>API: [newTemplate], 404
else Templates found
Service-->>API: [templates], 200
end
API->>User: { data: [...], meta: { count }, status }
Class diagram for updated API response typesclassDiagram
class DashboardTemplateListResponse {
+DashboardTemplate[] Data
+ListResponseMeta Meta
}
class ListResponseMeta {
+int Count
}
class BaseWidgetDashboardTemplateListResponse {
+BaseWidgetDashboardTemplate[] Data
+ListResponseMeta Meta
}
class WidgetMappingResponse {
+map<string, WidgetModuleFederationMetadata> Data
}
DashboardTemplateListResponse --> ListResponseMeta
BaseWidgetDashboardTemplateListResponse --> ListResponseMeta
WidgetMappingResponse --> WidgetModuleFederationMetadata
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Hyperkid123 - I've reviewed your changes - here's some feedback:
- Consider extracting the common list‐response wrapping logic (i.e. building {data, meta}) into a shared helper to avoid duplication across GetWidgetLayout, GetBaseWidgetDashboardTemplates, and GetWidgetMapping.
- The service now returns 404 when auto‐creating a template—please double‐check that this non-standard use of 404 aligns with your API design and consider returning 201 or 200 with a flag instead to prevent client confusion.
- Tests write directly to the shared database without explicit cleanup; consider wrapping each test in a transaction or truncating relevant tables to guarantee isolation and avoid flaky failures.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the common list‐response wrapping logic (i.e. building {data, meta}) into a shared helper to avoid duplication across GetWidgetLayout, GetBaseWidgetDashboardTemplates, and GetWidgetMapping.
- The service now returns 404 when auto‐creating a template—please double‐check that this non-standard use of 404 aligns with your API design and consider returning 201 or 200 with a flag instead to prevent client confusion.
- Tests write directly to the shared database without explicit cleanup; consider wrapping each test in a transaction or truncating relevant tables to guarantee isolation and avoid flaky failures.
## Individual Comments
### Comment 1
<location> `pkg/service/DashboardTemplate.go:46` </location>
<code_context>
+func GetUserTemplates(id identity.XRHID, params api.GetWidgetLayoutParams) ([]api.DashboardTemplate, int, error) {
var templates []api.DashboardTemplate
- err := database.DB.Where(api.DashboardTemplate{UserId: id.Identity.User.UserID}).Find(&templates).Error
+ where := api.DashboardTemplate{UserId: id.Identity.User.UserID}
+ if params.DashboardType != nil {
+ where.TemplateBase.Name = *params.DashboardType
+ }
</code_context>
<issue_to_address>
Potentially incorrect use of struct for dynamic filtering.
Struct-based Where may not filter nested fields like TemplateBase.Name. Use map[string]interface{} or chain Where calls for accurate filtering.
</issue_to_address>
### Comment 2
<location> `pkg/service/DashboardTemplate.go:44` </location>
<code_context>
}
-func GetUserTemplates(id identity.XRHID) ([]api.DashboardTemplate, int, error) {
+func GetUserTemplates(id identity.XRHID, params api.GetWidgetLayoutParams) ([]api.DashboardTemplate, int, error) {
var templates []api.DashboardTemplate
- err := database.DB.Where(api.DashboardTemplate{UserId: id.Identity.User.UserID}).Find(&templates).Error
</code_context>
<issue_to_address>
Consider extracting the 'fork and set default' logic into a helper function to keep GetUserTemplates focused on querying.
Here’s one way to flatten out `GetUserTemplates` by moving the “fork & default” path into its own helper. You don’t change any behavior, you just keep the main function focused on querying:
```go
// new helper for the "no rows + type" case
func getOrCreateTemplatesForType(
userID string,
dashboardType string,
) ([]api.DashboardTemplate, int, error) {
// fork
tmpl, status, err := ForkBaseTemplate(dashboardType, id)
if err != nil {
logrus.Errorf("fork failed for %s/%s: %v", userID, dashboardType, err)
return nil, status, err
}
// set default
tmpl, status, err = ChangeDefaultTemplate(int64(tmpl.ID), id)
if err != nil {
logrus.Errorf("set-default failed for %s/%s: %v", userID, dashboardType, err)
return nil, status, err
}
return []api.DashboardTemplate{tmpl}, http.StatusNotFound, nil
}
```
Then simplify `GetUserTemplates`:
```go
func GetUserTemplates(id identity.XRHID, params api.GetWidgetLayoutParams) ([]api.DashboardTemplate, int, error) {
var templates []api.DashboardTemplate
where := api.DashboardTemplate{UserId: id.Identity.User.UserID}
if params.DashboardType != nil {
where.TemplateBase.Name = *params.DashboardType
}
res := database.DB.Where(where).Find(&templates)
// delegate the zero‐row & type‐provided case
if res.Error == nil && res.RowsAffected == 0 && params.DashboardType != nil {
return getOrCreateTemplatesForType(id.Identity.User.UserID, *params.DashboardType)
}
if _, status, err := handleServiceError(
res.Error,
fmt.Sprintf("No dashboard templates found for user %s", id.Identity.User.UserID),
"Failed to retrieve dashboard templates for user %s: %v", http.StatusNotFound,
nil, []api.DashboardTemplate{},
); err != nil {
return nil, status, err
}
return templates, http.StatusOK, nil
}
```
This pulls the multi-step “fork & default” logic out of the main flow and reduces the cyclomatic branches in `GetUserTemplates`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+46
to
+47
| where := api.DashboardTemplate{UserId: id.Identity.User.UserID} | ||
| if params.DashboardType != nil { |
There was a problem hiding this comment.
issue (bug_risk): Potentially incorrect use of struct for dynamic filtering.
Struct-based Where may not filter nested fields like TemplateBase.Name. Use map[string]interface{} or chain Where calls for accurate filtering.
| } | ||
|
|
||
| func GetUserTemplates(id identity.XRHID) ([]api.DashboardTemplate, int, error) { | ||
| func GetUserTemplates(id identity.XRHID, params api.GetWidgetLayoutParams) ([]api.DashboardTemplate, int, error) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting the 'fork and set default' logic into a helper function to keep GetUserTemplates focused on querying.
Here’s one way to flatten out GetUserTemplates by moving the “fork & default” path into its own helper. You don’t change any behavior, you just keep the main function focused on querying:
// new helper for the "no rows + type" case
func getOrCreateTemplatesForType(
userID string,
dashboardType string,
) ([]api.DashboardTemplate, int, error) {
// fork
tmpl, status, err := ForkBaseTemplate(dashboardType, id)
if err != nil {
logrus.Errorf("fork failed for %s/%s: %v", userID, dashboardType, err)
return nil, status, err
}
// set default
tmpl, status, err = ChangeDefaultTemplate(int64(tmpl.ID), id)
if err != nil {
logrus.Errorf("set-default failed for %s/%s: %v", userID, dashboardType, err)
return nil, status, err
}
return []api.DashboardTemplate{tmpl}, http.StatusNotFound, nil
}Then simplify GetUserTemplates:
func GetUserTemplates(id identity.XRHID, params api.GetWidgetLayoutParams) ([]api.DashboardTemplate, int, error) {
var templates []api.DashboardTemplate
where := api.DashboardTemplate{UserId: id.Identity.User.UserID}
if params.DashboardType != nil {
where.TemplateBase.Name = *params.DashboardType
}
res := database.DB.Where(where).Find(&templates)
// delegate the zero‐row & type‐provided case
if res.Error == nil && res.RowsAffected == 0 && params.DashboardType != nil {
return getOrCreateTemplatesForType(id.Identity.User.UserID, *params.DashboardType)
}
if _, status, err := handleServiceError(
res.Error,
fmt.Sprintf("No dashboard templates found for user %s", id.Identity.User.UserID),
"Failed to retrieve dashboard templates for user %s: %v", http.StatusNotFound,
nil, []api.DashboardTemplate{},
); err != nil {
return nil, status, err
}
return templates, http.StatusOK, nil
}This pulls the multi-step “fork & default” logic out of the main flow and reduces the cyclomatic branches in GetUserTemplates.
Hyperkid123
force-pushed
the
read-feo-widgets
branch
from
a89b4a5 to
a7c9087
Compare
Contributor
Author
|
The tekton job will not work until we are onboarded |
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
2 participants
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.
This will ensure easy frontend migration.
Migration example is in this draft PR: RedHatInsights/widget-layout#180
Summary by Sourcery
Align list endpoints with Chrome service API by enveloping responses in a consistent data/meta format, adding dashboardType filtering with auto-creation behavior, and updating service, server, specs, tests, and documentation accordingly.
New Features:
Enhancements:
Documentation:
Tests: