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 #14446 - Respect STI class in taxonomy search #157

Closed
wants to merge 1 commit into from

Conversation

shlomizadok
Copy link
Member

Not sure whether this should go here or in core.
@ares, @ohadlevy thoughts?
Anyhow tested w/ $downstream project and works as expected.

@ohadlevy
Copy link
Member

ohadlevy commented Apr 4, 2016

I would open a refactoring issue to enable this usage case, and until its added, will add the workaround here.
However, if its a oneliner change (change from self.name to self.base_name) maybe just fix it in foreman itself? (at least raise a PR to start the discussion).

(in the larger scope - maybe you don't want to use taxonomix at all).

@ohadlevy
Copy link
Member

ohadlevy commented Apr 4, 2016

Also, I'm missing a test? :)

@shlomizadok
Copy link
Member Author

@ohadlevy not needed since theforeman/foreman#3384 ?
image

@shlomizadok
Copy link
Member Author

@ohadlevy - Test has been added

@ares
Copy link
Member

ares commented Apr 4, 2016

LGTM, sounds as a good fix to core as well, if you open the redmine issue for it, please add a comment with link to this method so it can be tracked and removed later

@shlomizadok
Copy link
Member Author

Not so keen to add to core.
Will open an issue on redmine

@ares
Copy link
Member

ares commented Apr 4, 2016

Not so keen to add to core.

why? isn't this broken for Compute Resources now? Foreman::Model::Ovirt.name and Foreman::Model::Ovirt.base_class.name returns different value

@shlomizadok
Copy link
Member Author

@@ -173,6 +173,17 @@ def destroy
end
end

def self.inner_select(taxonomy, inner_method = which_ancestry_method)
Copy link
Member

Choose a reason for hiding this comment

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

please add a comment "# can be removed after http://projects.theforeman.org/issues/14458 is fixed"

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@shlomizadok
Copy link
Member Author

Closing in favor of theforeman/foreman#3389 which is now merged and was asked to be ported into 1.11

@shlomizadok shlomizadok closed this Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants