-
Notifications
You must be signed in to change notification settings - Fork 900
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
Refactor combine_filtered_ids in RBAC #15346
Merged
gtanzillo
merged 12 commits into
ManageIQ:master
from
lpichler:refactor_combine_filtered_ids
Jun 14, 2017
Merged
Refactor combine_filtered_ids in RBAC #15346
gtanzillo
merged 12 commits into
ManageIQ:master
from
lpichler:refactor_combine_filtered_ids
Jun 14, 2017
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lpichler
force-pushed
the
refactor_combine_filtered_ids
branch
from
June 12, 2017 13:01
2fed708
to
e260821
Compare
There are operations like intersection and union in the algorithm. But these operations are not same as we know from maths. They can recieve also nil as operand and it means: don't use this operand and don't apply operator here. For example mentioned operators works in this way: [1] INTERSECTION [1, 2] = [1] [] INTERSECTION [1, 2] = [] nil INTERSECTION [1, 2] = [1, 2] operands represents filter as array of ids, when filter is not array but nil it means don't apply the filter
put intersection of belongs_to and managed filters to one result
These operations are not applying the operation when any operand is nil and when it is nil the other operand is returned: intersection.call([], [1,2]) => [] intersection.call(nil, [1,2]) => [1]
b_intersection_m = (b_filtered_ids INTERSECTION m_filtered_ids) is b_intersectionn_m = intersection.call(b_filtered_ids, m_filtered_ids)
and rewrite this line: u_union_d_union_b_and_m = u_filtered_ids UNION d_filtered_ids UNION b_intersection_m to u_union_d_union_b_intersection_m = union.call(u_filtered_ids, d_filtered_ids, b_intersectionn_m)
filter = u_union_d_union_b_and_m INTERSECTION tenant_filter_ids to intersection.call(u_union_d_union_b_intersection_m, tenant_filter_ids)
lpichler
force-pushed
the
refactor_combine_filtered_ids
branch
from
June 12, 2017 13:04
e260821
to
ac0473d
Compare
Checked commits lpichler/manageiq@6b110ad~...ac0473d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
gtanzillo
approved these changes
Jun 14, 2017
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.
This is a really elegant change!! π
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Goal β½
make method
combine_filtered_ids
more understandableMotivation π‘
there is needed some change and it was hard to do it with the previous state.
Description π
This method contained a lot of
ifs
and it was confusing.See comments in commits for the whole story.
@miq-bot add_label refactoring, rbac
cc @kbrock
@miq-bot assign @gtanzillo