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 #23918 - Find correct scope when updating taxonomy #5768

Merged
merged 1 commit into from Jul 10, 2018

Conversation

xprazak2
Copy link
Contributor

@xprazak2 xprazak2 commented Jul 4, 2018

resource_scope caches the scope, on update the @resource_scope is [] (no idea how that happens), but scope_for fetches the correct scope.

@theforeman-bot
Copy link
Member

Issues: #23918

Copy link
Member

@dLobatog dLobatog left a comment

Choose a reason for hiding this comment

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

@xprazak2 It looks like it fixes the issue with hammer but could you add a test? It is unclear to me how this broke and I'm afraid we will break it again.

Thank you!

@@ -88,7 +88,7 @@ def destroy

# overriding public FindCommon#resource_scope to scope only to user's taxonomies
def resource_scope(*args)
super.send("my_#{taxonomies_plural}")
@resource_scope = scope_for(resource_class, args).send("my_#{taxonomies_plural}")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be ||= too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not see a reason why there is a ||= in the first place, what if the cached data is stale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

||= seems to work here as well, so I made the change.

@xprazak2
Copy link
Contributor Author

xprazak2 commented Jul 9, 2018

I added test and changed = to ||=.

@dLobatog
Copy link
Member

hammer -u dlobatog -p changeme  organization add-location --id=15 --location-id=16
The location has been associated

Looks like it works perfectly. Thanks for the test!

cc @jhutar @sghai

@dLobatog dLobatog merged commit 3ab5660 into theforeman:develop Jul 10, 2018
@sghai
Copy link

sghai commented Jul 11, 2018

@dLobatog @xprazak2 : thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants