Skip to content

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