Fixes #26226 - Filter Module Streams on CV #8295

parthaa · 2019-08-23T03:44:09Z

This commit adds UI, API and publish bindings for filtering module
streams in a content view.
Check out https://partha.fedorapeople.org/CVModuleFilters/ for the screen shots

theforeman-bot · 2019-08-23T03:44:13Z

Issues: #26226

omkarkhatavkar · 2019-08-23T10:26:39Z

@parthaa I tried quick glance today for UI, found some issues mentioned below.

Module Stream Link with not shown here.

Module Stream Count shown wrong after all modules are applied in exclude filter with having Solve Dependencies is 'Yes'. This is not reproducible if Solve Dependencies is 'No'.

We can get rid of this message as we have module stream filters

Currently, Publishing content views is failing with the below steps

Add all module streams in the filter and publish the content view
Now remove all modules from the filter and publish the filter

Minor UI Issue, while removing module stream list we are showing multiple success message not similar while adding. we should stick only UI success message?

I will be testing this further in detailed.

omkarkhatavkar · 2019-08-26T09:27:47Z

Module Stream Filter Create hangs in UI, not happens with other filters

omkarkhatavkar · 2019-08-26T11:39:48Z

@parthaa One More thing, I was not able to create the content view filter rule. tried some options, can you please that as well.

Option details:
  Following parameters accept format defined by its schema (bold are required):

  --module-streams    "name=string\,stream=string, ... "
hammer> 

hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-streams "name='duck', stream='0'"
Could not create the filter rule:
  Missing arguments for 'module_streams[0][stream]', 'module_streams[1][name]'
hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-streams "name=duck, stream=0"
Could not create the filter rule:
  Missing arguments for 'module_streams[0][stream]', 'module_streams[1][name]'

parthaa · 2019-08-26T13:32:27Z

Option details:
  Following parameters accept format defined by its schema (bold are required):

  --module-streams    "name=string\,stream=string, ... "
hammer> 

hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-streams "name='duck', stream='0'"
Could not create the filter rule:
  Missing arguments for 'module_streams[0][stream]', 'module_streams[1][name]'
hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-streams "name=duck, stream=0"
Could not create the filter rule:
  Missing arguments for 'module_

@omkarkhatavkar I think this should be a separate hammer bug. The --module-streams parameter in the api creates "multiple" rules - one for each module-stream.
We should probably tweak the hammer call to have flags for module name and stream. Something like

hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-name='duck'  --module-stream "0"

theforeman-bot · 2019-08-26T13:37:10Z

There were the following issues with the commit message:

2816e87 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

coveralls · 2019-08-26T13:40:25Z

Coverage remained the same at 58.011% when pulling 770a327 on parthaa:module-stream-filters into 14a8471 on Katello:master.

omkarkhatavkar · 2019-08-26T13:43:51Z

@omkarkhatavkar I think this should be a separate hammer bug. The --module-streams parameter in the api creates "multiple" rules - one for each module-stream.
We should probably tweak the hammer call to have flags for module name and stream. Something like
hammer> content-view filter rule create --name "exclude duck module stream" --content-view-id 9 --content-view-filter-id 24 --module-name='duck'  --module-stream "0" 

Make Sense, Bug raised here https://projects.theforeman.org/issues/27712

theforeman-bot · 2019-08-26T13:44:12Z

There were the following issues with the commit message:

8380fe9 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

This message was auto-generated by Foreman's prprocessor

parthaa · 2019-08-26T14:32:42Z

Module Stream Filter Create hangs in UI, not happens with other filters

addressed

parthaa · 2019-08-26T22:04:46Z

[test katello]

omkarkhatavkar · 2019-08-27T09:36:09Z

@parthaa I saw today another issue really weird one
Steps :

added all module streams in the filter -> publish the filter
remove all module streams from the filter -> publish
Change the Content view Depedancy => Solve 'Yes'
Now try to update module stream filter click on these two tabs

