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

Add MiqExpression support for managed filters #15623

Merged
merged 2 commits into from
Aug 9, 2017

Conversation

imtayadeway
Copy link
Contributor

@imtayadeway imtayadeway commented Jul 20, 2017

Just opening as a wip for discussion

@gtanzillo this needs a few more test cases and a more general solution, but opening so we can discuss a few problem areas

@miq-bot add-label core, enhancement, wip
@miq-bot assign @gtanzillo

Resolves https://www.pivotaltracker.com/story/show/148794877

@@ -416,6 +416,7 @@ def get_belongsto_filter_object_ids(klass, filter)
end

def get_managed_filter_object_ids(scope, filter)
return scope.where(filter.to_sql.first) if filter.kind_of?(MiqExpression)
Copy link
Contributor

Choose a reason for hiding this comment

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

this MiqExpression will be always convertible to sql ?

Copy link
Member

Choose a reason for hiding this comment

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

@lpichler, yes, these expressions will be limited to tags on the main class only and will always be convertible to SQL.

@miq-bot
Copy link
Member

miq-bot commented Aug 2, 2017

This pull request is not mergeable. Please rebase and repush.

@imtayadeway imtayadeway changed the title [WIP] Add MiqExpression support for managed filters Add MiqExpression support for managed filters Aug 7, 2017
@imtayadeway
Copy link
Contributor Author

@miq-bot rm-label wip

@miq-bot miq-bot removed the wip label Aug 7, 2017
@miq-bot
Copy link
Member

miq-bot commented Aug 7, 2017

Checked commits imtayadeway/manageiq@639ee18~...33a722b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

Looks awesome 👍

@@ -351,9 +351,11 @@ def get_self_service_objects(user, miq_group, klass)

def calc_filtered_ids(scope, user_filters, user, miq_group, scope_tenant_filter)
klass = scope.respond_to?(:klass) ? scope.klass : scope
expression = miq_group.try(:entitlement).try(:filter_expression)
Copy link
Member

Choose a reason for hiding this comment

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

You probably don't need the try for entitlement. In practice, every group should have an entitlement. Since removing the try will cause tests to fail, this can be done in a followup PR.

u_filtered_ids = pluck_ids(get_self_service_objects(user, miq_group, klass))
b_filtered_ids = get_belongsto_filter_object_ids(klass, user_filters['belongsto'])
m_filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, user_filters['managed']))
m_filtered_ids = pluck_ids(get_managed_filter_object_ids(scope, expression || user_filters['managed']))
Copy link
Member

Choose a reason for hiding this comment

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

Should the the Entitlement model enforce setting only one of expression or user_filters?

@@ -416,6 +416,7 @@ def get_belongsto_filter_object_ids(klass, filter)
end

def get_managed_filter_object_ids(scope, filter)
return scope.where(filter.to_sql.first) if filter.kind_of?(MiqExpression)
Copy link
Member

Choose a reason for hiding this comment

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

@lpichler, yes, these expressions will be limited to tags on the main class only and will always be convertible to SQL.

@gtanzillo gtanzillo added this to the Sprint 67 Ending Aug 21, 2017 milestone Aug 9, 2017
@gtanzillo gtanzillo merged commit d8e59f5 into ManageIQ:master Aug 9, 2017
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.

4 participants