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

Binary type for attribute was getting overridden to Value type for 5.1, so passing type to attribute api. #359

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

priyankatapar
Copy link

@priyankatapar priyankatapar commented Dec 20, 2019

Binary type for attribute was getting changed to Value type for Rails 5.1 due to this, so passing type to attribute api.

Please note:
After the above fix Unit tests were failing as the tables did not exist in database.
Fixed unit tests, the tables need to be present in DB before the ActiveRecord classes are loaded as attr_encrypted call internally uses columns_hash to identify the attribute type.

@priyankatapar priyankatapar force-pushed the v3.1.0_CD-182663_rails5.1 branch 2 times, most recently from 89ca46c to 17ff856 Compare December 24, 2019 11:15
@priyankatapar priyankatapar changed the title Binary type for attribute was getting overridden to Value type for 5.1 due to this, so removed it. Binary type for attribute was getting overridden to Value type for 5.1, so passing type to attribute api. Dec 24, 2019
@priyankatapar priyankatapar force-pushed the v3.1.0_CD-182663_rails5.1 branch 2 times, most recently from 739c3fd to f334dab Compare January 3, 2020 11:57
…es, attr_encrypted uses `columns_hash` to identify column type in table.
@priyankatapar priyankatapar force-pushed the v3.1.0_CD-182663_rails5.1 branch from f334dab to e6f4893 Compare January 6, 2020 06:45
@saghaulor
Copy link
Contributor

@priyankatapar thanks for taking a stab at this. There's a lot of change unrelated to your fix. Let's keep the change focused. Moreover, something in your changes has entirely broken the build. Perhaps with a focused change this won't be an issue?

Lastly, we try to keep this code tested as well as possible. I don't think you have a test that asserts the behavior of your change. Please add tests.

@priyankatapar
Copy link
Author

@saghaulor, any update on this PR? Can we go forward with approach? Is the test changes fine?

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.

2 participants