fixes #6488 - enable autosearch completion on Katello model filters #4385

stbenjam · 2014-07-03T11:28:43Z

First shot at this, I've got some questions:

Should the AutoCompleteSearch go through the API controllers instead? I've had to create normal controllers for the models to handle the requests to keep the Foreman style of URL's. Although with fixes #5753: Engines can now override autocomplete path used in FiltersHelper theforeman/foreman#1452 we can route it wherever.
app/controllers/katello/auto_complete_search.rb - this isn't significantly different from Foreman's, could we use theirs instead? The only difference is 'filter' feature, but I don't see it used anywhere...? Also shouldn't it be a concern?
What are all these auto_complete methods everywhere? Intended for the angular views it seems? Shouldn't we try to use the same style as Foreman for searching?

stbenjam · 2014-07-03T13:04:08Z

[test]

ehelms · 2014-07-03T13:16:34Z

On Thu, Jul 3, 2014 at 7:28 AM, Stephen Benjamin notifications@github.com
wrote:

First shot at this, I've got some questions:

Should the AutoCompleteSearch go through the API controllers instead?
I've had to create normal controllers for the models to handle the requests
to keep the Foreman style of URL's. Although with fixes #5753: Engines can now override autocomplete path used in FiltersHelper theforeman/foreman#1452
fixes #5753: Engines can now override autocomplete path used in FiltersHelper theforeman/foreman#1452 we can route it
wherever.

I tend to prefer as much as possible being anchored on the API. That being
said, if this has no use for an API user, then that pushes the argument
towards UI only controllers.

app/controllers/katello/auto_complete_search.rb - this isn't
significantly different from Foreman's, could we use theirs instead? The
only difference is 'filter' feature, but I don't see it used anywhere...?
Also shouldn't it be a concern?

If you can, feel free to consolidate on a single method to handle
autocomplete (i.e. make use of the Foreman one). Ours was written many
years ago before the idea of concerns was being used in the Rails world.
The filter feature extends from the fact that Katello made use of
Elasticsearch to do autocomplete originally (and still does if you go and
use Content Search for example). If possible, a single autocomplete
controller would be nice.

What are all these auto_complete methods everywhere? Intended for the
angular views it seems? Shouldn't we try to use the same style as Foreman
for searching?

There is a much larger issue around what to use for searching. Scoped
search / AR vs Elasticsearch. I wouldn't try to solve that here. For the
permissions autocomplete, using the Foreman method seems sane for
consistency.

You can merge this Pull Request by running

git pull https://github.com/stbenjam/katello autocomplete

Or view, comment on, or merge it at:

#4385
Commit Summary

fixes Fixes #17211 - optimize capsule sync on repo sync #6435 enable autosearch completion on Katello model filters

File Changes

A app/controllers/katello/activation_keys_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-0 (18)

M app/controllers/katello/application_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-1 (14)

M app/controllers/katello/auto_complete_search.rb
https://github.com/Katello/katello/pull/4385/files#diff-2 (18)

M app/controllers/katello/content_views_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-3 (1)

A app/controllers/katello/gpg_keys_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-4 (18)

A app/controllers/katello/host_collections_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-5 (18)

A app/controllers/katello/kt_environments_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-6 (21)

M app/controllers/katello/products_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-7 (2)

A app/controllers/katello/sync_plans_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-8 (18)

A app/controllers/katello/systems_controller.rb
https://github.com/Katello/katello/pull/4385/files#diff-9 (18)

M app/models/katello/activation_key.rb
https://github.com/Katello/katello/pull/4385/files#diff-10 (2)

M app/models/katello/content_view.rb
https://github.com/Katello/katello/pull/4385/files#diff-11 (3)

M app/models/katello/host_collection.rb
https://github.com/Katello/katello/pull/4385/files#diff-12 (3)

M app/models/katello/kt_environment.rb
https://github.com/Katello/katello/pull/4385/files#diff-13 (3)