omkarkhatavkar · 2019-08-27T09:43:16Z

Module Stream Link with not shown here.

PASS

Module Stream Count shown wrong after all modules are applied in exclude filter with having Solve Dependencies is 'Yes'. This is not reproducible if Solve Dependencies is 'No'.

Still Failing

We can get rid of this message as we have module stream filters

PASS

Currently, Publishing content views is failing with the below steps

Add all module streams in the filter and publish the content view

Now remove all modules from the filter and publish the filter

Module Stream Filter Create hangs in UI, not happens with other filters

PASS

Minor UI Issue, while removing module stream list we are showing multiple success message not similar while adding. we should stick only UI success message?

Can be Ignored

omkarkhatavkar · 2019-08-29T06:18:36Z

@partha: One change may be required here to make sure that API docs get updated

katello/app/controllers/katello/api/v2/content_view_filters_controller.rb

Line 37 in e3e0f95

    
           param :type, String, :desc => N_("type of filter (e.g. rpm, package_group, erratum, docker)"), :required => true

johnpmitsch · 2019-09-05T13:54:32Z

app/models/katello/module_stream.rb

+      query
+    end
+
+    def self.uniquify_by_name_streams(name_streams)


where is this used?

Good call. Going to remove it (left over stray method from the name/stream implementation).

omkarkhatavkar · 2019-09-09T03:58:35Z

@parthaa please take a look on added automated tests for this PR here SatelliteQE/robottelo#7339 (whenever you get a chance)

johnpmitsch · 2019-09-09T23:57:47Z

@parthaa I wonder if we need the description in the table of module streams in the filter? They can be lengthy and make the list very long and busy:

johnpmitsch · 2019-09-10T00:43:48Z

@parthaa it looks like dependency solving does actually affect module streams?

I'm guessing this is intended, if so, I think we should change some of the dep solving help text in the content view details page, new CV page, and settings page. It currently says:
This will solve RPM dependencies on every publish of this Content View. , we should mention module streams or rephrase this

johnpmitsch · 2019-09-10T13:21:31Z

app/models/katello/content_view_module_stream_filter.rb

+    private
+
+    def module_stream_arel
+      ::Katello::ModuleStream.arel_table


Why use arel in this file, does AR not provide the capability to do what you need?

So this query is only generating the where clause, instead of hard coding "(item in 111,24,5)" as string I feel arel table is a better approach

can't you do ::Katello::ModuleStream.find(module_stream_ids)?

johnpmitsch

Code and functionality looks good, just a few comments and open questions!

This commit adds UI, API and publish bindings for filtering module streams in a content view.

parthaa · 2019-09-11T13:56:23Z

[test katlelo]

parthaa · 2019-09-11T15:40:04Z

[test katello]

parthaa · 2019-09-11T18:34:34Z

Code and functionality looks good, just a few comments and open questions!

updated

parthaa · 2019-09-11T18:34:45Z

[test katello]

johnpmitsch · 2019-09-12T19:50:44Z

...katello/app/assets/javascripts/bastion_katello/content-views/new/views/content-view-new.html

@@ -82,7 +82,7 @@ <h2 translate>Create Content View</h2>
          </label>
        </div>
        <p class="help-block" translate>
-          This will solve RPM dependencies on every publish of this Content View. Dependency solving significantly increases
+          This will solve RPM and Module Stream dependencies on every publish of this Content View. Dependency solving significantly increases


This text is also in engines/bastion_katello/app/assets/javascripts/bastion_katello/content-views/details/views/content-view-info.html, can you update there as well?

johnpmitsch · 2019-09-12T19:53:11Z

app/models/katello/content_view_module_stream_filter.rb

+    def generate_clauses(_repo)
+      return if module_stream_rules.blank?
+
+      module_stream_ids = module_stream_rules.map(&:module_stream_id)


Suggested change

module_stream_ids = module_stream_rules.map(&:module_stream_id)

module_stream_ids = module_stream_rules.map(&:module_stream_id)

