-
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
[EUWE] Don't lookup category names if tag tree view all #13308
Conversation
ugh, rails 5 and |
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.
Seems fine, but take a look at my comment about the return
statement. Not sure why there is a difference there.
@@ -44,17 +44,26 @@ def root_options | |||
end | |||
|
|||
def x_get_tree_roots(count_only, _options) | |||
return @categories.size if count_only |
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.
There a reason the return
was removed from the "master" version of this PR: https://github.com/ManageIQ/manageiq-ui-classic/pull/57/files
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.
That was probably an oversight. May have to put in a PR to get this line into master
else | ||
kid_id = "#{parent.name}-#{kid.name}" | ||
(@edit && @edit.fetch_path(:new, :filters, kid_id)) || (@filters && @filters.key?(kid_id)) | ||
end |
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.
So this is the main savings here, correct? We are just avoiding building kid_id
for each node since it is not used when @edit
and @filters
are blank, correct?
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.
Also, just noticed that contain_selected_kid
is basically this exact same logic. Is it possible to refactor this so we don't have so much code duplication?
This is minor, don't bother if it isn't worth it. Also, we have already merged this in master, so we would probably want to go back and do this there as well, which might be a pain.
1afeb3d
to
5904f18
Compare
Checked commit kbrock@5904f18 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
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.
Ugh... after my suggestions... now I see an issue with has_key_path?
...
kids = parent.entries.map do |kid| | ||
kid_id = "#{parent.name}-#{kid.name}" | ||
select = (@edit && @edit.fetch_path(:new, :filters, kid_id)) || (@filters && @filters.key?(kid_id)) | ||
select = selected_entry_value(parent, kid) |
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.
I should have asked this before... was this supposed to be a boolean or the value for that key? Seems like it should have been the value, but not sure.
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.
thanks - fixed
5904f18
to
24ab41c
Compare
@NickLaMuro is this better? |
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.
Looks good. I think I probably had you do a bit more than was necessary to get the job done, so thanks for the extra work. 👍
This was executing a few hundred extra queries category.name is the culprit https://bugzilla.redhat.com/show_bug.cgi?id=1399345
24ab41c
to
c6fb39b
Compare
@NickLaMuro cool. I changed Will put in a master PR to get these changes into there. |
When building the tag tree in access control groups, the tag tree is not expandable. But building the node name (using
category.name
) executes a few hundred extra queries (20%).This skips those queries.
Numbers are for small 5_7 db. numbers in larger DBS will increase for each tag and node in the db. But they don't complete for me within a few minutes, so it was too hard to get numbers.
/ops/explorer
before/ops/explorer
afterhttps://bugzilla.redhat.com/show_bug.cgi?id=1399345
This is the E version of #13301