M app/models/katello/product.rb
https://github.com/Katello/katello/pull/4385/files#diff-14 (3)

M app/models/katello/sync_plan.rb
https://github.com/Katello/katello/pull/4385/files#diff-15 (1)

M app/models/katello/system.rb
https://github.com/Katello/katello/pull/4385/files#diff-16 (10)

M config/routes.rb
https://github.com/Katello/katello/pull/4385/files#diff-17 (38)

M lib/katello/engine.rb
https://github.com/Katello/katello/pull/4385/files#diff-18 (1)

M lib/katello/permissions/activation_key_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-19 (2)

M lib/katello/permissions/content_host_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-20 (1)

M lib/katello/permissions/content_view_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-21 (2)

M lib/katello/permissions/gpg_key_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-22 (2)

M lib/katello/permissions/host_collections_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-23 (3)

M lib/katello/permissions/lifecycle_environment_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-24 (1)

M lib/katello/permissions/product_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-25 (2)

M lib/katello/permissions/sync_plan_permissions.rb
https://github.com/Katello/katello/pull/4385/files#diff-26 (2)

A lib/katello/search_path_overrides.rb
https://github.com/Katello/katello/pull/4385/files#diff-27 (18)

Patch Links:

https://github.com/Katello/katello/pull/4385.patch

https://github.com/Katello/katello/pull/4385.diff

—
Reply to this email directly or view it on GitHub
#4385.

stbenjam · 2014-07-04T07:38:05Z

Ok, thanks! That's perfect. I've moved it all to use the Foreman AutoCompleteSearch controller. The code's ready for review when you have time.

parthaa · 2014-07-08T17:42:05Z

@stbenjam Were you planning to add any tests for some of the controller actions you added ?

parthaa · 2014-07-08T17:42:57Z

app/controllers/katello/application_controller.rb

-      'katello/dashboard'
+      'katello/sync_management',
+      'katello/sync_plans',
+      'katello/systems'


appreciate the alphabetic ordering :). Makes things easier to read ..

parthaa · 2014-07-08T18:13:57Z

I was just chatting about this with @ehelms . I see a lot of controllers with this kinda code

module Katello
   class ActivationKeysController < Katello::ApplicationController
     include Foreman::Controller::AutoCompleteSearch

   end
 end

I was wondering if it would be possible for you to create 1 controller, but create many routes and point their routes to this controller?

stbenjam · 2014-07-08T18:40:43Z

@parthaa Could do, I assumed the best practice would've been to have one named controller per one named model, I've rarely seen it done otherwise. But I could use a single controller for all the models.

parthaa · 2014-07-08T20:46:05Z

Somehow creating 5 empty controllers only just for that one text search box in the permissions filters page seems like an over kill for me.. May we could group all the auto complete search stuff pointing to that one controller. @ehelms , @jlsherrill what do you guys think ?

stbenjam · 2014-07-08T20:54:38Z

I agree, it does seem unnecessary. I've got something like this in routes.rb now, still working on ironing out the details, but it does reduce this all to 1 controller. Is that the right direction?

  %w(activation_keys content_views gpg_keys content_hosts
     environments host_collections sync_plans).each do |path|

    model = case path
      when 'content_hosts'
        'System'
      when 'environments'
        'KTEnvironment'
      else
        path.singularize.camelize
    end

    match "#{path}/auto_complete_search",
      :controller => :auto_complete_search,
      :action     => :auto_complete_search,
      :model      => model
  end

jlsherrill · 2014-07-08T21:50:40Z

could you do something like:

match '/:model/auto_complete_search', :action => :auto_complete_search, :controller => :auto_complete_search, :via => :get

and then just in the controller

"Katello::#{param[:model].singularize.camelcase}".constantize"

To get the model class from there. Not sure if that's too hacky for some :) It is a lot shorter, and just works for new models. If you wanted to limit it only to certain models, you could simply have a name to lookup conversion in the controller:

