-
Notifications
You must be signed in to change notification settings - Fork 93
LF-4995: Create post endpoint to add market directory info #3914
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
LF-4995: Create post endpoint to add market directory info #3914
Conversation
Co-authored-by: Joyce Yuki <[email protected]>
| phone_number: { type: ['string', 'null'], maxLength: 255 }, | ||
| address: { type: 'string', minLength: 1, maxLength: 255 }, | ||
| website: { type: ['string', 'null'] }, | ||
| website: { type: ['string', 'null'], maxLength: 2000 }, |
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.
maxLength set based on typical browser URL limits: https://stackoverflow.com/questions/417142/what-is-the-maximum-length-of-a-url-in-different-browsers
* Extract username * Adjust checkAndTransformMarketDirectoryInfo and tests
|
Updated social validation to be stricter, and the function now only returns a username. |
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.
It looks fantastic! I love the social validations and their test suite ❤️
Now that I've seen it, I think I might go completely against what I said yesterday (😓) and run social validation and trim on the frontend as well, onBlur() -- that actually might be easier than parsing each error message for the offending social when the error is returned from the API. Do you anticipate any problems with moving validateSocialAndExtractUsername() into packages/shared?
| // 3: username ([A-Za-z0-9._-]+) | ||
| // 4: trailing path/query (optional) ([/?#].*)? e.g., "/?hl=en", "/#" | ||
| const urlMatch = trimmedInput.match( | ||
| new RegExp(`^(https?://)?(www.)?${domain}/([A-Za-z0-9._-]+)([/?#].*)?$`), |
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.
I think the validations you picked are the perfect balance of permissive and strict 👌
The only thing I wonder if we should be a tiny bit more permissive on would be a capitalized domain? Like Facebook.com/username or Instagram.com/username? My Android phone does this initial capitalization automatically in form fields so I bet other systems do too.
Maybe just this regex could be case insensitive ('i')?
|
Thank you @kathyavini! |
|
@SayakaOno oh yeah TypeScript, duh 🤦♀️ I did try the straight import yesterday, saw it didn't work, and copied the file to frontend 😂 |
Description
market_directory_infopermissions.POST /market_directory_infoendpoint with tests.LiteFarmRequestso thatResBodyandReqBodycan be specified.mock.factories.Note:
Validation logic should be updated when frontend validations for socials and URLs are implemented.
Jira link: https://lite-farm.atlassian.net/browse/LF-4995
Type of change
How Has This Been Tested?
Checklist:
pnpm i18nto help with this)