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 #16798 - move scoped_search definitions to STI subclasses #3917

Merged
merged 1 commit into from Nov 14, 2016

Conversation

domcleal
Copy link
Contributor

@domcleal domcleal commented Oct 5, 2016

scoped_search doesn't support class inheritance with STI, so registering
definitions on the subclass fixes various issues. This fixes an issue
where scoped_search on CommonParameter calls Parameter.all and is
returned a list of CommonParameters under Rails 4.2, as it relies on a
bug (#18806) for applying the type='CommonParameter' condition when
calling Parameter.all.

On Rails 5.0, this bug was fixed and calling Parameter.all within a
scope on CommonParameter now returns all types of parameters.
Registering all scoped_search definitions on the subclasses ensures that
scoped_search calls CommonParameter.all instead.

The taxonomies API changed as it called Taxonomix objects with scopes
such as .locations.search_for, which no longer existed when the
scoped_search definitions were removed from Taxonomy (.locations is a
Taxonomy association with a where clause). This now explicitly searches
via the appropriate Taxonomy subclass.

scoped_search :on => :name, :complete_value => true
def self.inherited(child)
child.instance_eval do
scoped_search :on => :name, :complete_value => true
Copy link
Member

Choose a reason for hiding this comment

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

this won't be necessary anymore I think

Copy link
Contributor Author

@domcleal domcleal Oct 5, 2016

Choose a reason for hiding this comment

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

Thanks, I've removed the search definitions from CommonParameter and added the :value definition here.

scoped_search doesn't support class inheritance with STI, so registering
definitions on the subclass fixes various issues. This fixes an issue
where scoped_search on CommonParameter calls Parameter.all and is
returned a list of CommonParameters under Rails 4.2, as it relies on a
bug (#18806) for applying the type='CommonParameter' condition when
calling Parameter.all.

On Rails 5.0, this bug was fixed and calling Parameter.all within a
scope on CommonParameter now returns all types of parameters.
Registering all scoped_search definitions on the subclasses ensures that
scoped_search calls CommonParameter.all instead.

The taxonomies API changed as it called Taxonomix objects with scopes
such as `.locations.search_for`, which no longer existed when the
scoped_search definitions were removed from Taxonomy (.locations is a
Taxonomy association with a where clause). This now explicitly searches
via the appropriate Taxonomy subclass.
dhlavac pushed a commit to dhlavac/foreman that referenced this pull request Nov 7, 2016
Filtering of attributes has moved from the protected_attributes gem to
strong parameters in controller concerns, to be in line with current
Rails recommendations.

Concerns are shared between UI and both API controllers and list the
attributes using Foreman::ParameterFilter, which provides additional
features:

1. Registration of additional attributes from plugins, through the
plugin API or `attr_accessible` for short term compatibility.
2. Re-use of permitted attribute lists for nested models (e.g. host
and interface).
3. Combining of lists of attributes from all sources in a single
permit call.
4. A small DSL for changing accepted parameters based on controller,
action and UI/API type.

Plugins should either temporarily depend on protected_attributes to
continue to protect their models or call permit/use ParameterFilter
similarly: http://projects.theforeman.org/projects/foreman/wiki/Strong_parameters

Some UI changes were required to make nested model hash keys all
integers instead of "new_123456" etc, else strong parameters would
filter the entries out.

The Role model's builtin default has been moved from initialize to the
database as the removal of protected_attrs changed the initialisation
order.
@dLobatog dLobatog merged commit 08763fc into theforeman:develop Nov 14, 2016
@dLobatog
Copy link
Member

Thanks @domcleal @ares !

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