{
'activation_keys' => ActivationKey,
'content_views' => ContentView
}

jlsherrill · 2014-07-08T21:53:11Z

Oh yeah forgot about the ones where the route doesn't match the model name. So my example is nearly the same as yours, the logic just goes to the controller. I like it better in the controller (i prefer routes with as little logic as possible), but others may disagree.

parthaa · 2014-07-08T21:53:56Z

+1 to @jlsherrill 's idea . I like the lookup much better .. Constantize too hacky. Lookup and raise a 404 if it cant find the model.

parthaa · 2014-07-08T21:54:41Z

You can then write 1 test for this autocomplete search controller and be done :)

stbenjam · 2014-07-09T10:36:19Z

@parthaa @jlsherrill Thanks guys! There's some sorcery needed to get permissions working, still working on that, I'll let you know when it's ready to review again :-)

stbenjam · 2014-07-10T10:21:41Z

app/controllers/katello/application_controller.rb

  def authorized
    if converted_controllers.include?(request.params['controller'])
-      super
+      if self.respond_to? :permission_controller
+        User.current.allowed_to?(params.slice(:action, :id).merge(:controller => permission_controller))


The idea here is so that permissions can be evaluated against an arbitrary target defined by the controller -- e.g. content_hosts#auto_complete_search, not auto_complete_search#auto_complete_search.

stbenjam · 2014-07-10T10:22:04Z

@partha Ok, have a look! :-)

parthaa · 2014-07-11T14:19:52Z

ACK from me.... @jlsherrill - do you want go over ?

jlsherrill · 2014-07-21T15:23:14Z

The code itself looks very good and concise! +1. One thing i noted was that when using auto complete with katello items it seemed to ignore my selected org and show me items in all orgs. Foreman objects like 'Host/Managed' did not seem to do this.

stbenjam · 2014-07-22T14:23:49Z

@jlsherrill Good find. This is fixable by adding a default scope to the model. Something like:

default scope { readable.where(:organization_id => Organization.current.id) }

However, this breaks the API controllers. I would prefer to address this as a separate issue, ideally by making Katello use the same taxonomy configuration as Foreman -- we seem to have done our own thing, which causes problems when we try to integrate into Foreman things like this.

Although, I was trying it out today, and it just doesn't work right (even if you reach the point where Rails starts), there seems to be a lot of issues with taxonomies in general (many stemming from this nested stuff). Has anyone tried this before with Katello?

ehelms · 2014-08-06T11:40:47Z

@stbenjam I think for now, baking checking of Organzation.current into the autocomplete will be the best approach as all other choices appear to involve re-factoring a section of other areas. Could you also rebase/fix tests?

stbenjam · 2014-08-06T12:55:46Z

@ehelms Done, but that only partially works. I can only do the top most layer, e.g on Activation Keys, "name =" is properly confined to the current org if there is one. However, on a Content View, querying "activation_key = " is not and you'll see them all.

ehelms · 2014-08-15T18:03:49Z

@stbenjam sorry for the delay in getting back to you - I think that could be OK for now, as long as we open an issue to track respecting organization's on associations.

stbenjam · 2014-09-05T12:27:54Z

@ehelms Not really a good way to fix it, so I removed the scoped searches on associations.

There's already an issue to enable this: http://projects.theforeman.org/issues/4638, which we can do when we fix the problem with searching in associations.

stbenjam · 2014-09-05T12:28:08Z

And current failure is due to Koji disk space.

ehelms · 2014-09-05T13:17:24Z

[test]

ehelms · 2014-09-08T13:18:06Z

ACK for me

jlsherrill · 2014-09-09T18:16:16Z

ACK from me

stbenjam · 2014-09-10T08:17:42Z

@ehelms Needed a rebase due to #4635 (made my changes to lib/katello/tasks/test.rake unneccessary). Do you want to re-ACK?

fixes #6488 - enable autosearch completion on Katello model filters

