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

Merge 0.6 and master branches to add support for Rails >= 4.2 #74

Merged
merged 2 commits into from
Apr 17, 2019

Conversation

ahmetabdi
Copy link

@ahmetabdi ahmetabdi commented Apr 17, 2019

This adds support for both 4.2.x and 5.2 and up by having two versions of Vault::EncryptedModel in the codebase they both work very differently, so it made the most sense to keep them separate.

This now means that when we want to make changes to one of the versions of Vault::EncryptedModel we'll need to make the change in two files, this is a better solution than the current process as previously we had to manage two codebases and two versions, so it's a step in the right direction.

Changes:

  • Adds support for Rails 4.2 and up

  • No longer need to include Vault::AttributeProxy to get vault_attribute_proxy as it is part of Vault::EncryptedModel for both 4.2.x and 5.x versions (This is not a breaking change as it only displays a warning)

  • If a type is provided which doesn't exist as a valid ActiveRecord::Type in the 5.x and up version on Vault::EncryptedModel we now fall back to ActiveModel::Type::Value e.g. IPAddr will now use the ActiveModel::Type::Value type

  • Breaking change - Passing an ActiveRecord type as an object to type e.g. the below is no longer supported, use a symbol e.g. :time instead.

  vault_attribute :time_data,
    type: ActiveRecord::Type::Time.new,
    encode: -> (raw) { raw.to_s if raw },
    decode: -> (raw) { raw.to_time if raw }

This is hard to tell in the PR but the change was on this line https://github.com/FundingCircle/fc-vault-rails/pull/74/files#diff-49c0b5f7740f6464b0867afa2fe6c6e5R172 previous it was attribute_type

@ahmetabdi
Copy link
Author

/cc @FundingCircle/gdpr-engineering

Copy link
Member

@popovm popovm left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ahmetabdi ahmetabdi merged commit b879e6c into master Apr 17, 2019
@ahmetabdi ahmetabdi deleted the merging_master_and_0_6_branch branch April 17, 2019 14:28
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