Fixes #8699 - Scoped search for system #5541
Conversation
| @@ -20,7 +21,7 @@ class Api::V2::SystemsController < Api::V2::ApiController | |||
|
|
|||
| before_filter :find_environment_and_content_view, :only => [:create] | |||
| before_filter :find_content_view, :only => [:create, :update] | |||
| before_filter :load_search_service, :only => [:index, :available_host_collections, :tasks] | |||
| before_filter :load_search_service, :only => [:index, :available_host_collections, :tasks, :auto_complete_search] | |||
There was a problem hiding this comment.
We shouldn't need to load the search service here if we are switching to scoped search.
There was a problem hiding this comment.
This comment is still unaddressed
|
Shouldn't there be removal of files such as https://github.com/Katello/katello/blob/master/app/models/katello/glue/elastic_search/system.rb included in this PR? |
| @@ -244,5 +226,20 @@ def test_product_content | |||
| assert_response :success | |||
| assert_template 'api/v2/systems/product_content' | |||
| end | |||
|
|
|||
There was a problem hiding this comment.
Will need to remove loading of the fake search service in this test class as well.
johnpmitsch
force-pushed
the
contenthost
branch
2 times, most recently
from
b52cfa6
to
ee459ce
Compare
| } | ||
| respond_for_index(:collection => item_search(System, params, options)) | ||
| environment_ids << params[:environment_id].to_i if params[:environment_id] | ||
| collection = collection.where(:id => KTEnvironment.where(:id => environment_ids).map(&:systems).flatten.map(&:id)) if environment_ids.any? |
There was a problem hiding this comment.
You should be able to just do a where with the environment_id directly here since systems has a belongs to relationship with environment.
johnpmitsch
force-pushed
the
contenthost
branch
from
ee459ce
to
6bd7edb
Compare
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
| @@ -5,7 +5,6 @@ class AutoAttachSubscriptions < Actions::EntryAction | |||
| middleware.use ::Actions::Middleware::RemoteAction | |||
|
|
|||
| def plan(system) | |||
| system.disable_auto_reindex! | |||
| action_subject system | |||
| plan_action(::Actions::Candlepin::Consumer::AutoAttachSubscriptions, system) if ::SETTINGS[:katello][:use_cp] | |||
| plan_action(ElasticSearch::Reindex, system) if ::SETTINGS[:katello][:use_elasticsearch] | |||
johnpmitsch
force-pushed
the
contenthost
branch
2 times, most recently
from
a16da24
to
81465f0
Compare
|
@ehelms @jlsherrill updated |
| :through => :system_activation_keys, | ||
| :after_add => :add_activation_key, | ||
| :after_remove => :remove_activation_key | ||
| :through => :system_activation_keys | ||
|
|
There was a problem hiding this comment.
Move this up to line 35 so it's not dangling
johnpmitsch
force-pushed
the
contenthost
branch
from
81465f0
to
6612cdc
Compare
johnpmitsch
force-pushed
the
contenthost
branch
from
d992cc8
to
99489ba
Compare
|
@jlsherrill ah ok, took me a minute to catch up to your train of thought :) I removed that line and added the scoped search on environment. |
|
Getting an error when registering a client with subscription-manager, looks like the system create and update actions need updating, such as: https://github.com/Katello/katello/blob/master/app/lib/actions/katello/system/create.rb#L45 (also the system.disable_auto_reindex! at the top) |
johnpmitsch
force-pushed
the
contenthost
branch
from
99489ba
to
f5f119e
Compare
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
|
@jlsherrill updated to fix errors when registering a client |
|
Since you are removing all usage of hooks, mind removing it from the katello.gemspec? |
|
Other than those two item, (auto complete on role filters ,and removal of hooks gem dep), it all looks good to me. I also tried merging this as well as #5536 together and the conflicts were very minimal, so I'm okay with going ahead and merging once these two things are fixed. |
jlsherrill
added
Waiting on contributor
and removed
Needs re-review
Needs testing
labels
|
@jlsherrill does #5536 have to be merged first? I am adding dynflow to that right now. |
johnpmitsch
force-pushed
the
contenthost
branch
from
f5f119e
to
4e22aed
Compare
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
|
@jlsherrill @cfouant updated |
|
In systems bulk actions controller please remove |
|
in system.rb please remove |
added autocomplete
johnpmitsch
force-pushed
the
contenthost
branch
from
4e22aed
to
e01a907
Compare
|
@cfouant updated to remove those |
|
Thanks! It worked great for me. ACK pending a senior engineer review |
|
ACK from me, and i say go ahead and merge, the conflict with #5536 is minimal. |
Fixes #8699 - Scoped search for system
No description provided.