chore: External contri/fix/bug 130 file picker widget able to pick a file when max no of files is defined as zero 0 14857 #34893
Conversation
…estrict user to upload file
…r-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857' into external-contri/fix/bug-130-file-picker-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857
rohan-arthur
requested review from
ApekshaBhosale and
sagar-qa007
as code owners
|
Important Review skippedAuto reviews are limited to specific labels. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe recent updates introduce error handling for the FilePicker widget, specifically addressing situations where the maximum number of files is set to zero. Changes include adding error message functionality within the widget's component and state management. Additionally, Cypress test cases were created to verify this new functionality. Changes
Assessment against linked issues
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 Configuration File (
|
github-actions
bot
added
Widgets Product
|
🔴 There's new test files in JS, please port to TS and re-trigger Cypress tests:
|
github-actions
bot
added
Bug
![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
Outside diff range, codebase verification and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.js (1)
1-6: Use constants for locators and avoid plain strings.Instead of using plain strings for locators, define constants to ensure maintainability and readability.
-const commonlocators = require("../../../../../locators/commonlocators.json"); +import commonlocators from "../../../../../locators/commonlocators.json";
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.js (1 hunks)
- app/client/src/widgets/FilePickerWidgetV2/component/index.tsx (3 hunks)
- app/client/src/widgets/FilePickerWidgetV2/constants.ts (1 hunks)
- app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx (12 hunks)
Files skipped from review due to trivial changes (1)
- app/client/src/widgets/FilePickerWidgetV2/constants.ts
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.js (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Avoid using cy.pause in code.
Use locator variables for locators and do not use plain strings.
Use data-* attributes for selectors.
Avoid Xpaths and CSS attributes.
Avoid selectors like .btn.submit or button[type=submit].
Perform logins via API withLoginFromAPI.
Avoid using it.only.
Use before, beforeEach, after, afterEach correctly.
Clean state before tests.
Use multiple assertions.
Don't treat Cypress as unit tests.
Use constants for strings in locator.
Include datasource operations inbeforehooks.
Do not use duplicate filenames.
Additional comments not posted (7)
app/client/src/widgets/FilePickerWidgetV2/component/index.tsx (3)
6-10: Styled component for error message is correct.The styled component for error message is correctly defined using
styled-components.
34-34: Error message handling is correct.The error message is correctly displayed if
props.errorMessageis present.
50-51: New propsmaxNumFilesanderrorMessageare correctly defined.The new props
maxNumFilesanderrorMessageare correctly defined and optional.app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx (4)
52-52: New state propertyerrorMessageis correctly defined.The new state property
errorMessageis correctly initialized tonull.
712-716: Error message state is correctly managed in componentDidUpdate.The error message state is correctly reset when the max number of files changes from 0 to a positive number.
803-807: Error message handling on openModal is correct.The error message is correctly set when the max number of files is 0.
827-827: Error message prop is correctly passed to the component.The error message state is correctly passed as a prop to the
FilePickerComponent.
| it("Should display an error message when clicked on filepicker when max no-of files are set to 0", () => { | ||
| EditorNavigation.SelectEntityByName("FilePicker1", EntityType.Widget); | ||
| propPane.UpdatePropertyFieldValue("Max no. of files", "1"); | ||
| const fixturePath = "cypress/fixtures/dummy.pdf"; | ||
| cy.get(commonlocators.filepickerv2).click(); | ||
| cy.get(commonlocators.filePickerInput).first().selectFile(fixturePath, { | ||
| force: true, | ||
| }); | ||
| cy.get(commonlocators.filePickerUploadButton).click(); | ||
| cy.get(commonlocators.dashboardItemName).contains("dummy.pdf"); | ||
| agHelper.GetNClick(commonlocators.filePickerRemoveButton, 0, true); | ||
| propPane.UpdatePropertyFieldValue("Max no. of files", "0"); | ||
| cy.get(commonlocators.filepickerv2).click(); | ||
| }); |
There was a problem hiding this comment.
Test case is valid but consider adding assertions to verify the error message.
The test case is valid for verifying the error message when the max number of files is set to 0. However, consider adding assertions to explicitly verify the presence of the error message.
+ cy.get(commonlocators.filePickerErrorMessage)
+ .should("exist")
+ .and("contain.text", "Cannot upload any files");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.
| it("Should display an error message when clicked on filepicker when max no-of files are set to 0", () => { | |
| EditorNavigation.SelectEntityByName("FilePicker1", EntityType.Widget); | |
| propPane.UpdatePropertyFieldValue("Max no. of files", "1"); | |
| const fixturePath = "cypress/fixtures/dummy.pdf"; | |
| cy.get(commonlocators.filepickerv2).click(); | |
| cy.get(commonlocators.filePickerInput).first().selectFile(fixturePath, { | |
| force: true, | |
| }); | |
| cy.get(commonlocators.filePickerUploadButton).click(); | |
| cy.get(commonlocators.dashboardItemName).contains("dummy.pdf"); | |
| agHelper.GetNClick(commonlocators.filePickerRemoveButton, 0, true); | |
| propPane.UpdatePropertyFieldValue("Max no. of files", "0"); | |
| cy.get(commonlocators.filepickerv2).click(); | |
| }); | |
| it("Should display an error message when clicked on filepicker when max no-of files are set to 0", () => { | |
| EditorNavigation.SelectEntityByName("FilePicker1", EntityType.Widget); | |
| propPane.UpdatePropertyFieldValue("Max no. of files", "1"); | |
| const fixturePath = "cypress/fixtures/dummy.pdf"; | |
| cy.get(commonlocators.filepickerv2).click(); | |
| cy.get(commonlocators.filePickerInput).first().selectFile(fixturePath, { | |
| force: true, | |
| }); | |
| cy.get(commonlocators.filePickerUploadButton).click(); | |
| cy.get(commonlocators.dashboardItemName).contains("dummy.pdf"); | |
| agHelper.GetNClick(commonlocators.filePickerRemoveButton, 0, true); | |
| propPane.UpdatePropertyFieldValue("Max no. of files", "0"); | |
| cy.get(commonlocators.filepickerv2).click(); | |
| cy.get(commonlocators.filePickerErrorMessage) | |
| .should("exist") | |
| .and("contain.text", "Cannot upload any files"); | |
| }); |
|
@rohan-arthur Please run cypress test. |
|
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hi @rohan-arthur , @sagar-qa007 , pls review this pr if you have enough bandwidth , due to in-activity |
|
@saiprabhu-dandanayak I'll check it out. |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Took recent pull from |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description
shadow PR for #34363
Fixes #14857
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
Tests