-
-
Notifications
You must be signed in to change notification settings - Fork 729
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
Instagram pattern #2859
Instagram pattern #2859
Conversation
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.
Thank you! This is great.
Since there are some detours in the commit history, we should squash the commits when merging.
Would you like to continue working on this? I have some ideas how to further improve this.
- Use a more precise regular expression to test the URL. Your current test would fail if someone had the handle
@i_love_instagram.com
which is valid. - Convert your
before_save
check into abefore_validation
check. - Include a validation check for the handle. I think it would be a regex like
/^@[a-zA-Z0-9._]{1,30}$/
.
Maybe we can do something like: validates :instagram, format: /\A@[a-zA-Z0-9._]{1,30}\z/
def instagram=(value)
write_attribute(:instagram, value.try!(:gsub, %r{\Ahttps?://(?:www.)?instagram.com/([a-zA-Z0-9._]{1,30})/?\z}, '@\1'))
end That way we care about that manipulating on setter and we care about the format on the validation. |
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.
Wow, this is so much better than my suggestions. I commented on a few things below that can still be improved.
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.
Nice! Thank you. That's really good now.
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.
nice!
they are complementary @sigmundpetersen |
Ready to go :) Testing notes: https://docs.google.com/document/d/1e8eWTX7YVYkkxYXpDZwXKiR_vUsRvpjrpqFvBElRfHI/edit# |
@RachL Sally found an issue with this pull request: #2849 (comment) We should have included in the test description that you need to check on a profile page if the Instagram link is actually working. :-D |
Issue: #1760
There is no suggestion in the Instagram empty field in enterprise profile setup menu. Some users copy-paste the full URL on their page. That doesn't work because what is expected is only their Instagram user pseudo.
FIX
Add an before save to check if the user enter the whole URL, and extract and store just the handle.
So, if the user enter "www.instagram.com/my_user", we store just "@my_user".