Move ResourceGroup relationship into VmOrTemplate model #14948
Conversation
spec/models/resource_group_spec.rb
Outdated
| end | ||
|
|
||
| it "has many vms_or_templates" do | ||
| expect(resource_group).to respond_to(:vm_or_templates) |
There was a problem hiding this comment.
I feel like the expectation on respond_to is unnecessary if the method is called on the next line.
There was a problem hiding this comment.
I know people disagree with me on tests like this, but I like them because they're good for sanity checking. At worst, it's harmless.
There was a problem hiding this comment.
Again, these tests appear to testing rails itself.
| @@ -335,6 +335,19 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "#resource_group" do | |||
| before do | |||
There was a problem hiding this comment.
Why the context, @variables and a before block if there is only one test?
There was a problem hiding this comment.
In case I add more tests later.
| @@ -335,6 +335,19 @@ | |||
| end | |||
| end | |||
|
|
|||
| context "#resource_group" do | |||
| before do | |||
There was a problem hiding this comment.
On second thought... Do we even need this test? It feels like we're testing rails here.
There was a problem hiding this comment.
Again, sanity testing in case someone adds scopes/filters/whatever that conflict with what I'm expecting.
There was a problem hiding this comment.
If this is a regular activerecord has_one, I don't understand why we'd need to test that. They're usually helpful when you're developing and want to make sure you did both sides of the association correctly but aren't really needed after that.
| @@ -79,6 +79,8 @@ class VmOrTemplate < ApplicationRecord | |||
| has_many :guest_applications, :dependent => :destroy | |||
| has_many :patches, :dependent => :destroy | |||
|
|
|||
| belongs_to :resource_group | |||
app/models/resource_group.rb
Outdated
|
|
||
| has_many :vm_or_templates | ||
|
|
||
| has_many :vms, -> { where(:template => false) }, :class_name => 'VmOrTemplate' |
There was a problem hiding this comment.
Do we want to use :class_name => 'Vm' and :class_name => 'Template'?
There was a problem hiding this comment.
Without changing the tests, if I do that I get: uninitialized constant ResourceGroup::Template. There is a "Vm" class, but no "Template" class.
|
Also, due to architectural limitations, moving relations up to a parent makes life simpler |
| @@ -1,3 +1,8 @@ | |||
| class ResourceGroup < ApplicationRecord | |||
| has_many :vms | |||
| alias_attribute :images, :templates | |||
There was a problem hiding this comment.
I just thought it would be a handy alias. :)
|
@djberg96 The class is |
Modified vms method, added templates method. Added resource_group specs. Updated ResourceGroup specs to reflect model changes. Rely on default scope for vms method. Remove tests that were only testing Rails. Update templates association and specs.
|
@bdunne Thanks, updated. |
|
Checked commit https://github.com/djberg96/manageiq/commit/00d261489dfc201db48eee38a7b68a7a88ead5bd with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@bdunne @blomquisg bump |
This is a followup to #14000.
Originally I put the resource group relationship in the cloud_manager/vm.rb. Unfortunately, this left templates out. So, I've set the relationship in the VmOrTemplate model instead.
For backwards compatibility (and general handiness) I've created a
vmsmethod, as well as atemplatesmethod (and an :images alias). I've also updated the specs.