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

Update Rails to 5.0.6 #19811

Merged
merged 4 commits into from
Jan 10, 2018
Merged

Update Rails to 5.0.6 #19811

merged 4 commits into from
Jan 10, 2018

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Jan 5, 2018

Update patch-version of Rails (using bundle update rails). This includes security/bug fixes and support for Ruby 2.5.

Also adds ruby '~> 2.2' version specifier to the Gemfile. This tells Bundler to raise an error if the current Ruby version doesn't match the specifier. This provided ~> 2.2 specifier should be a good starting point because Rails 5.0 already requires >= 2.2.2. Setting this specifier now will let us easily bump this constraint once we adopt language features only present in future Ruby versions. (The specifier adds a patch-level RUBY VERSION string to Gemfile.lock but this doesn't seem to be updated unless something else in the Gemfile.lock is also modified, so won't cause unnecessary thrashing in the lockfile.)

Includes a fix to ops_mailer due to a minor-version update to mail that fixed a bug affecting implied behavior related to trimming whitespace/newlines.

Add ruby-version specifier to Gemfile
remove newline
Since `mail v2.7.0`, this is no longer done automatically.
`mail 2.7.0` supports RFC6532, enabling internationalized email headers.
@wjordan wjordan requested a review from aoby January 10, 2018 19:33
@wjordan
Copy link
Contributor Author

wjordan commented Jan 10, 2018

@aoby could you let me know if the changes to the ops-mailer class looks OK (looks like old/unused/legacy code anyway?), and also more generally if this behavior change to valid-email addresses from upgrading mail won't cause any other problems/side-effects elsewhere in our mailer system?

@aoby
Copy link
Contributor

aoby commented Jan 10, 2018

Incidentally, @joshlory just removed most of the old ops dashboard code, but apparently missed ops_mailer? See #19836

Anyway, this looks fine and is indeed dead code.

As an aside, we are using 2 different email validations throughout our code base that we should reconcile / consolidate at some point - the validates_email_format_of gem, and our own Cdo::EmailValidator which wraps Mail. We had issues with discrepancies between them this year during HOC signup, where the gem was more permissive than the EmailValidator and allowed form responses that failed later validation in process_forms. Jeremy made a workaround and it stopped bugging us. I wonder if this version upgrade will incidentally fix that discrepancy (as evidence by the test fix). However we're also pretty sure that the failing emails were actually typos, even if they were technically valid.

@wjordan wjordan merged commit 5240528 into staging Jan 10, 2018
@wjordan wjordan deleted the rails-5.0.6 branch January 10, 2018 21:36
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

2 participants