-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix: generate crud queries to have dynamicBindingPathList #40833
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughA conditional block was added to the Changes
Sequence Diagram(s)sequenceDiagram
participant TemplateAction
participant CreateDBTablePageSolutionCEImpl
participant NewActionDTO
TemplateAction->>CreateDBTablePageSolutionCEImpl: cloneActionsFromTemplateApplication()
CreateDBTablePageSolutionCEImpl->>TemplateAction: Access unpublished action
alt unpublished action exists
CreateDBTablePageSolutionCEImpl->>NewActionDTO: Copy dynamicBindingPathList
end
CreateDBTablePageSolutionCEImpl-->>NewActionDTO: Return cloned action
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
🔇 Additional comments (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java (1)
640-643
: Preserve dynamicBindingPathList for cloned actions
This conditional correctly ensures thedynamicBindingPathList
from the template’s unpublished action is applied to the newActionDTO
. For safety, consider making a defensive copy (e.g.new ArrayList<>(…)
) and defaulting to an empty list if the source is null to prevent unintended side-effects.Can we add a unit or integration test to assert that cloned actions include the expected binding paths?
📜 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/solutions/ce/CreateDBTablePageSolutionCEImpl.java
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sneha122 does this PR need any tests? Other than the test everything else LGTM
@sneha122 since we are making changes to |
Description
Generate CRUD issue fixed where dynamicBindingPathList was not getting added for newly generated actions.
Steps To Reproduce (#40812) Needs to be tested on https://ee-7616.dp.appsmith.com/ as this has reactivity changes
Steps To Reproduce (#39660)
Fixes #40812
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.Datasource"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/15414265041
Commit: f638e96
Cypress dashboard.
Tags:
@tag.Datasource
Spec:
Tue, 03 Jun 2025 11:01:55 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit