-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
chore: Rahulbarwal/hotfix-v1.53/select-widget-handling #38259
chore: Rahulbarwal/hotfix-v1.53/select-widget-handling #38259
Conversation
07/10 - Daily promotion
08/10 Daily Promotion
10/10 Daily Promotion
11/10 - Daily Promotion
14/10 Daily Promotion
17/10 Daily Promotion
18/10 Daily Promotion
21/10 Daily Promotion
23/10 Daily Promotion
24/10 Daily Promotion
25/10 Daily Promotion
28/10 Daily Promotion
30/10 Daily Promotion
05/11 Daily Promotion
06/11 Daily Promotion
11/11 Daily Promotion
12/11 Daily Promotion
15/11/24 Daily Promotion
18/11 Daily Promotion
19/11 - Daily promotion
21/11 Daily Promotion
22/11 Daily Promotion
25/11 - Daily promotion
Fixes issue where the detection for signup when using OAuth was not being handled correctly. [Slack conversation](https://theappsmith.slack.com/archives/C02K2MZERSL/p1732600773587469?thread_ts=1732554015.110689&cid=C02K2MZERSL). ## Automation /test sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12024883331> > Commit: d53fcdf > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12024883331&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: @tag.Sanity > Spec: > It seems like **no tests ran** 😔. We are not able to recognize it, please check <a href="https://github.com/appsmithorg/appsmith/actions/runs/12024883331" target="_blank">workflow here</a>. > <hr>Tue, 26 Nov 2024 06:16:02 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Improved handling of user authentication success, enhancing the flow for email verification and OAuth2 authentication. - **Refactor** - Simplified the logic for determining user sign-up or login status. - Streamlined the method for handling OAuth2 redirects, improving clarity and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This PR hides shared drive support for Google Sheets Integration behind a feature flag as we need to do thorough testing for it. The feature flag implementation done in this PR involves passing feature flags from server to google sheets plugin module. This temporary feature flagging solution tech debt can be removed once the testing is done and once we would release this feature to all. **Why feature flags are only available at server module?** Because they have a dependency on UserIdentifierService, SessionUserService, TenantService etc which exists at server module. Supporting feature flags out of the box at plugin module level would require significant efforts to migrate these dependencies. Fixes #37714 _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ /ok-to-test tags="@tag.Authentication, @tag.Datasource" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12063930619> > Commit: 47f08f9 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12063930619&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Authentication, @tag.Datasource` > Spec: > <hr>Thu, 28 Nov 2024 07:38:58 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **New Features** - Introduced support for Google Sheets shared drive through feature flags. - Added new methods to handle feature flags in plugin execution and triggering processes. - Enhanced action execution and triggering logic to utilize feature flags for dynamic behavior. - **Bug Fixes** - Improved error handling for plugin execution processes. - **Tests** - Integrated `FeatureFlagService` into the testing framework for enhanced test coverage. - Expanded test scenarios to include various response types and error conditions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: “sneha122” <“[email protected]”>
Failing server unit test fixed, This test started failing due to changes in this PR: #37776, for some reason it did not show up on this Pr in failed test cases 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._ /ok-to-test tags="@tag.IconButton" <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/12066311357> > Commit: f915b4f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12066311357&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IconButton` > Spec: > <hr>Thu, 28 Nov 2024 09:59:28 UTC <!-- end of auto-generated comment: Cypress test results --> Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **Bug Fixes** - Improved error handling for stale connections during action execution, allowing the system to recover and continue executing actions after an initial failure. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: “sneha122” <“[email protected]”>
…d-drive chore: cherry pick gs shared drive
29/11 Daily Promotion
02/12 Daily Promotion
…37909) ## Description Fixing the Broken UI on JS module instance in the side by side view mode. Fixes # ## Automation /ok-to-test tags="@tag.IDE" ### 🔍 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/12135280169> > Commit: bc011f0 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12135280169&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.IDE` > Spec: > <hr>Tue, 03 Dec 2024 08:32:26 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 flexibility of the dropdown selector in the JS Function Run component by removing the minimum width constraint. - **Bug Fixes** - Adjusted layout to improve the visual adaptability of the dropdown selector based on its content. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…e-instance-fix fix: Broken UI on JS module instance in the side by side view mode (#37909)
## Description - For usage pulse we need pageId - URL contains basePageId - basePageId needs to be converted to pageId - The conversion needs application data to be populated into redux - The PR adds a wait for application to initialise first before sending the usage pulse Fixes [37904](#37904) ## Automation /ok-to-test tags="@tag.All" ### 🔍 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/12157463790> > Commit: b4c4df1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12157463790&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Wed, 04 Dec 2024 11:40:43 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 user navigation tracking with improved handling of route changes. - Introduced a new function for managing user activity tracking, improving clarity and control flow. - **Bug Fixes** - Improved error handling during route changes to ensure proper logging. - **Refactor** - Restructured user-related sagas for better organization and separation of concerns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fix: fixing usage pulse for anon user (#37940)
06/12 Daily Promotion
09/12 Daily Promotion
… pulse to avoid overwriting default fields. (#38030) … ## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ 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 /test all ### 🔍 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/12229443145> > Commit: 9f3eebb > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12229443145&attempt=4" target="_blank">Cypress dashboard</a>. > Tags: `@tag.All` > Spec: > <hr>Mon, 09 Dec 2024 08:27:39 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 ## Release Notes - **New Features** - Introduced methods to update user records directly by ID, enhancing user management capabilities. - Added functionality to update user information without permission checks for administrative purposes. - **Bug Fixes** - Improved error handling for user updates, ensuring robustness in user management operations. - **Documentation** - Updated documentation to reflect new methods and their functionalities in user services. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
fix: Only updating the required fields in User while generating usage…
11/12 Daily Promotion
chore: Cherry picking fix for client build failure
12/12 Daily Promotion
13/12 Daily Promotion
16/12 Daily Promotion
WalkthroughThe pull request introduces improvements to the SelectWidget's option processing logic. A new function Changes
Sequence DiagramsequenceDiagram
participant Widget as SelectWidget
participant Util as PropertyUtils
participant Source as SourceData
Widget->>Util: Validate label/value keys
Util->>Source: Check source data
Util-->>Widget: Return processed options
Possibly related PRs
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
|
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: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/client/src/widgets/SelectWidget/widget/derived.js
(1 hunks)app/client/src/widgets/SelectWidget/widget/propertyUtils.ts
(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/widgets/SelectWidget/widget/propertyUtils.ts
[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 250-250: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (1)
app/client/src/widgets/SelectWidget/widget/derived.js (1)
45-45
: LGTM: Proper implementation of processOptionArray
The function is correctly used for both label and value processing.
Also applies to: 51-51
const processOptionArray = (optionArray, sourceData) => { | ||
if (!sourceData.length) return []; | ||
|
||
const allEqual = optionArray.every((item, _, arr) => item === arr[0]); | ||
const keyExistsInSource = optionArray[0] in sourceData[0]; | ||
|
||
return allEqual && keyExistsInSource | ||
? sourceData.map((d) => d[optionArray[0]]) | ||
: optionArray; | ||
}; |
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.
Add null checks and safer property access
The function needs defensive programming for edge cases.
Apply these improvements:
const processOptionArray = (optionArray, sourceData) => {
- if (!sourceData.length) return [];
+ if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) return [];
const allEqual = optionArray.every((item, _, arr) => item === arr[0]);
- const keyExistsInSource = optionArray[0] in sourceData[0];
+ const keyExistsInSource = Object.hasOwn(sourceData[0], String(optionArray[0]));
return allEqual && keyExistsInSource
? sourceData.map((d) => d[optionArray[0]])
: optionArray;
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const processOptionArray = (optionArray, sourceData) => { | |
if (!sourceData.length) return []; | |
const allEqual = optionArray.every((item, _, arr) => item === arr[0]); | |
const keyExistsInSource = optionArray[0] in sourceData[0]; | |
return allEqual && keyExistsInSource | |
? sourceData.map((d) => d[optionArray[0]]) | |
: optionArray; | |
}; | |
const processOptionArray = (optionArray, sourceData) => { | |
if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) return []; | |
const allEqual = optionArray.every((item, _, arr) => item === arr[0]); | |
const keyExistsInSource = Object.hasOwn(sourceData[0], String(optionArray[0])); | |
return allEqual && keyExistsInSource | |
? sourceData.map((d) => d[optionArray[0]]) | |
: optionArray; | |
}; |
/* | ||
* Here assumption is that if evaluated array is all equal, then it is a key, | ||
* and we can return the parsed value(from source data) as the options. | ||
*/ | ||
const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]); | ||
|
||
if ( | ||
areAllValuesEqual && | ||
props.sourceData[0].hasOwnProperty(String(value[0])) | ||
) { | ||
const parsedValue = props.sourceData.map( | ||
(d: Record<string, unknown>) => d[String(value[0])], | ||
); | ||
|
||
return { | ||
parsed: parsedValue, | ||
isValid: true, | ||
messages: [], | ||
}; | ||
} |
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.
🛠️ Refactor suggestion
Remove duplicated validation logic
This block duplicates the logic from labelKeyValidation.
Use the suggested validateSourceDataKey
helper function here as well.
🧰 Tools
🪛 Biome (1.9.4)
[error] 250-250: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
|
||
if ( | ||
areAllValuesEqual && | ||
props.sourceData[0].hasOwnProperty(String(value[0])) |
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.
Replace hasOwnProperty with Object.hasOwn
Direct hasOwnProperty calls are unsafe.
Use Object.hasOwn()
instead of the prototype method call.
Also applies to: 250-250
🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
/* | ||
* Here assumption is that if evaluated array is all equal, then it is a key, | ||
* and we can return the parsed value(from source data) as the options. | ||
*/ | ||
const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]); | ||
|
||
if ( | ||
areAllValuesEqual && | ||
props.sourceData[0].hasOwnProperty(String(value[0])) | ||
) { | ||
const parsedValue = props.sourceData.map( | ||
(d: Record<string, unknown>) => d[String(value[0])], | ||
); | ||
|
||
return { | ||
parsed: parsedValue, | ||
isValid: true, | ||
messages: [], | ||
}; | ||
} |
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.
🛠️ Refactor suggestion
Refactor validation logic for better safety and reusability
The validation logic needs improvement in error handling and property access.
Consider extracting the common logic and adding safety checks:
+ const validateSourceDataKey = (value: unknown[], sourceData: unknown[]) => {
+ if (!Array.isArray(sourceData) || !sourceData.length || !sourceData[0]) {
+ return { isValid: false, parsedValue: [] };
+ }
+
+ const areAllValuesEqual = value.every((item, _, arr) => item === arr[0]);
+ const firstValue = String(value[0]);
+
+ if (areAllValuesEqual && Object.hasOwn(sourceData[0], firstValue)) {
+ return {
+ isValid: true,
+ parsedValue: sourceData.map((d: Record<string, unknown>) => d[firstValue]),
+ };
+ }
+
+ return { isValid: false, parsedValue: [] };
+ };
Then use this helper in both validation functions.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 149-149: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
This PR has been closed because of inactivity. |
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
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=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes