Make attribute last_scan_on sql-friendly #18198
Conversation
kbrock
force-pushed
the
virtual_attr_last_scan_on
branch
2 times, most recently
from
5230c2d
to
97c041f
Compare
associations can have scopes, and those can include select clauses.
When one of those relations are used, the sub-select generated will
contains too many columns:
sub-select returns 2 columns - expected 1
This change ensures only 1 column is in the virtual_attribute sub-select.
kbrock
force-pushed
the
virtual_attr_last_scan_on
branch
from
97c041f
to
9c91523
Compare
This makes the drift state columns virtual This is used by the Host-host.yml and 21 screen views
kbrock
force-pushed
the
virtual_attr_last_scan_on
branch
from
9c91523
to
1387ffe
Compare
|
Thanks @NickLaMuro - these read much better |
|
Checked commits kbrock/manageiq@aae11e5~...1387ffe with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
|
@kbrock outside of my review, I am thinking that this should be a hard Also, thanks for making all the fixes! |
|
|
||
| let(:host) { FactoryGirl.create(:host) } | ||
|
|
||
| let(:drift_states) do |
There was a problem hiding this comment.
Minor, but if this is a let! then you don't need to call it in any of the below methods for "setup"
There was a problem hiding this comment.
Oh, nvm...I didn't see that one test expressly can't have that...
That being said, perhaps there can be a context "with some drift states" around all of the tests that need that setup?
Again, it's minor...won't let it hold up a merge.
last_scan_onis used in 21product/views/*.ymlscreens.This PR converts
last_scan_onfrom a ruby method tovirtual_attribute.This uncovered a bug in
virtual_attribute'svirtual_delegate. If you delegate to an association that has aselect()defined, then the sql generated for the sub select is invalid:So this fixes the bug and converts
last_scan_onto a virtual attribute.Disclaimer: @NickLaMuro already fixed the main performance problem for https://bugzilla.redhat.com/show_bug.cgi?id=1648412 - but this adds a little to the performance effort.
Oh, and for the record. this is a
+5/-15PR - if I didn't need to add so many missing tests