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 #33342 - plugins can avoid lone taxonomies #8739

Closed
wants to merge 1 commit into from

Conversation

lzap
Copy link
Member

@lzap lzap commented Aug 25, 2021

Some model classes treat taxonomies differently and therefore API base controller filter is not applicable. This patch introduces a mechanism (an empty mixin) to opt-in.

@theforeman-bot
Copy link
Member

Issues: #33342

@xprazak2
Copy link
Contributor

Nice, though I am not sure if it should be in core or handled in a plugin. We had a fix in openscap which was recently removed - it was no longer needed because ArfReport < Report so if we remove the inheritance, maybe we could bring it back.

@xprazak2
Copy link
Contributor

xprazak2 commented Sep 2, 2021

@lzap, rubocop needs a fix if you plan to move forward here

@lzap
Copy link
Member Author

lzap commented Sep 2, 2021

I plan to remove STI from SCAP report table however since I have no idea what lone taxonomies are, I was unsure what to do. So I simply went ahead and created this empty module.

Rebased. I'd appreciate guidance here, if you think there is a better solution let's do it!

@xprazak2
Copy link
Contributor

xprazak2 commented Sep 6, 2021

@tbrisker, is this something worth having in core?

@tbrisker
Copy link
Member

tbrisker commented Oct 3, 2021

Some background: This callback is used when there is only one location or organization and a resource is created via the API. In that case, even if the user doesn't specify the taxonomy, it gets assigned to the resource. This was needed when we removed the option to disable taxonomies, so that the same behaviour is preserved when people only use the default taxonomies and automation doesn't need changes.

What might be a better solution is to not assign them when they are a through relation, maybe something like:

  if resource_class.reflections.has_key? tax_name.pluralize
    next if resource_class.reflections[tax_name.pluralize].options.has_key? :through
...
  elsif resource_class.reflections.has_key? tax_name
    next if resource_class.reflections[tax_name].options.has_key? :through

I'm not 100% certain about that, but it does make some sense to me when the taxonomy isn't directly assigned that the api shouldn't assign it automatically as well.

@lzap
Copy link
Member Author

lzap commented Mar 2, 2022

I am not sure what am I doing here, I've been told to do this kind of patch. Closing.

@lzap lzap closed this Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants