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

Fix the ability to exclude encryption params from being autofiltered #44329

Merged
merged 1 commit into from Feb 8, 2022

Conversation

attack
Copy link
Contributor

@attack attack commented Feb 3, 2022

Summary

A bug is preventing the exclusion of params from being auto-filtered in conjunction with ActiveRecord encryption.

This fixes it by exposing the bug in a test, then using the correct variable in the code.

Copy link
Contributor

@nvasilevski nvasilevski left a comment

Choose a reason for hiding this comment

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

👍
Just in case confirmed that the test is failing on the main branch

Really good catch ❤️

Copy link
Contributor

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Good fix and test! This is good to merge @rafaelfranca.

@rafaelfranca rafaelfranca merged commit 6acdb4e into rails:main Feb 8, 2022
rafaelfranca added a commit that referenced this pull request Feb 8, 2022
Fix the ability to exclude encryption params from being autofiltered
@ghiculescu
Copy link
Member

@attack @jorgemanrubia FYI, this test has recently started failing on main:

image

https://buildkite.com/rails/rails/builds/84488#156fa457-40dd-4ced-87d4-83f42a7f22f4

@nvasilevski
Copy link
Contributor

Oops, sorry folks, I have renamed the method in a separate PR and should have asked for a rebase
Let me fix this failure

@nvasilevski
Copy link
Contributor

#44364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants