chore: added autocommit and protected branches methods #38370
Conversation
WalkthroughThe pull request introduces enhancements to the Git service in the Appsmith server, focusing on branch protection and auto-commit functionality. New methods have been added to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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
Git Product
sondermanish
changed the title
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 (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (1)
60-61: Dependency ordering suggestion.
Consider grouping these new parameters consistently with the others for better readability, maintaining a uniform ordering (e.g., grouping Git-related helpers together).- gitAutoCommitHelper, - transactionalOperator, + transactionalOperator, + gitAutoCommitHelper,app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (1)
1855-1891: Minor typo in the log statement.
The log message "failed tp toggle auto commit" could be changed to "failed to toggle auto commit".- log.error("failed tp toggle auto commit. reason: {} is not the id of the base Artifact", baseArtifactId); + log.error("failed to toggle auto commit. reason: {} is not the id of the base Artifact", baseArtifactId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java(4 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java(2 hunks)
🔇 Additional comments (27)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceImpl.java (3)
6-6: Looks good – new import aligns with the introduced dependency.
21-21: Seamless integration with Spring’s reactive transaction support.
42-43: Constructor expansion helps incorporate auto-commit features.
If there's an existing set of integration tests for Git operations, we strongly recommend adding coverage for auto-commit logic to ensure these new dependencies work as intended.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCECompatibleImpl.java (3)
6-6: LGTM! Required imports added for new functionality.
The new imports align with the PR objectives for adding auto-commit functionality and transactional support.
Also applies to: 21-21
61-62: LGTM! Proper delegation to parent class.
The new dependencies are correctly passed to the parent class constructor, maintaining the inheritance contract.
43-44: Verify transaction boundary requirements.
The addition of TransactionalOperator suggests new transactional operations. Ensure that the transaction boundaries are properly defined in the parent class implementation.
✅ Verification successful
No transaction boundaries found - safe to proceed
Based on the codebase analysis, while TransactionalOperator is injected, there are no explicit usages found in the implementation. This suggests it's being added for future use or as part of a broader refactoring. The current changes don't introduce any transaction-related operations that would require boundary verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transactional operation usage in the parent class
ast-grep --pattern 'class CentralGitServiceCEImpl {
$$$
transactionalOperator.$$$
$$$
}'
Length of output: 96
Script:
#!/bin/bash
# Let's first check if the file exists and its content
fd "CentralGitServiceCEImpl.java" --type f --exec cat {}
# Also check for any transactional operations in the implementation
rg -A 3 "transactionalOperator"
Length of output: 65626
Script:
#!/bin/bash
# Let's check for transactional operations in the implementation
rg -A 3 "transactionalOperator\." --type java
# Also check for any transaction-related method signatures
ast-grep --pattern 'class $_ {
$$$
@Transactional
$$$
}'
Length of output: 112
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (10)
19-19: No issues found for this import.
29-29: Import statement looks good.
35-35: Import statement verified.
56-56: Import is valid.
67-67: No concerns with this import.
113-114: Fields added for auto-commit and transactional management.
Looks straightforward and consistent with dependency injection patterns.
1777-1808: Verify logic for protecting only one default branch.
This logic is strict, which may align with the requirement that only the default branch can be protected.
1810-1827: Branch protection updates and analytics call look solid.
No obvious concurrency or correctness issues observed.
1829-1853: Retrieves only the default protected branch.
Implementation is concise and appears correct.
1893-1897: Logic to retrieve auto-commit progress is straightforward.
No issues identified.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCE.java (6)
10-10: Import for AutoCommitResponseDTO is valid.
14-15: Importing java.util.List is acceptable here.
59-61: Method signature for updating protected branches.
Matches implementation policy from the implementation class.
62-63: Method signature for retrieving protected branches.
No issues noted.
64-65: Method signature for toggling auto-commit.
Appropriately typed return value and parameters.
66-67: Method signature for fetching auto-commit progress.
Matches the associated implementation.
app/server/appsmith-server/src/main/java/com/appsmith/server/git/utils/GitAnalyticsUtils.java (5)
16-16: Import statement is fine.
19-19: No issues with the added import.
21-21: Import confirmed.
24-25: Static imports of analytics events.
Fits conventional usage.
154-189: sendBranchProtectionAnalytics method is well-structured.
Efficient approach to send events for added/removed protected branches.
| CollectionUtils.isEmpty(branchNames) ? new ArrayList<>() : branchNames; | ||
|
|
||
| // user cannot protect multiple branches | ||
| if (incomingProtectedBranches.size() > 1) { |
There was a problem hiding this comment.
QQ: Since user cannot protect multiple branches what's the reason behind being it a list?
There was a problem hiding this comment.
When we select the list from the UI, we are essentially sending multiple protected branches, hence this is take as a list. This api request is stateless, what this essentially means is that your request is the absolute value which is going to be saved.
| GitArtifactMetadata baseGitMetadata = baseArtifact.getGitArtifactMetadata(); | ||
| if (!baseArtifact.getId().equals(baseGitMetadata.getDefaultArtifactId())) { | ||
| log.error( | ||
| "failed tp toggle auto commit. reason: {} is not the id of the base Artifact", |
There was a problem hiding this comment.
Can I get this done in the subsequent PR (Gonna raise it today only), reason for this is an extra run for a string. Let me know if you think otherwise
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1869-1871: Fix error message formatting.The error message should start with a capital letter and end with a period.
- "failed to toggle auto commit. reason: {} is not the id of the base Artifact", + "Failed to toggle auto commit. Reason: {} is not the id of the base Artifact.",
1855-1891: Consider adding null check for AutoCommitConfig.The code assumes AutoCommitConfig is never null when accessing it. Add a null check or ensure it's initialized with default values.
- AutoCommitConfig autoCommitConfig = baseGitMetadata.getAutoCommitConfig(); + AutoCommitConfig autoCommitConfig = baseGitMetadata.getAutoCommitConfig(); + if (autoCommitConfig == null) { + autoCommitConfig = new AutoCommitConfig(); + autoCommitConfig.setEnabled(FALSE); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java(7 hunks)
🔇 Additional comments (2)
app/server/appsmith-server/src/main/java/com/appsmith/server/git/central/CentralGitServiceCEImpl.java (2)
1776-1808: LGTM! The protected branches validation logic is well-implemented.
The method correctly enforces business rules:
- Only one branch can be protected
- Only the default branch can be protected
1893-1897: LGTM! Clean delegation to helper.
The method correctly delegates to the specialized helper class.
Description
Fixes #37445, #37446 , #37447, #37457, #37458
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/12513470547
Commit: 20cae3e
Cypress dashboard.
Tags:
@tag.GitSpec:
Fri, 27 Dec 2024 08:43:08 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation