fix: fix validation for datasource in action import and save flow#34754
fix: fix validation for datasource in action import and save flow#34754AnaghHegde merged 2 commits intoreleasefrom
Conversation
WalkthroughThe changes enhance the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
/build-deploy-preview -skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9807396078. |
AnaghHegde
added
ok-to-test
|
@AnaghHegde Please run import export and templates tag in |
![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
Outside diff range and nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1)
[!TIP]
Codebase VerificationUpdate test case to use the new method signature
The test case in
RefactoringServiceCETest.javahas not been updated to use the newvalidateActionmethod signature with theisDryOpsparameter. Please update the test case to ensure it correctly uses the new method signature.
app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.javaat line 578Analysis chain
Line range hint
293-386: LGTM! But verify the method usage in the codebase.The addition of the new
validateActionmethod with theisDryOpsparameter is consistent with the interface's purpose. TheisDryOpsparameter is used to control the validation behavior, particularly in handling the absence of a datasource.However, ensure that all invocations of this method are updated accordingly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all invocations of the new validateAction method with the isDryOps parameter. # Test: Search for the method usage. Expect: Only occurances of the new method signature. rg --type java $'validateAction'Length of output: 2285
Script:
#!/bin/bash # Description: List lines of code where validateAction is called without the isDryOps parameter to ensure all instances are updated. # Search for validateAction invocations without the isDryOps parameter. rg --type java 'validateAction\([^,]*\)' -nLength of output: 699
Script:
#!/bin/bash # Description: Check the `RefactoringServiceCETest.java` file to verify if the test case is correctly updated to use the new method signature. # Display the relevant lines around the invocation in RefactoringServiceCETest.java rg --type java 'validateAction' -A 5 -B 5 app/server/appsmith-server/src/test/java/com/appsmith/server/refactors/ce/RefactoringServiceCETest.javaLength of output: 749
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
Additional comments not posted (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java (1)
42-43: LGTM! But verify the method usage in the codebase.The addition of the new
validateActionmethod with theisDryOpsparameter is consistent with the interface's purpose.However, ensure that all implementations and invocations of this method are updated accordingly.
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3)
290-291: LGTM!The method
validateActionhas been updated to include theisDryOpsparameter with a default value offalse.
451-453: LGTM!The method
bulkValidateAndInsertActionInRepositoryhas been updated to callvalidateAction(newAction, true), setting theisDryOpsparameter totruefor bulk validation and insertion.
459-461: LGTM!The method
bulkValidateAndUpdateActionInRepositoryhas been updated to callvalidateAction(newAction, true), setting theisDryOpsparameter totruefor bulk validation and update.
Failed server tests
|
| invalids.add(AppsmithError.NO_RESOURCE_FOUND.getMessage( | ||
| FieldName.DATASOURCE, | ||
| action.getDatasource().getId())); | ||
| if (!isDryOps) { |
There was a problem hiding this comment.
When would this validation happen in case of dryOps?
There was a problem hiding this comment.
When the datasource is attached to action, we fetch datasource from db and update the workspaceId for action.
...rc/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
Show resolved
Hide resolved
...er/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCE.java
Show resolved
Hide resolved
AnaghHegde
added
ok-to-test
AnaghHegde
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
|
Approved with deferred tackling of |
…psmithorg#34754) ## Description More details here - https://theappsmith.slack.com/archives/CTHN8GX5Y/p1720156198256419 ## Automation /ok-to-test tags="@tag.Git, @tag.Templates, @tag.ImportExport" ### 🔍 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/9809585760> > Commit: 72dc336 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9809585760&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git, @tag.Templates, @tag.ImportExport` > <hr>Fri, 05 Jul 2024 14:30:53 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 validation process for actions with the introduction of a `Dry Operations` mode, allowing for validation without persistence. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
4 participants
Description
More details here - https://theappsmith.slack.com/archives/CTHN8GX5Y/p1720156198256419
Automation
/ok-to-test tags="@tag.Git, @tag.Templates, @tag.ImportExport"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9809585760
Commit: 72dc336
Cypress dashboard.
Tags:
@tag.Git, @tag.Templates, @tag.ImportExportFri, 05 Jul 2024 14:30:53 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Dry Operationsmode, allowing for validation without persistence.