::Katello::ModuleStream.find(module_stream_ids) unless module_stream_ids.empty?

This is what I am thinking, but maybe I'm missing something

Ok I can see why this is confusing. Check the following output on rails console

irb(main):005:0> ::Katello::ModuleStream.arel_table[:id].in([1,2,3]).to_sql => "\"katello_module_streams\".\"id\" IN (1, 2, 3)"

The contract for generate_clauses is to return a sub clause like a where condition. Some thing like katello_module_streams in (...) . This method returns the arel version. Some where down the line in ModuleClauseGenerator it actually does a to_sql and aggregates various clauses with "or". Check out https://github.com/Katello/katello/pull/8295/files#diff-bba4518369114a7121b8027dac527bb1R56-R64

The main intent of generate clauses is to return \"katello_module_streams\".\"id\" IN (1, 2, 3)
" (a sub condition if you wll)

@parthaa I get what you are doing here, but I'm still failing to see why all this logic is necessary. It's very possible that I'm overlooking some scenarios, I only poked around with basic examples.

Currently, it seems like a lot of hoops to jump through just to get a list of pulp ids from a list of module stream ids.

I tried something like this:

diff --git a/app/lib/katello/util/module_stream_clause_generator.rb b/app/lib/katello/util/module_stream_clause_generator.rb index e67cc35..8a7d9f8 100644 --- a/app/lib/katello/util/module_stream_clause_generator.rb +++ b/app/lib/katello/util/module_stream_clause_generator.rb @@ -54,13 +54,11 @@ module Katello end def clauses_for_module_streams(module_stream_clauses = []) - query_clauses = module_stream_clauses.map do |clause| - "(#{clause.to_sql})" - end - return unless query_clauses.any? + ids = module_stream_clauses + return unless ids.any? + ids.flatten! - statement = query_clauses.join(" OR ") - {'_id' => { "$in" => ModuleStream.where(statement).pluck(:pulp_id)}} + {'_id' => { "$in" => ModuleStream.where(id: ids).pluck(:pulp_id)}} end end end diff --git a/app/models/katello/content_view_module_stream_filter.rb b/app/models/katello/content_view_module_stream_filter.rb index f7803a2..ceca720 100644 --- a/app/models/katello/content_view_module_stream_filter.rb +++ b/app/models/katello/content_view_module_stream_filter.rb @@ -9,18 +9,7 @@ module Katello def generate_clauses(_repo) return if module_stream_rules.blank? - module_stream_ids = module_stream_rules.map(&:module_stream_id) - module_streams_in(module_stream_ids) unless module_stream_ids.empty? - end - - private - - def module_stream_arel - ::Katello::ModuleStream.arel_table - end - - def module_streams_in(ids) - module_stream_arel[:id].in(ids) + module_stream_rules.map(&:module_stream_id) end end end

What is a basic example like the one above missing?

My main concern with using arel is I'm not sure how well Rails will support its API in upgrades and I would hate to see us cause headaches for ourselves down the road. Is there a way to do this without arel?

johnpmitsch · 2019-09-12T23:43:55Z

I still want to continue the discussion about the arel clauses before approving, but otherwise, things look good and my other concerns are addressed 💯

johnpmitsch

Works well!

theforeman-bot added Needs testing Not yet reviewed labels Aug 23, 2019

parthaa force-pushed the module-stream-filters branch 6 times, most recently from 0f1b053 to 63649e3 Compare August 24, 2019 04:21

parthaa force-pushed the module-stream-filters branch from 63649e3 to 2816e87 Compare August 26, 2019 13:37

theforeman-bot added the Waiting on contributor label Aug 26, 2019

parthaa force-pushed the module-stream-filters branch from 2816e87 to 8380fe9 Compare August 26, 2019 13:44

theforeman-bot removed the Waiting on contributor label Aug 26, 2019

theforeman-bot added the Waiting on contributor label Aug 26, 2019

parthaa mentioned this pull request Aug 26, 2019