stbenjam changed the title ~~fixes #6435 enable autosearch completion on Katello model filters (WIP, don't merge yet)~~ fixes #6488 enable autosearch completion on Katello model filters (WIP, don't merge yet) Jul 3, 2014

stbenjam changed the title ~~fixes #6488 enable autosearch completion on Katello model filters (WIP, don't merge yet)~~ fixes #6488 - enable autosearch completion on Katello model filters Jul 4, 2014

parthaa reviewed Jul 8, 2014
View reviewed changes

stbenjam reviewed Jul 10, 2014
View reviewed changes

ehelms added the Needs re-review label Jul 29, 2014

ehelms added Waiting on contributor and removed Needs re-review labels Aug 6, 2014

fixes #6488 enable autosearch completion on Katello model filters

ec74eaf

stbenjam removed the Waiting on contributor label Sep 10, 2014

stbenjam added a commit that referenced this pull request Sep 11, 2014

Merge pull request #4385 from stbenjam/autocomplete

09d306e

fixes #6488 - enable autosearch completion on Katello model filters

stbenjam merged commit 09d306e into Katello:master Sep 11, 2014

stbenjam deleted the autocomplete branch September 18, 2014 19:11

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #6488 - enable autosearch completion on Katello model filters #4385

fixes #6488 - enable autosearch completion on Katello model filters #4385

stbenjam commented Jul 3, 2014

stbenjam commented Jul 3, 2014

ehelms commented Jul 3, 2014

stbenjam commented Jul 4, 2014

parthaa commented Jul 8, 2014

parthaa Jul 8, 2014

parthaa commented Jul 8, 2014

stbenjam commented Jul 8, 2014

parthaa commented Jul 8, 2014

stbenjam commented Jul 8, 2014

jlsherrill commented Jul 8, 2014

jlsherrill commented Jul 8, 2014

parthaa commented Jul 8, 2014

parthaa commented Jul 8, 2014

stbenjam commented Jul 9, 2014

stbenjam Jul 10, 2014

stbenjam commented Jul 10, 2014

parthaa commented Jul 11, 2014

jlsherrill commented Jul 21, 2014

stbenjam commented Jul 22, 2014

ehelms commented Aug 6, 2014

stbenjam commented Aug 6, 2014

ehelms commented Aug 15, 2014

stbenjam commented Sep 5, 2014

stbenjam commented Sep 5, 2014

ehelms commented Sep 5, 2014

ehelms commented Sep 8, 2014

jlsherrill commented Sep 9, 2014

stbenjam commented Sep 10, 2014

fixes #6488 - enable autosearch completion on Katello model filters #4385

fixes #6488 - enable autosearch completion on Katello model filters #4385

Conversation

stbenjam commented Jul 3, 2014

stbenjam commented Jul 3, 2014

ehelms commented Jul 3, 2014

stbenjam commented Jul 4, 2014

parthaa commented Jul 8, 2014

parthaa Jul 8, 2014

Choose a reason for hiding this comment

parthaa commented Jul 8, 2014

stbenjam commented Jul 8, 2014

parthaa commented Jul 8, 2014

stbenjam commented Jul 8, 2014

jlsherrill commented Jul 8, 2014

jlsherrill commented Jul 8, 2014

parthaa commented Jul 8, 2014

parthaa commented Jul 8, 2014

stbenjam commented Jul 9, 2014

stbenjam Jul 10, 2014

Choose a reason for hiding this comment

stbenjam commented Jul 10, 2014

parthaa commented Jul 11, 2014

jlsherrill commented Jul 21, 2014

stbenjam commented Jul 22, 2014

ehelms commented Aug 6, 2014

stbenjam commented Aug 6, 2014

ehelms commented Aug 15, 2014

stbenjam commented Sep 5, 2014

stbenjam commented Sep 5, 2014

ehelms commented Sep 5, 2014

ehelms commented Sep 8, 2014

jlsherrill commented Sep 9, 2014

stbenjam commented Sep 10, 2014