fixes #8690 - converting repository to scoped search

[jlsherrill]

No description provided.

[jlsherrill]

Note, as part of this PR, i convert the content view 'add repositories' page to use a param instead of search. After playing around, search did not seem to work well for this use case. So here i present two variations to solving the issue:

1. Simply adds an available_to_content_view param:

jlsherrill@51165a3#diff-4a58e16252386909489b00a2f4451879R67

2. A bit more advanced adds an 'available_for' param that when used, and set to the value of 'content_view' changes the meaning of the content_view_id param, from one of 'what repos are associated with' to one of 'what repos are available to be associated with'

06f134c#diff-4a58e16252386909489b00a2f4451879R62

I am open to other solutions as well. Will squash and write a test to cover it after discussion.

[ehelms]

On Thu, Jun 11, 2015 at 11:17 PM, Justin Sherrill notifications@github.com
wrote:

Note, as part of this PR, i convert the content view 'add repositories'
page to use a param instead of search. After playing around, search did not
seem to work well for this use case. So here i present two variations to
solving the issue:

1. Simply adds an available_to_content_view param:

jlsherrill/katello@51165a3#diff-4a58e16252386909489b00a2f4451879R67
jlsherrill@51165a3#diff-4a58e16252386909489b00a2f4451879R67

What about available_for_content_view? Reads better to me.

2. A bit more advanced adds an 'available_for' param that when used, and
   set to the value of 'content_view' changes the meaning of the
   content_view_id param, from one of 'what repos are associated with' to one
   of 'what repos are available to be associated with'

06f134c#diff-4a58e16252386909489b00a2f4451879R62
06f134c#diff-4a58e16252386909489b00a2f4451879R62

What are your thoughts on the second approach being more automatic? For
example, available_for=content_view_id where the code takes whatever
parameter is passed in for available_for, looks for that parameter in the
params and then performs the 'negation'. I realize this still couldn't work
for some parameters, and would only allow negation of a single parameter at
a time; however, without that I am not sure I see an advantage over method
1 that provides an explicit parameter. The plus side to this approach would
be you can take an existing query, and add a parameter to negate instead of
having to craft the query fresh.

—
Reply to this email directly or view it on GitHub
#5294 (comment).

[jlsherrill]

The negation is a lot trickier than what you'd expect. Take this example from the repos controller:

     elsif params[:content_view_id]
        query = query.joins(:content_view_repositories).where("#{ContentViewRepository.table_name}.content_view_id" => params[:content_view_id])
      end

To negate that via sql-logic, you'd say something like:

     elsif params[:content_view_id]
        query = query.joins(:content_view_repositories).where("#{ContentViewRepository.table_name}.content_view_id != #{params[:content_view_id]}")
      end

You'd expect that would return you the opposite of the first query, but in fact it does not simply because the join() here is an inner join and excludes all things that are not associated to a content view at all. Sure you could re-write this query to use an inner join and negate that, but what you are suggesting would require rewriting most all of our filter queries to use 'in clauses', which might be an option if it not for....

Available repos for a content view isn't simply the inverse of repos already associated to the content view. You have to consider org, only the repos in Library (default org view), etc... (notice here i set params[:library] = true, otherwise i'd get the literal inverse which would be wayyy too much.) In most other situations the same will happen, available_for is not simply the inverse of the initial query as there are other things control for.

[jlsherrill]

Yeah, i agree, if we go that route, i'd change it to 'available_for_content_view', i started using available_for for the 2nd route cause it sounded better and just didn't go back and change the initial param

[ehelms]

I see your point. Thoughts on trying to capture the parameter logic into a function similar to how the content concern works? Something like:

filter_by_content_view(params[:content_view_id], params[:available_for] == 'content_view')

[jlsherrill]

@ehelms updated and added a test

[ehelms]

Probably not a symptom of this change, but when listing docker repositories for a content view version I see not just the library version listed.

Edit: In other words, I see both the library version and the one that I just created by publishing a version.

[ehelms]

Do you think we should change the repository list page to use normal search instead of filter with this update?

[jlsherrill]

@ehelms i do think we should add that, but it would be best to just re-write that page to use the details-nutupane template, rather than modifying the existing code and i'd rather do that in a separate PR

[ehelms]

I thought with past conversions we had updated the pages to take advantage of the conversion, if you feel strongly enough in this case that is should be a stand-alone update please file an issue.

[jlsherrill]

@ehelms generally they are already using nutupane or nutupane-details and no conversion has been needed. I started to do the conversion but with all the interaction and given the size of this PR already I didn't feel comfortable doing it as part of this PR.

Opened http://projects.theforeman.org/issues/10837

[jlsherrill]

squashed

[ehelms]

Needs removing

[ehelms]

I think some of the work here, the UI portion (https://github.com/Katello/katello/pull/5268/files), may have some coincidence with this update. You may want to review it, and perhaps consider getting it merged and then rebasing this PR as the autocomplete nature would be able to be added fairly easily?

[jlsherrill]

@ehelms tested that PR ontop of mine, but i don't know that they need to applied in any order, they don't conflict and the auto complete works just fine with his patch as is (it works better after the masterOnly change i suggested)

[ehelms]

@jlsherrill thanks for giving it a try in conjunction with this, one last comment (#5294 (comment)) and I think I am good with this.

[ehelms]

ACK