-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
fix: add regex check on phone number change #34715
base: release
Are you sure you want to change the base?
fix: add regex check on phone number change #34715
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
|
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: 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.wait
in Cypress tests.Using
cy.wait
can 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.submit
orbutton[type=submit]
.
Perform logins via API withLoginFromAPI
.
Only interact with controlled sites/servers.
Ensure tests can run withit.only
and are independent.
Usebefore
,beforeEach
,after
,afterEach
correctly; 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 inbefore
hooks.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SaiCharanChetpelly31 can you write a comment here what this regex actually means?
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.
@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 ? |
6eb0a71
to
631facf
Compare
Fixes - 25911
Summary by CodeRabbit
New Features
Tests