Fixes #27712 - module stream rules Katello/hammer-cli-katello#681

Merged

parthaa force-pushed the module-stream-filters branch from 8380fe9 to 9c8ade3 Compare August 26, 2019 14:07

theforeman-bot removed the Waiting on contributor label Aug 26, 2019

johnpmitsch reviewed Sep 5, 2019

View reviewed changes

parthaa force-pushed the module-stream-filters branch from f714a2e to e34745a Compare September 6, 2019 13:21

omkarkhatavkar mentioned this pull request Sep 6, 2019

added the API/UI tests for Module Stream Filter SatelliteQE/robottelo#7339

Merged

johnpmitsch reviewed Sep 10, 2019

View reviewed changes

johnpmitsch suggested changes Sep 10, 2019

View reviewed changes

parthaa added 9 commits September 10, 2019 19:11

Fixes #26226 - Filter Module Streams on CV

d3f778c

This commit adds UI, API and publish bindings for filtering module streams in a content view.

Refs #26226 - Addressed a PR issue

5d7b631

Refs #26226 - Fixed routing for mod-streams filter

0590807

Refs #26226 - Back to orignal model

bfd5efc

Refs #26226 - VCRs

9612c1a

Refs #26226 - fixed tests

538e102

Refs #26226 - Marked mod stream ids as array

1efa235

Refs #26226 - removing stray methods

583ebde

Refs #26226 - addressing pr comments

ac4d96b

parthaa force-pushed the module-stream-filters branch from e34745a to ac4d96b Compare September 10, 2019 23:18

johnpmitsch reviewed Sep 12, 2019

View reviewed changes

Refs #26226 - addressing pr comments

034e41d

Refs #26226 - addressing pr comments

3d49543

johnpmitsch approved these changes Sep 13, 2019

View reviewed changes

parthaa merged commit 84bb258 into Katello:master Sep 14, 2019

parthaa deleted the module-stream-filters branch September 14, 2019 04:01

	module_stream_ids = module_stream_rules.map(&:module_stream_id)
	module_stream_ids = module_stream_rules.map(&:module_stream_id)
	::Katello::ModuleStream.find(module_stream_ids) unless module_stream_ids.empty?

Fixes #26226 - Filter Module Streams on CV #8295

Fixes #26226 - Filter Module Streams on CV #8295

Conversation

parthaa commented Aug 23, 2019 • edited Loading

theforeman-bot commented Aug 23, 2019

omkarkhatavkar commented Aug 23, 2019

omkarkhatavkar commented Aug 26, 2019

omkarkhatavkar commented Aug 26, 2019

parthaa commented Aug 26, 2019

theforeman-bot commented Aug 26, 2019

coveralls commented Aug 26, 2019 • edited Loading

omkarkhatavkar commented Aug 26, 2019 • edited Loading

theforeman-bot commented Aug 26, 2019

parthaa commented Aug 26, 2019

parthaa commented Aug 26, 2019

omkarkhatavkar commented Aug 27, 2019 • edited Loading

omkarkhatavkar commented Aug 27, 2019

PASS

Still Failing

PASS

PASS

Can be Ignored

omkarkhatavkar commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

omkarkhatavkar commented Sep 9, 2019

johnpmitsch commented Sep 9, 2019

johnpmitsch commented Sep 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch left a comment

Choose a reason for hiding this comment

parthaa commented Sep 11, 2019

parthaa commented Sep 11, 2019

parthaa commented Sep 11, 2019

parthaa commented Sep 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parthaa Sep 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch commented Sep 12, 2019

johnpmitsch left a comment

Choose a reason for hiding this comment

parthaa commented Aug 23, 2019 •

edited

Loading

coveralls commented Aug 26, 2019 •

edited

Loading

omkarkhatavkar commented Aug 26, 2019 •

edited

Loading

omkarkhatavkar commented Aug 27, 2019 •

edited

Loading

parthaa Sep 12, 2019 •

edited

Loading