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.

Sign up for GitHub

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

Jump to bottom

Validate towhat policy field #18032

Merged
merged 3 commits into from
Sep 28, 2018
Merged

Validate towhat policy field #18032

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d344e43
Validate towhat policy field
jvlcek Sep 28, 2018
6152ab4
Alphabetize TOWHAT_APPLIES_TO_CLASSES and focus new tests on towhat.
jvlcek Sep 28, 2018
a9bdbd9
Simplify the new specs based on review feedback.
jvlcek Sep 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/models/miq_policy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
# TODO: Import/Export support

class MiqPolicy < ApplicationRecord
TOWHAT_APPLIES_TO_CLASSES = %w(ContainerGroup
ContainerImage
ContainerNode
ContainerProject
ContainerReplicator
ExtManagementSystem
Host
PhysicalServer
Vm).freeze

acts_as_miq_taggable
acts_as_miq_set_member
include_concern 'ImportExport'
Expand Down Expand Up @@ -29,6 +39,7 @@ class MiqPolicy < ApplicationRecord
validates_presence_of :name, :description, :guid
validates_uniqueness_of :name, :description, :guid
validates :mode, :inclusion => { :in => %w(compliance control) }
validates :towhat, :inclusion => { :in => TOWHAT_APPLIES_TO_CLASSES }

scope :with_mode, ->(mode) { where(:mode => mode) }
scope :with_towhat, ->(towhat) { where(:towhat => towhat) }
Expand Down
18 changes: 18 additions & 0 deletions spec/models/miq_policy_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,4 +307,22 @@
)
end
end

context '.validates' do
it 'validates towhat' do
expect(described_class.create!(:towhat => "Host",
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to save the record to test this, you can

expect(FactoryGirl.build(:miq_policy, :towhat => "Host", :mode => "compliance").valid?).to be true

Copy link
Member

Choose a reason for hiding this comment

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

Should mode even be tested here since it didn't change in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

I'm usually not a huge fan of this syntax, but you could also

expect(FactoryGirl.build(:miq_policy, :towhat => "Host")).to be_valid

Copy link
Member Author

Choose a reason for hiding this comment

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

@bdunne Thank you for the input. I'll have the new test focus on towhat

I like the syntax the way it is and if you don't have strong feelings about it I'd like to leave it as is, mostly because it's more consistent with the tests through the spec.

Let me know if you are OK with leaving the test as is.

Thank you. JoeV

Copy link
Member

@bdunne bdunne Sep 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I guess I don't see the point of writing to the database if we just want to make sure the instance is valid. Also, using FactoryGirl makes it much more obvious that you're only testing :towhat and not any other validation.

Copy link
Member

Choose a reason for hiding this comment

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

I guess the question is: are you testing that it's valid or testing if it raises an exception. If you're testing validity: you don't need to create the record, you can do as @bdunne suggests. If you're testing exception raising, you need to create!

It looks like your testing validity so create! isn't needed here. It's minor and I know other code does this but it's fine to mix patterns in the specs until you can convert the rest to the new way.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bdunne and @jrafanie Ah I see the reasoning now. Thank you for explaining. Will do.

JoeV

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I prefer expecting the specific thing (validity) rather than the side effect of rails raising an error when calling create! on an invalid instance.

:active => false,
:description => 'x',)).to have_attributes(:towhat => "Host",
:active => false)
end

it 'reports invalid towhat' do
expect do
described_class.create!(:towhat => "BobsYourUncle",
Copy link
Member

Choose a reason for hiding this comment

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

Similar here

expect(FactoryGirl.build(:miq_policy, :towhat => "BobsYourUncle")).not_to be_valid

Copy link
Member Author

Choose a reason for hiding this comment

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

@bdunne Here again, if you feel strongly I'd be glad to change this test but for me it's much easier to quickly grasp what the test is doing, that a particular exception is expected and it's more consistent with existing tests in the spec.

Let me know if you are OK with it as is.

Thank you for the review feedback.
JoeV

Copy link
Member

@jrafanie jrafanie Sep 28, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'd be ok with testing create! if you're concerned with a custom validation message but since this is out of the box validation messages, I think you can just do thing.new + thing.valid? and maybe even thing.errors[:towhat].blank? or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jrafanie Thank you for explaining. Will do.

JoeV

:active => false,
:description => 'x')
end.to raise_error(ActiveRecord::RecordInvalid,
"Validation failed: MiqPolicy: Towhat is not included in the list")
end
end
end