Skip to content

Rails 6#4466

Merged
mitchellhenke merged 6 commits intomasterfrom
mitchellhenke/rails-lg-3753
Nov 30, 2020
Merged

Rails 6#4466
mitchellhenke merged 6 commits intomasterfrom
mitchellhenke/rails-lg-3753

Conversation

@mitchellhenke
Copy link
Contributor

Following #4451 #4453 #4462, can finally update to Rails 6 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

WARNING: NOT conditions will no longer behave as NOR in Rails 6.1. To continue using NOR conditions, NOT each condition individually (`.where.not(:iaa => ...).where.not(:iaa_start_date => ...).where.not(:iaa_end_date => ...)`)

where.not being NOR is deprecated and will become NAND in 6.1 (rails/rails#31209)

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow that thread was a ride

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this configuration is forward compatible, but not backward compatible. Once we confirm we are not rolling back to Rails 5.2, we can remove this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEPRECATION WARNING: ActionView::Base instances should be constructed with a lookup context, assignments, and a controller.

The presenter class only uses the view_context to access the request referer header, so I slightly refactored to pass that in directly instead of using the view context.

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/rails-lg-3753 branch from 4339565 to 8e8fce1 Compare November 30, 2020 17:22
end

it 'gracefully handles invalid formats' do
xit 'gracefully handles invalid formats' do
Copy link
Contributor

Choose a reason for hiding this comment

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

what would it take to bring this spec back? I think it was guarding for real regressions we might see from various scans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was my hope as well. it looks like we can render a 406 error page, but that's still a significant change in behavior. do we do scans that send malformed Accept or Content-Type headers and need the server response to ignore that?

Copy link
Contributor

Choose a reason for hiding this comment

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

The goal of the spec is to not blow up, so anything other than a 500 should be acceptable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in 471e5e2 and cbac137

@mitchellhenke mitchellhenke marked this pull request as ready for review November 30, 2020 18:15
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/rails-lg-3753 branch from f79d07f to cbac137 Compare November 30, 2020 19:57
Comment on lines 27 to 30
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this return a 200 if we render the 406 page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I misunderstood how this works. Since it's in the test environment, it always raises and doesn't render the 406 page as I expected.

If I set config.consider_all_requests_local = false it renders like it would in prod, but I can't find a way to test that this gets rendered appropriately in test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably needs to be a request spec --- I would remove the response.status then and just let it assume that the error being raised is appropriate

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/rails-lg-3753 branch from cbac137 to 36c6e40 Compare November 30, 2020 20:36
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@jmhooper
Copy link
Contributor

Lots of work here, nicely done!

@mitchellhenke mitchellhenke merged commit f8f0515 into master Nov 30, 2020
@mitchellhenke mitchellhenke deleted the mitchellhenke/rails-lg-3753 branch November 30, 2020 21:37
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.

3 participants