Fixes #17526 - Taxonomy.ignore? does not work with "any context" #4070
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
|
@dLobatog, thanks for your PR! By analyzing the history of the files in this pull request, we identified @isratrade, @ares and @abenari to be potential reviewers. |
houndci-bot
reviewed
Nov 30, 2016
| should_not allow_value(' ').for(:name) | ||
| # Taxonomy 1 is already used by a fixture | ||
| should_not allow_value("#{taxonomy_name} 1").for(:name) | ||
| should_not allow_value("#{'a' * 256}").for(:name) |
There was a problem hiding this comment.
Prefer to_s over string interpolation.
dLobatog
commented
Nov 30, 2016
| assert_equal _("is too long (maximum is 0 characters)"), taxonomy.errors[:name].first | ||
| end | ||
|
|
||
| test 'ignores the taxable_type if current taxonomy ignores it' do |
There was a problem hiding this comment.
These two are the only new tests, the rest of them are just removing the duplication between location_test and organization_test and moving them both to a module
dLobatog
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
|
Tons of tests broke, setting to WoC |
In this scenario:
1. User has one organization - "E-Corp" - this organization allows the
user to see every hostgroup (via selecting "all host groups" when
editing the organization).
2. User has a location "Czech republic" that also includes all host
groups through that checkbox.
3. User visits /hostgroups with "any context" selected, or "E-Corp/any
location", or "any organization/Czech republic". The result is that the
user does not see all host groups even though there's an
organization/location combination (e-corp/czech republic) that should
allow the user to see all host groups.
---
The reason is that when `Hostgroup.taxable_ids` is called
`Organization.ignore?` does not realize that there it should look in all
Organizations to see if any of them 'ignores' (has "all host groups"
checked) the resource. The same thing happens with Locations.
The fix is to make `ignore?` aware the 'Organization.current == nil'
means 'Any organization', not 'No organization'.
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
@theforeman-bot ok to test |
|
Test failure in Katello is due to https://github.com/Katello/katello/pull/6480/files |
|
Thanks @dLobatog ! |
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.
In this scenario:
User has one organization - "E-Corp" - this organization allows the
user to see every hostgroup (via selecting "all host groups" when
editing the organization).
User has a location "Czech republic" that also includes all host
groups through that checkbox.
User visits /hostgroups with "any context" selected, or "E-Corp/any
location", or "any organization/Czech republic". The result is that the
user does not see all host groups even though there's an
organization/location combination (e-corp/czech republic) that should
allow the user to see all host groups.
The reason is that when
Hostgroup.taxable_idsis calledOrganization.ignore?does not realize that there it should look in allOrganizations to see if any of them 'ignores' (has "all host groups"
checked) the resource. The same thing happens with Locations.
The fix is to make
ignore?aware the 'Organization.current == nil'means 'Any organization', not 'No organization'.