chore: Adding host as a parameter for get home page workspaces API#38392
chore: Adding host as a parameter for get home page workspaces API#38392trishaanand merged 2 commits intoreleasefrom
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/WorkspaceControllerCE.java (1)
111-114: Optional hostname parameter is well integrated.
Passing the hostname togetUserWorkspacesByRecentlyUsedOrderis a good step toward host-specific processing. In the future, consider adding logic or documentation to clarify how the hostname shapes the returned workspaces.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/controllers/ce/WorkspaceControllerCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java(1 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCE.java (1)
27-27: Nice addition of the hostname parameter.
Adding the hostname parameter expands future flexibility, enabling host-based workspace retrieval if needed.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserWorkspaceServiceCEImpl.java (1)
396-396: Parameter currently unused.
Although the hostname parameter is introduced, the logic does not reference it yet. If additional host-based filtering is planned, leave as is. Otherwise, consider either removing or implementing the parameter usage to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java (2)
250-250: Add a test scenario for non-null hostname
Currently, this test only verifies behavior for a null hostname. To fully ensure coverage of the seat-based pricing logic, consider adding a test that passes a valid hostname (e.g.,"my-deployment-host") and asserts the resulting workspace ordering.+ @Test + @WithUserDetails(value = "api_user") + public void getUserWorkspacesByRecentlyUsedOrder_withNonNullHost_shouldReturnExpectedWorkspaces() { + StepVerifier.create(userWorkspaceService.getUserWorkspacesByRecentlyUsedOrder("my-deployment-host")) + .assertNext(workspaces -> { + // Insert assertions checking workspace ordering or filtering based on hostname + }) + .verifyComplete(); + }
277-277: Consider verifying non-null hostname with recently used workspaces
Similar to the above test, ensure the logic holds when a valid hostname is provided for recently used workspaces.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserWorkspaceServiceUnitTest.java(2 hunks)
Failed server tests
|
| @GetMapping("/home") | ||
| public Mono<ResponseDTO<List<Workspace>>> workspacesForHome() { | ||
| public Mono<ResponseDTO<List<Workspace>>> workspacesForHome( | ||
| @RequestHeader(name = "Host", required = false) String hostname) { |
There was a problem hiding this comment.
Should we replace this with x-appsmith-host to make it apparent that this is custom header?
There was a problem hiding this comment.
Not a custom header.
abhvsn
left a comment
abhvsn
left a comment
There was a problem hiding this comment.
Very minor comment but feel free to reject 🙂
…ppsmithorg#38392) ## Description Helper refactor for Seat based pricing feature for recording the deployment host Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12515621363> > Commit: 481dcbc > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12515621363&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 27 Dec 2024 12:14:36 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced workspace retrieval functionality to support optional hostname parameter in user workspace requests. - **Bug Fixes** - Improved handling of user workspaces based on recently used order with the inclusion of hostname. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Helper refactor for Seat based pricing feature for recording the deployment host
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12515621363
Commit: 481dcbc
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 27 Dec 2024 12:14:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes