fix:default value for PhoneInput set to text which is invalid(#34594) #34742
Conversation
WalkthroughThe changes introduce new tests, error handling, and validation features for the Phone Input widget. Specifically, the test suite has been updated to verify default values and error scenarios, while the widget's main file now ensures that default values are numbers and displays appropriate error messages when they are not. Changes
Sequence Diagram(s)The changes are relatively straightforward and do not introduce a new feature or significantly modify control flow, so no sequence diagrams are necessary. Possibly related 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 Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_DefaultValue_spec.ts (1 hunks)
- app/client/cypress/support/Pages/PropertyPane.ts (1 hunks)
- app/client/src/widgets/PhoneInputWidget/widget/index.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/support/Pages/PropertyPane.ts
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_DefaultValue_spec.ts (1)
Pattern
app/client/cypress/**/**.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait in code.
Use variables for locators, not strings.
Usedata-*attributes for selectors; avoid Xpaths and CSS attributes.
Avoid selectors like.btn.submitorbutton[type=submit].
Perform logins via API withLoginFromAPI.
Only interact with controlled sites/servers.
Ensure tests can run withit.onlyand are independent.
Usebefore,beforeEach,after,afterEachcorrectly; clean state before tests.
Check new specs for flakiness by running them 10 times on CI.
Use multiple assertions; don't treat Cypress as unit tests.
Use constants for strings.
Include datasource operations inbeforehooks.
Additional comments not posted (7)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_DefaultValue_spec.ts (5)
1-7: LGTM! Import addition is appropriate.The addition of
commonlocatorsis necessary for the tests.
9-12: LGTM! Describe block is well-structured.The describe block is correctly structured and tagged for the Phone Input widget tests.
13-15: LGTM! Before hook setup is appropriate.The before hook correctly initializes the state by dragging and dropping the Phone Input widget.
17-23: LGTM! Test case implementation is correct.The test case correctly verifies the default value functionality of the Phone Input widget.
24-28: LGTM! Test case implementation is correct.The test case correctly verifies the error handling when the default value is not a number.
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (2)
55-58: LGTM! Addition of number error message is appropriate.The number error message is correctly defined.
79-86: LGTM! Number validation logic is correctly implemented.The function correctly parses the value as a number and validates it.
|
|
||
| describe( | ||
| "Phone Input widget Tests", | ||
| { tags: ["@tag.Widget", "@tag.PhoneInput"] }, |
There was a problem hiding this comment.
- Add description to understand this spec more
- remove before and make everything as one test
- Don't use string to validate messages. use variable instead from respective files
There was a problem hiding this comment.
- Add description to understand this spec more
- remove before and make everything as one test
- Don't use string to validate messages. use variable instead from respective files
hello @ApekshaBhosale can you please be more specific about the third one ,are you saying about this,
'agHelper.UpdateCodeInput(propPane._propertyContentDefaultValueField, '9872139');',
and
'agHelper.VerifyEvaluatedErrorMessage('This value must be number');'
There was a problem hiding this comment.
i am talking about 'agHelper.VerifyEvaluatedErrorMessage('This value must be number');' ===> This value must be number
There was a problem hiding this comment.
Everywhere they are using the string @ApekshaBhosale , I think its not declared anywhere ,I couldnt find any file where its declared ,can I declare it in a file can I use it here ?
|
hello @ApekshaBhosale made changes as suggested ,can you review it now |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_DefaultValue_spec.ts (1 hunks)
- app/client/cypress/support/Objects/PhoneInputWidgetErrorMessage.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- app/client/cypress/support/Objects/PhoneInputWidgetErrorMessage.ts
Files skipped from review as they are similar to previous changes (1)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_DefaultValue_spec.ts
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 #
34594or
Fixes
https://github.com/appsmithorg/appsmith/issues/34594Warning
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?
SCREENSHOTS AFTER SOLVING THE ISSUE:
No issue when an number is entered
Shows error when a text is entered
VIDEO
VIDEO:CYPRESS TESTING
Summary by CodeRabbit
New Features
Tests
Bug Fixes