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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default value of Vault attributes to String #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

finalwharf
Copy link

Since Vault assumes that all attributes are Strings, unless explicitly
defined otherwise, it makes sense to have this as a default. This will make
it easier (shorter) to define attributes like house and phone numbers for
example, where they can include only numbers and be mistaked for Integers
by ActiveRecord.

@FundingCircle/gdpr-engineering 馃憖

Since Vault assumes that all attributes are `String`s, unless explicitly
defined otherwise, it makes sense to have this as a default. This will make
it easier (shorter) to define attributes like house and phone numbers for
example, where they can include only numbers and be mistaked for Integers
by ActiveRecord.
@finalwharf
Copy link
Author

@h-lame @elenatanasoiu please 馃憖

Copy link

@h-lame h-lame left a comment

Choose a reason for hiding this comment

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

In one of the attribute proxy PRs @elenatanasoiu and I were talking about having to add types to all the vault attribute definitions to avoid can't convert an Integer into a String errors with vault_attributes that rails wouldn't have with db columns. Defaulting the type to string would mean we'd only need to add a type for the ones that aren't strings. This is probably a pragmatic choice, even if there's part of me that thinks it's a good idea to be explicit everywhere.

Is there a test we can write to show that this is working? E.g. maybe setting an integer value and having it come out as a string by default when it's on an attribute defined without a type. This would fail if the type was :value - we could even write a test that expects that failure too for a type: :value attribute.

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.

None yet

2 participants