fix: add regex check on phone number change#34715
fix: add regex check on phone number change#34715SaiCharanChetpelly31 wants to merge 1 commit intoappsmithorg:releasefrom SaiCharanChetpelly31:fix/Issue_25911_onTextChanged_phone_input_widget
Conversation
WalkthroughThe latest changes enhance the phone input widget by adding a validation step that checks against a regex pattern before updating the Changes
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)
🔇 Additional comments (1)
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: 0
Outside diff range and nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (1)
295-297: Avoid usingcy.waitin Cypress tests.Using
cy.waitcan lead to flaky tests. Instead, use dynamic waits to ensure the toast message disappears before proceeding.Consider using
cy.get()withshould('not.exist')to wait for the toast message to disappear.- agHelper.Sleep(2000); + cy.get(locators._toastMsg).should('not.exist');
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (1 hunks)
- app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1 hunks)
Additional context used
Path-based instructions (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (1)
Pattern
app/client/cypress/**/*.*: Follow best practices for Cypress code and e2e automation.
Avoid using cy.wait.
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.
Learnings (1)
app/client/cypress/e2e/Regression/ClientSide/Widgets/PhoneInput/PhoneInput_Part2_spec.ts (1)
Learnt from: Aishwarya-U-R PR: appsmithorg/appsmith#29405 File: app/client/cypress/e2e/Regression/ClientSide/Widgets/Radio/Radio2_spec.ts:404-407 Timestamp: 2023-12-08T09:01:51.841Z Learning: The `Radio2_spec.ts` file is not part of the initial phase for the removal of static waits in the pull request titled "test: Cypress | Replace static with Dynamic waits - Part 1".
Additional comments not posted (1)
app/client/src/widgets/PhoneInputWidget/widget/index.tsx (1)
411-419: Ensure the regex pattern is appropriate for phone number validation.The regex pattern
/^(?!\s)[\d\s()+-]*$/allows digits, spaces, parentheses, plus signs, and hyphens. Ensure this pattern meets the validation requirements for phone numbers.
|
I hope this message finds you well. Your review is highly valued, and I am committed to ensuring this contribution meets our collective goals. I would greatly appreciate it if you could take some time to review the PR at your earliest convenience. Thank you for your attention to this matter, and I look forward to your feedback. |
| type: EventType.ON_TEXT_CHANGE, | ||
| }, | ||
| }); | ||
| if (/^(?!\s)[\d\s()+-]*$/.test(value)) { |
There was a problem hiding this comment.
@SaiCharanChetpelly31 can you write a comment here what this regex actually means?
There was a problem hiding this comment.
@jsartisan I added a comment explaining regex i used.
|
Hi @jsartisan Gentle Remainder: Thanks for reviewing.I have addressed review comment. Can you please review it? |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10041082887. |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10061534208. |
|
Deploy-Preview-URL: https://ce-34715.dp.appsmith.com |
|
Hi @ramsaptami Could you please merge this PR if everything is ok? |
|
@jsartisan please review and merge if everything is ok |
|
@rahulbarwal Can you please check this ? |
SaiCharanChetpelly31
requested review from
a team,
ankitakinger,
ayushpahwa and
sharat87
as code owners
SaiCharanChetpelly31
requested review from
a team and
KelvinOm
as code owners
4 participants
Fixes - 25911
Summary by CodeRabbit
New Features
Tests