Add Dashboard Config API for managing dashboard configurations#1
Merged
Conversation
Implement DashboardConfigService and its implementation, DashboardConfigServiceImpl, to handle retrieval and storage of dashboard configurations. Create DashboardConfigWebscript to expose the API for getting and saving configurations via web requests. Update service and webscript contexts for dependency injection. Add XML descriptor for webscript and unit tests for the webscript functionality.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request adds dashboard configuration management: a new service interface and Alfresco-backed implementation, a web script REST endpoint for get/save actions, Spring bean wiring, a webscript descriptor, and unit tests for the webscript behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as HTTP Client
participant WebScript as DashboardConfigWebscript
participant Service as DashboardConfigServiceImpl
participant SiteService as SiteService
participant FileFolderService as FileFolderService
participant ContentService as ContentService
participant FileSystem as AlfrescoFS
rect rgba(100,150,200,0.5)
Note over Client,WebScript: GET flow
Client->>WebScript: POST {action: "get", site, slug}
WebScript->>Service: getConfig(site, slug)
Service->>SiteService: getSite(site)
SiteService-->>Service: site info
Service->>FileFolderService: getContainer(site, DOCUMENT_LIBRARY)
FileFolderService-->>Service: docLib NodeRef
Service->>FileFolderService: searchSimple(docLib, "dashboard")
alt dashboard folder exists
FileFolderService-->>Service: dashboard NodeRef
else missing
Service-->>WebScript: null
WebScript-->>Client: {data: null}
end
Service->>FileFolderService: searchSimple(dashboard, filename)
FileFolderService-->>Service: file NodeRef
Service->>ContentService: getReader(file)
ContentService->>FileSystem: read JSON
FileSystem-->>ContentService: JSON
ContentService-->>Service: content string
Service-->>WebScript: config JSON
WebScript-->>Client: {data: config}
end
sequenceDiagram
participant Client as HTTP Client
participant WebScript as DashboardConfigWebscript
participant Service as DashboardConfigServiceImpl
participant SiteService as SiteService
participant FileFolderService as FileFolderService
participant ContentService as ContentService
participant FileSystem as AlfrescoFS
rect rgba(150,200,100,0.5)
Note over Client,WebScript: SAVE flow
Client->>WebScript: POST {action: "save", site, slug, config}
WebScript->>Service: saveConfig(site, slug, configJson)
Service->>SiteService: getSite(site)
SiteService-->>Service: site info
Service->>FileFolderService: getContainer(site, DOCUMENT_LIBRARY)
FileFolderService-->>Service: docLib NodeRef
Service->>FileFolderService: getOrCreateFolder(docLib, "dashboard")
FileFolderService-->>Service: dashboard NodeRef
Service->>FileFolderService: getOrCreateFile(dashboard, filename)
FileFolderService-->>Service: file NodeRef
Service->>ContentService: getWriter(file)
ContentService->>FileSystem: write JSON (application/json, UTF-8)
FileSystem-->>ContentService: write complete
ContentService-->>Service: success
Service-->>WebScript: void
WebScript-->>Client: {success: true}
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 1
🧹 Nitpick comments (3)
src/test/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscriptTest.java (1)
42-101: Consider adding test coverage for error handling paths.The current tests cover the happy paths well. However, the webscript has several error-handling branches that aren't tested:
- Missing required parameters (site/slug) → 400 response
IllegalStateExceptionfrom service → 404 response- General exceptions → 500 response
Would you like me to generate additional test cases for these error scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscriptTest.java` around lines 42 - 101, Add unit tests in DashboardConfigWebscriptTest to cover the error branches: create a test for missing required params by setting req.getContent().getContent() to JSON missing "site" or "slug" and assert webscript.execute(req,res) results in res.setStatus(400), response contains "error", and that service.getConfig/saveConfig is not called; create a test where service.getConfig (or saveConfig for save action) is mocked to throw new IllegalStateException(...) and assert webscript.execute sets res.setStatus(404) and response contains the exception message; and create a test where the service throws a generic RuntimeException and assert webscript.execute sets res.setStatus(500) and response contains "error" — use existing helpers/objects (Match, req, res, responseWriter, service, webscript) and verify interactions with service and response status for each case.src/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigServiceImpl.java (2)
18-18: Unused constant.
MIMETYPE_JSONis defined but not used. Line 83 uses a hardcoded"application/json"string instead.Use the constant consistently
var writer = contentService.getWriter(configFile, ContentModel.PROP_CONTENT, true); - writer.setMimetype("application/json"); + writer.setMimetype(MIMETYPE_JSON); writer.setEncoding("UTF-8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigServiceImpl.java` at line 18, The MIMETYPE_JSON constant in DashboardConfigServiceImpl is defined but not used; replace the hardcoded "application/json" occurrences (e.g., the usage at the method around the current line that sets the response/content type) with the MIMETYPE_JSON constant to use it consistently, and if after replacing all occurrences the constant ends up unused elsewhere remove the now-unnecessary constant declaration.
107-109: Add slug validation before constructing filename.The
slugparameter is used directly to construct filenames without validation. Alfresco'sFileFolderServiceenforces acm:filenameconstraint that rejects characters like/,\,:,*,",<,>,|,?, and trailing spaces. If the slug contains these characters,FileFolderService.create()andsearchSimple()will fail with a constraint violation.Add validation to reject or sanitize invalid characters before calling
toFileName(), or implement pre-sanitization in the method itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigServiceImpl.java` around lines 107 - 109, The toFileName method currently concatenates the slug into a filename without validation; update DashboardConfigServiceImpl.toFileName to either validate the incoming slug and throw a clear IllegalArgumentException when it contains any forbidden cm:filename characters (/ \ : * " < > | ? or trailing spaces) or to sanitize the slug by removing or replacing those characters and trimming trailing spaces before appending "-config.json"; ensure you perform this check/sanitization wherever toFileName is called (or centralize it inside toFileName) so FileFolderService.create() and searchSimple() receive only valid filenames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscript.java`:
- Around line 85-87: The error thrown in DashboardConfigWebscript.java currently
says "Missing required fields: site, slug, and config" even though site and slug
were already validated; update the WebScriptException thrown in the block that
checks body.has("config") to only mention the missing "config" field (e.g.,
"Missing required field: config") so the exception message accurately reflects
the validation that failed in the method where the check occurs.
---
Nitpick comments:
In
`@src/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigServiceImpl.java`:
- Line 18: The MIMETYPE_JSON constant in DashboardConfigServiceImpl is defined
but not used; replace the hardcoded "application/json" occurrences (e.g., the
usage at the method around the current line that sets the response/content type)
with the MIMETYPE_JSON constant to use it consistently, and if after replacing
all occurrences the constant ends up unused elsewhere remove the now-unnecessary
constant declaration.
- Around line 107-109: The toFileName method currently concatenates the slug
into a filename without validation; update DashboardConfigServiceImpl.toFileName
to either validate the incoming slug and throw a clear IllegalArgumentException
when it contains any forbidden cm:filename characters (/ \ : * " < > | ? or
trailing spaces) or to sanitize the slug by removing or replacing those
characters and trimming trailing spaces before appending "-config.json"; ensure
you perform this check/sanitization wherever toFileName is called (or centralize
it inside toFileName) so FileFolderService.create() and searchSimple() receive
only valid filenames.
In
`@src/test/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscriptTest.java`:
- Around line 42-101: Add unit tests in DashboardConfigWebscriptTest to cover
the error branches: create a test for missing required params by setting
req.getContent().getContent() to JSON missing "site" or "slug" and assert
webscript.execute(req,res) results in res.setStatus(400), response contains
"error", and that service.getConfig/saveConfig is not called; create a test
where service.getConfig (or saveConfig for save action) is mocked to throw new
IllegalStateException(...) and assert webscript.execute sets res.setStatus(404)
and response contains the exception message; and create a test where the service
throws a generic RuntimeException and assert webscript.execute sets
res.setStatus(500) and response contains "error" — use existing helpers/objects
(Match, req, res, responseWriter, service, webscript) and verify interactions
with service and response status for each case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62ae9600-2284-483a-821d-1f94cf51c889
📒 Files selected for processing (7)
src/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigService.javasrc/main/java/com/microboxlabs/dashboards/dashboard/service/DashboardConfigServiceImpl.javasrc/main/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscript.javasrc/main/resources/alfresco/extension/templates/webscripts/microboxlabs/dashboards/dashboard-config/api.post.desc.xmlsrc/main/resources/alfresco/module/miot-dashboards/context/service-context.xmlsrc/main/resources/alfresco/module/miot-dashboards/context/webscript-context.xmlsrc/test/java/com/microboxlabs/dashboards/dashboard/webscript/DashboardConfigWebscriptTest.java
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.
Implement DashboardConfigService and its implementation, DashboardConfigServiceImpl, to handle retrieval and storage of dashboard configurations. Create DashboardConfigWebscript to expose the API for getting and saving configurations via web requests. Update service and webscript contexts for dependency injection. Add XML descriptor for webscript and unit tests for the webscript functionality.
Summary by CodeRabbit
New Features
Tests
Chores