-
Notifications
You must be signed in to change notification settings - Fork 897
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
Properly generate report yml include with virtual attributes #18205
Conversation
e96d09c
to
c97e707
Compare
I love this change so much...can we remove the "includes" sections from the report.yml files after this? I would love to kill an entire section |
*rels, _col = col.split(".") | ||
rels.inject(ret) { |h, rel| h[rel.to_sym] ||= {} } unless col =~ /managed\./ | ||
end | ||
include_as_hash(include.presence || invent_report_includes).deep_merge(include_for_find || {}).presence |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My question in #18205 (comment) is really asking, can we remove include.presence
from this section entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
users tend to copy paste the report templates, and most of the report views have a blank include block. Of our 180 reports, 95 are a blank hash.
$ ag -l '^include:\n\n' product/views/ | wc -l
95
$ ag -l '^include:\n' product/views/ | wc -l
170
$ find product/views | wc -l
180
c97e707
to
e7e781a
Compare
@Fryguy Sweet. This is working. For Unlike the last time I tried to make this change, the columns on associations (e.g.: |
e7e781a
to
1d26993
Compare
If a report yml file has virtual attributes in the cols and no :include it was not generating the proper includes hash. Now it is generating the includes from :col_order or :include regardless of whether virtual attributes are in the col (or col_order) leaving invent_includes around for report_sanity_checker
1d26993
to
1b31bd7
Compare
@Fryguy changed some comments to actual tests. Are you good to go with this PR? |
Checked commit kbrock@1b31bd7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
Ugh. Rebasing onto red master strikes again |
This one has the potential to break the UI...can you verify that the UI tests pass with this PR? cc @himdel (This is where I would have liked to do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending UI test verification.
What this effects:
And this makes sense, because if we did have any of those, they would be broken already. Hmm. Little confused this bug bit us so badly, since if we tend to remove the |
Problem
Before
If a
report.yml
file had virtual attributes in the:cols
but did not have an:include
entry,it was not properly generating the
includes().references()
hash.REMINDER:
report.yml
files are used to represent every data view in our product.After
It is now generating the correct
:includes
from:col_order
when:include
is not defined,regardless of whether
:col
contains virtual attributes or not.Impact
We are constantly tuning the
app/model
classes. Sometimes the relations change (e.g.:pictures
no longer points tobinary_blobs
) and other times the virtual attributes are embeddable in sql. So this means the correctincludes()
/references()
values change.Since the
report.yml
files are in a different git repo, they are often not updated. This means extra data gets downloaded, or in some cases, just plain break. Which happened withServiceTemplate.yml
.The frustrating part is the
includes
entries can just be derived from theapp/model
class relationships, and shouldn't even need to be declared in thereport.yml
files in the first place.Conclusion
Properly generating the
includes()
allows us to remove more of the:include
entries from the reports, and that means we have fewer incorrect reports / views. And it means faster views./cc @jrafanie @martinpovolny