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

[WIP] update MiqReport::Search to include all columns #18176

Closed
wants to merge 1 commit into from
Closed

[WIP] update MiqReport::Search to include all columns #18176

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 13 additions & 1 deletion app/models/miq_report/search.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,19 @@ def paged_view_search(options = {})

search_options = options.merge(:class => db, :conditions => conditions, :include_for_find => includes)
search_options.merge!(:limit => limit, :offset => offset, :order => order) if order
search_options[:extra_cols] = va_sql_cols if va_sql_cols.present?

extra_cols = if includes
Copy link
Member

Choose a reason for hiding this comment

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

Kinda seems like this should be put in it's own method. va_sql_cols is just a memoized method as well, so you could toss it there, but I could see the argument that this should be it's own method and probably makes sense that it is.

Copy link
Member

Choose a reason for hiding this comment

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

I am also confused why this is needed, which leads back to LJ's comment here:

#18176 (comment)

The :extra_cols attribute on Rbac (**cough** **cough** nick is the original author **cough** **cough**) was meant to add additional columns and not remove ones that were already there (even if the default Arel.star was originally just assumed). So something seems borked in Rails if we need to start adding the foreign keys ourselves, or we broke something in manageiq along the way.

Is there possibly an additional .select in this report that is causing the issue? I also have some changes in #18353 that are affecting the results for :extra_cols that might be beneficial or detrimental to what you have here as well.

includes.keys.map do |rel|
if (rel = db_class.reflect_on_association(rel))
# adding primary_key is probably overkill.
Copy link
Member

Choose a reason for hiding this comment

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

You should always have the primary key just in case an association is needed on the model down the line, or you are immediately converting the results to a lower level data model. I think in the case of reports, we aren't doing that, so the use of relations might be necessary.

I would say remove this comment, just so there is no confusion.

rel.macro == :belongs_to ? rel.foreign_key : db_class.primary_key
end
end.compact
else
[]
end
extra_cols += va_sql_cols if va_sql_cols.present?
search_options[:extra_cols] = extra_cols if extra_cols.present?

if options[:parent]
targets = get_parent_targets(options[:parent], options[:association] || options[:parent_method])
Expand Down