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.

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

Fixes #29683 - fix params search on hg,os,domain,subnet pages #7670

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

kgaikwad
Copy link
Member

No description provided.

app/models/concerns/parameter_search.rb Outdated Show resolved Hide resolved
app/models/concerns/parameter_search.rb Outdated Show resolved Hide resolved
app/models/concerns/parameter_search.rb Outdated Show resolved Hide resolved
app/models/concerns/parameter_search.rb Outdated Show resolved Hide resolved
app/models/concerns/parameter_search.rb Outdated Show resolved Hide resolved
@theforeman-bot
Copy link
Member

Issues: #29683

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

see comments inline. can you add some tests as well please?

if respond_to?(:with_taxonomy_scope)
build_query = build_query.with_taxonomy_scope
end
resource_ids = build_query.joins(parameters_relation).where(conditions).map(&:id).flatten.uniq
Copy link
Member

Choose a reason for hiding this comment

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

I think this might be faster:

Suggested change
resource_ids = build_query.joins(parameters_relation).where(conditions).map(&:id).flatten.uniq
resource_ids = build_query.joins(parameters_relation).where(conditions).distinct.pluck(:id)

as it doesn't load all objects to memory but rather pulls the ids directly from the db

conditions = sanitize_sql_for_conditions(
["parameters.name = ? and parameters.searchable_value #{operator} ?", key_name, value_to_sql(operator, value)]
)
build_query = unscoped
Copy link
Member

Choose a reason for hiding this comment

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

what about authorization? or would that be added anyways by the controller after the search scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, at controller level it uses resource_base which calls authorized if that method exists on model.

end
resource_ids = build_query.joins(parameters_relation).where(conditions).map(&:id).flatten.uniq

opts = "#{table_name}.id < 0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
opts = "#{table_name}.id < 0"
opts = "1 < 0"

will get the same result without checking the table index

end

def parameter_relation_symbol
case to_s
Copy link
Member Author

Choose a reason for hiding this comment

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

@tbrisker,
Here, I changed method class_name to to_s as it was failing for class_name method while running tests.

@shiramax
Copy link
Contributor

@kgaikwad please rebase.

@kgaikwad
Copy link
Member Author

@shiramax, done with rebase!

@kgaikwad
Copy link
Member Author

Any progress on this?

shiramax
shiramax previously approved these changes Jul 14, 2020
@kgaikwad
Copy link
Member Author

@shiramax, it's green :) 🍏

@shiramax shiramax dismissed tbrisker’s stale review July 14, 2020 12:36

All comments were adderssed.

@shiramax shiramax merged commit f46e93f into theforeman:develop Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants