Skip to content
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

Add regex validation to user email #2389

Merged
merged 1 commit into from Jun 14, 2020

Conversation

sonalkr132
Copy link
Member

Fixes in delayed job to deliver mails:

501 Recipient syntax error

/usr/local/lib/ruby/2.5.0/net/smtp.rb:969:in `check_response'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:937:in `getok'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:865:in `rcptto'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:846:in `block in rcptto_list'

@sonalkr132 sonalkr132 force-pushed the email-regex-validation branch 3 times, most recently from ff92f62 to 3a8a297 Compare June 6, 2020 20:59
assert user.valid?
end

should "be invalid when it doesn't matche URI mail email regex" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/matche/match/g

@simi
Copy link
Member

simi commented Jun 7, 2020

What about old invalid values?

@sonalkr132
Copy link
Member Author

any tips on how do i run this?

User.where("email ~ '(?-mix:\A[a-zA-Z0-9.!\#$%&'*+\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*\z)'").count
Traceback (most recent call last):
        2: from (irb):12
        1: from (irb):12:in `rescue in irb_binding'
ActiveRecord::StatementInvalid (PG::SyntaxError: ERROR:  syntax error at or near "{")
LINE 1: ... WHERE (email ~ '(?-mix:A[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a...

@simi
Copy link
Member

simi commented Jun 7, 2020

Sadly that Ruby regexp uses unsupported feature in PostgreSQL regexp engine. It would be easier to check this in console by User.all.reject {|u| u.email.match?(URI::MailTo::EMAIL_REGEXP)}.

Comment on lines 7 to 9
expected_dom = %(<div class="errorExplanation" id="errorExplanation"><h2>3 errors prohibited this user from being saved</h2><p>There were \
problems with the following fields:</p><ul><li>Email address is not a valid email</li><li>Email address can't be blank</li><li>Email address is \
invalid</li></ul></div>)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expected_dom = %(<div class="errorExplanation" id="errorExplanation"><h2>3 errors prohibited this user from being saved</h2><p>There were \
problems with the following fields:</p><ul><li>Email address is not a valid email</li><li>Email address can't be blank</li><li>Email address is \
invalid</li></ul></div>)
expected_dom = <<~HTML.squish.gsub(/>\s+</, "><")
<div class="errorExplanation" id="errorExplanation">
<h2>3 errors prohibited this user from being saved</h2>
<p>There were problems with the following fields:</p>
<ul>
<li>Email address is not a valid email</li>
<li>Email address can't be blank</li>
<li>Email address is invalid</li>
</ul>
</div>
HTML

@colby-swandale
Copy link
Member

I think leaving any invalid email addresses as they are is fine for now. If the user ever updates their details, they will get an error saying their email is invalid.

We will need to update some of our scripts (ie: disable user) to skip validation as well.

@sonalkr132
Copy link
Member Author

remember token gets update during login, so login won't work. simi and I had private discussion about this. Only 16 users have invalid email and only one of them is active, we can handle it though help site.

Fixes in delayed job to deliver mails:
```
501 Recipient syntax error

/usr/local/lib/ruby/2.5.0/net/smtp.rb:969:in `check_response'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:937:in `getok'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:865:in `rcptto'
/usr/local/lib/ruby/2.5.0/net/smtp.rb:846:in `block in rcptto_list'
```
@sonalkr132 sonalkr132 merged commit bb2ba06 into rubygems:master Jun 14, 2020
@sonalkr132 sonalkr132 temporarily deployed to staging June 14, 2020 15:05 Inactive
@sonalkr132 sonalkr132 temporarily deployed to production June 15, 2020 20:00 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants