fix: ensure an error msg is displayed when max-no of files is 0 and r… #34363
Conversation
…estrict user to upload file
WalkthroughThe recent changes introduce enhancements to the FilePicker widget's functionality and testing. They include the addition of error message handling when the maximum number of files allowed is set to 0. This update ensures proper display of error messages through a new prop, styled components, and constants. Corresponding Cypress tests were added to verify this behavior. 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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 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
Biome
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.js
[error] 15-34: This function expression can be turned into an arrow function. (lint/complexity/useArrowFunction)
Function expressions that don't use this can be turned into arrow functions.
Safe fix: Use an arrow function instead.app/client/src/widgets/FilePickerWidgetV2/component/index.tsx
[error] 15-15: Change to an optional chain. (lint/complexity/useOptionalChain)
Unsafe fix: Change to an optional chain.
Additional comments not posted (5)
app/client/src/widgets/FilePickerWidgetV2/component/index.tsx (2)
50-51: Adding optional propertiesmaxNumFilesanderrorMessageto the interface is appropriate for the new functionality.
8-10: Ensure theErrorMessagestyled component's color is accessible against various backgrounds.app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx (3)
803-807: The logic to handle themaxNumFilesproperty and set the error message is clear and correctly implemented.
842-842: The stateerrorMessageis well-defined and appropriately used to manage error messages based on file upload conditions.
21-21: Ensure that theERROR_MESSAGEconstant anderrorMessagestate are consistently used across the widget to manage error visibility effectively.Also applies to: 52-52
Verification successful
Ensure that the
ERROR_MESSAGEconstant anderrorMessagestate are consistently used across the widget to manage error visibility effectively.The search results indicate that both
ERROR_MESSAGEanderrorMessageare used in various files, includingapp/client/src/widgets/FilePickerWidgetV2/widget/index.tsx. However, to confirm consistent usage within this specific widget, we need to focus on the relevant lines of code.
app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx:import FileDataTypes, { ERROR_MESSAGE } from "../constants";app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: errorMessage: null,app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: this.setState({ errorMessage: null });app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: this.setState({ errorMessage: ERROR_MESSAGE });app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: this.setState({ errorMessage: null });app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: errorMessage={this.state.errorMessage}app/client/src/widgets/FilePickerWidgetV2/widget/index.tsx: errorMessage: string | null;These lines show that
ERROR_MESSAGEanderrorMessageare indeed being used to manage error states within theFilePickerWidgetV2. The usage appears consistent, witherrorMessagebeing set and reset appropriately and rendered as needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistent use of `ERROR_MESSAGE` and `errorMessage` across the widget. # Command to check usage consistency rg --type tsx 'ERROR_MESSAGE|errorMessage'Length of output: 72
Script:
#!/bin/bash # Description: Verify consistent use of `ERROR_MESSAGE` and `errorMessage` across the widget. # Corrected command to check usage consistency rg 'ERROR_MESSAGE|errorMessage' --glob '**/*.tsx'Length of output: 29887
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.js
Outdated
Show resolved
Hide resolved
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Hi @ApekshaBhosale , can you please review this Pr. |
Hi @ApekshaBhosale |
Hi @rohan-arthur , if you could have an enough bandwidth , could you plese review this pr also. |
…smith into fix/bug-130-file-picker-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
app/client/cypress/fixtures/dummy.pdfis excluded by!**/*.pdf
Files selected for processing (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.ts (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.ts (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.
Avoid using agHelper.sleep().
Use locator variables for locators and do not use plain strings.
Use data-* attributes for selectors.
Avoid Xpaths, Attributes and CSS path.
Avoid selectors like .btn.submit or button[type=submit].
Perform logins via API with LoginFromAPI.
Perform logout via API with LogOutviaAPI.
Perform signup via API with SignupFromAPI.
Avoid using it.only.
Avoid using after and aftereach in test cases.
Use multiple assertions for expect statements.
Avoid using strings for assertions.
Do not use duplicate filenames even with different paths.
Additional comments not posted (2)
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.ts (2)
12-14: LGTM!The test suite description and tags are appropriate and follow best practices.
16-18: LGTM!The
beforehook correctly sets up the test environment by dragging and dropping the FilePicker widget and verifying its presence.
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.ts
Show resolved
Hide resolved
app/client/cypress/e2e/Regression/ClientSide/Widgets/Filepicker/FilePicker_Max_No_Spec.ts
Show resolved
Hide resolved
|
…smith into fix/bug-130-file-picker-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857
|
@saiprabhu-dandanayak It would be greatly helpful if you can fix the images and video not coming up in the description. Please update the description with correct links(or re-upload the assets) |
Hi @rahulbarwal , could you pls refresh it again , it could see both images and video , if still you are unable to see those assets , will re-upload them.
Full screenshot
|
|
Hello, @rahulbarwal I've uploaded the assets again and have attached a full page screenshot. If you were unable to view them in the PR description, please check full page screenshot. |
|
Hello @rahulbarwal , If you have sufficient bandwidth, please read this PR. |
|
Hi @rahulbarwal , @sagar-qa007 , When I recently pulled the latest changes from the release branch and attempted to push my changes to the source branch, I encountered an issue due to a pre-push hook that prevented me from pushing the changes. Can you please provide a solution to resolve this issue? Screenshot
|
|
@saiprabhu-dandanayak |
…smith into fix/bug-130-file-picker-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857
|
Hi @rahulbarwal , i have took the recent pull from |
…smith into fix/bug-130-file-picker-widget-able-to-pick-a-file-when-max-no-of-files-is-defined-as-zero-0-14857
|
Hi @rahulbarwal , i have took recent pull from the |
User Description
Bug : File Picker Bug
I have raised this PR
In order to restrict user from uploading files when max-no of files is set to 0 and displaying an error message when and clicks on filepickerScreenshots
1 file is uploded when max no-of files is set to 0
Displaying error msg and restricting user from uploading files
Unit testcases
Cypress Testing video
FilePicker_Max_No_Spec.js.mp4