chore: flexible return for test payload extraction func #38443
Conversation
WalkthroughThe pull request modifies the Changes
Suggested Labels
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
Task
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/client/src/ce/utils/actionExecutionUtils.ts(4 hunks)
🔇 Additional comments (7)
app/client/src/ce/utils/actionExecutionUtils.ts (7)
12-12: Good use of the custom utility function.
Using objectKeys from @appsmith/utils here is consistent and helps keep the codebase DRY.
24-24: Implementation looks good.
The refactor using objectKeys preserves logic and avoids enumerating unexpected props.
73-73: Consistent approach with the custom utility function.
Continuing to use objectKeys for params is a clear, uniform approach.
102-108: Nice documentation addition.
Thank you for clarifying the function's purpose and parameters in the JSDoc. This will help future maintainers.
111-111: Flexible default value.
Using a parameter default of an empty string is aligned with the function’s goal of handling undefined states gracefully.
113-113: Graceful fallback.
Correctly returning the defaultValue ensures that callers receive a defined result when collectionData is missing.
119-119: Edge case handled.
Returning defaultValue when activeJSActionId or testPayload is missing is consistent with the function’s contract.
| export const getTestPayloadFromCollectionData = ( | ||
| collectionData: JSCollectionData | undefined, | ||
| defaultValue = "", |
There was a problem hiding this comment.
I didn't quite understand the reasoning behind adding this. Basically we are passing a default value to a function which it will return when the collectionData is not there or something else is missing. Can't this function return null and the caller of this function can just do getTestPayloadFromCollectionData() || defaultValue?
There was a problem hiding this comment.
The main use is in the EE side of this change. Empty string is treated as a falsy value and for my use case I want the default value to be used only if the item is undefined. While I could do that check during consumption of this functions, but since the check is already happening here I preferred to update the function definition.
Description
The
getTestPayloadFromCollectionDataextracts thetestPayloadfrom passedcollectionDataand returns an empty string if no information is present. This PR updates the function definition to allow different default value based on the function call.Fixes #37742
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12572189037
Commit: 26ea6e2
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 01 Jan 2025 16:34:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Documentation