fix: added edge case fixes for consolidated api and publishing #37399
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (1)
1526-1540: Simplify redundantdoReturncalls in mock setupThe consecutive
doReturn(Mono.just(samplePageDTO))calls can be simplified to a single call since they return the same value.Apply this diff to simplify the mock setup:
- doReturn(Mono.just(samplePageDTO)) - .doReturn(Mono.just(samplePageDTO)) - .when(spyApplicationPageService) - .getPageAndMigrateDslByBranchedPageId(anyString(), anyBoolean(), anyBoolean()); + doReturn(Mono.just(samplePageDTO)) + .when(spyApplicationPageService) + .getPageAndMigrateDslByBranchedPageId(anyString(), anyBoolean(), anyBoolean());app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java (2)
2028-2029: Adjust the indentation of inline comments for clarity.The comments on lines 2028-2029 are misaligned, which may affect readability. Please align the comments properly to maintain code readability.
2031-2031: Ensure consistency in assertion messages.On line 2031, consider adding a descriptive message to the assertion to provide clearer test failure information.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/git/CommonGitServiceCETest.java(1 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java(4 hunks)
🧰 Additional context used
📓 Learnings (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (2)
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-11-12T08:11:25.416Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-11-12T08:11:25.417Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (2)
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-11-12T08:11:25.416Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
Learnt from: sumitsum
PR: appsmithorg/appsmith#29875
File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ConsolidatedAPIServiceImpl.java:0-0
Timestamp: 2024-11-12T08:11:25.417Z
Learning: Test cases for the `getConsolidatedInfoForPageLoad` method have already been added, and the refactoring of the method into smaller methods is planned for later and is being tracked in a separate GitHub issue.
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceCEImpl.java (1)
303-331: Logic for handling Git-connected applications and branches is well-implemented
The updated code correctly manages scenarios where the application is Git-connected and the branch name is either provided or absent. The conditions ensure that the appropriate application and page are retrieved based on the Git metadata and the view mode.
app/server/appsmith-server/src/test/java/com/appsmith/server/services/ce/ConsolidatedAPIServiceImplTest.java (1)
1425-1619: Tests added correctly for edge case handling
The new test method effectively covers the scenario when the page is from a feature branch and the cacheable repository returns the base application ID.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/common/CommonGitServiceCEImpl.java (1)
1490-1492: Code implementation looks good
The importArtifactInWorkspaceFromGit method is called correctly.
| .flatMap(importedArtifact -> gitArtifactHelper.publishArtifact(importedArtifact, false)) | ||
| .flatMap(publishedArtifact -> addAnalyticsForGitOperation( | ||
| AnalyticsEvents.GIT_CHECKOUT_REMOTE_BRANCH, | ||
| publishedArtifact, | ||
| Boolean.TRUE.equals( | ||
| publishedArtifact.getGitArtifactMetadata().getIsRepoPrivate()))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure error handling in reactive chain
Consider adding error handling to manage exceptions from publishArtifact and addAnalyticsForGitOperation to prevent potential unhandled errors.
| return gitArtifactHelper | ||
| .publishArtifact(importedBranchedArtifact, false) | ||
| .then(commitArtifact( | ||
| commitDTO, baseArtifact, importedBranchedArtifact, false, false) | ||
| .thenReturn(gitPullDTO)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle exceptions in reactive calls
Ensure that any errors from publishArtifact and commitArtifact are properly handled and propagated to avoid silent failures.
…ithorg#37399) ## Description 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 /ok-to-test tags="@tag.Git" ### 🔍 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/11851164290> > Commit: 3a76919 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11851164290&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` > Spec: > <hr>Fri, 15 Nov 2024 07:01:40 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 handling of Git operations, including improved branch management and artifact importation. - Expanded analytics integration for tracking changes in branch protection settings. - **Bug Fixes** - Improved error handling for various Git operations and application/page retrieval scenarios. - **Tests** - Added new test cases for Git-related functionalities and improved error handling in tests. - Enhanced test coverage for the `getConsolidatedInfoForPageLoad` method, particularly for feature branches. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
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
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11851164290
Commit: 3a76919
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 15 Nov 2024 07:01:40 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
getConsolidatedInfoForPageLoadmethod, particularly for feature branches.