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

Add CustomAttribute#name validation #17372

Closed

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented May 1, 2018

@miq-bot
Copy link
Member

miq-bot commented May 1, 2018

Checked commit bdunne@49a117c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

spec/models/custom_attribute_spec.rb

@JPrause
Copy link
Member

JPrause commented May 2, 2018

@miq-bot add_label blocker

@miq-bot miq-bot added the blocker label May 2, 2018
@bdunne
Copy link
Member Author

bdunne commented May 2, 2018

@JPrause Why is this being labeled as a blocker? As the tests point out, it actually breaks current implementation of several features.

@@ -4,6 +4,8 @@ class CustomAttribute < ApplicationRecord
belongs_to :resource, :polymorphic => true
serialize :serialized_value

validates :name, :format => {:with => /\A[\p{Alpha}_][\p{Alpha}_\d\$]*\z/, :message => "must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters can be letters, underscores, digits (0-9), or dollar signs ($)"}
Copy link
Member

@jrafanie jrafanie May 2, 2018

Choose a reason for hiding this comment

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

I wish validation messages showed the tested string, it's not clear what failed in the tests... you have to go to the test and find what name we're providing...

Validation failed: Name must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters can be letters, underscores, digits (0-9), or dollar signs ($)

@jrafanie
Copy link
Member

jrafanie commented May 2, 2018

@JPrause Yeah, this isn't a blocker since I opened this issue. I'm not even sure we can backport to gaprindashvili directly since it might break other features. We probably would have to deprecate it unless we know it won't break things.

@JPrause
Copy link
Member

JPrause commented May 2, 2018

The associated BZ is a blocker.

@bdunne
Copy link
Member Author

bdunne commented May 14, 2018

This doesn't appear to be the way we want to go about fixing this. We should probably make sure that consumers of these attributes are able to handle the various cases of special characters in the attribute names.

@bdunne bdunne closed this May 14, 2018
@bdunne bdunne deleted the validate_custom_attribute_name branch May 14, 2018 15:24
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.

4 participants