Fixes #19954 - Add CSV export on content host index #6814

tbrisker · 2017-06-07T13:16:41Z

TODO:

Tests
Make sure all needed tables are preloaded

mention-bot · 2017-06-07T13:16:43Z

@tbrisker, thanks for your PR! By analyzing the history of the files in this pull request, we identified @waldenraines, @ehelms and @jlsherrill to be potential reviewers.

houndci-bot · 2017-06-07T13:16:49Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+                'applicable_errata.enhancement.count', 'applicable_rpms.count',
+                :operatingsystem, :lifecycle_environment, :content_view,
+                'subscription_facet.registered_at', 'subscription_facet.last_checkin'],
+                ['Name', 'Subscription Status', 'Installable Updates - Security', 'Installable Updates - Bug Fixes', 'Installable Updates - Enhancements', 'Installable Updates - Package Count', 'OS', 'Environment', 'Content View', 'Registered', 'Last Checkin'])


Line is too long. [261/200]

houndci-bot · 2017-06-07T13:16:49Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+                'applicable_errata.security.count', 'applicable_errata.bugfix.count',
+                'applicable_errata.enhancement.count', 'applicable_rpms.count',
+                :operatingsystem, :lifecycle_environment, :content_view,
+                'subscription_facet.registered_at', 'subscription_facet.last_checkin'],


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-06-07T13:16:49Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+              csv_response(@hosts, [:name, :subscription_status_label,
+                'applicable_errata.security.count', 'applicable_errata.bugfix.count',
+                'applicable_errata.enhancement.count', 'applicable_rpms.count',
+                :operatingsystem, :lifecycle_environment, :content_view,


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-06-07T13:16:49Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+                         .preload(:subscription_status_object, :content_facet, :subscription_facet)
+              csv_response(@hosts, [:name, :subscription_status_label,
+                'applicable_errata.security.count', 'applicable_errata.bugfix.count',
+                'applicable_errata.enhancement.count', 'applicable_rpms.count',


Align the elements of an array literal if they span more than one line.

houndci-bot · 2017-06-07T13:16:49Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+              @hosts = resource_base_with_search.where(organization_id: params[:organization_id])
+                         .preload(:subscription_status_object, :content_facet, :subscription_facet)
+              csv_response(@hosts, [:name, :subscription_status_label,
+                'applicable_errata.security.count', 'applicable_errata.bugfix.count',


Align the elements of an array literal if they span more than one line.

akofink · 2017-06-07T13:32:45Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+            format.csv do
+              @hosts = resource_base_with_search.where(organization_id: params[:organization_id])
+                         .preload(:subscription_status_object, :content_facet, :subscription_facet)
+              csv_response(@hosts,


Why not use a view layer (i.e. hosts.csv.erb) ? More complex model-formatting logic could go in a decorator or presenter. It seems like bad practice to bloat the controller for csv functionality.

There is no view, the data is streamed directly one row at a time by the CSV exporter to the client. Since reports can sometimes be very large, we don't want to block the download until they are generated completely.
This is only complex because of the amount of columns and relations this table has, other tables are much simpler (you can see some examples in foreman core).

sjha4 · 2017-06-08T14:26:30Z

This works after updating the foreman controller/csv_responder.rb file following similar changes in the app/services/csv_exporter.rb to accept the third parameter. Could you verify this?

jlsherrill · 2017-06-08T19:09:20Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

@@ -23,6 +26,35 @@ def puppet_environment_for_content_view
          cvpe = Katello::ContentViewPuppetEnvironment.where(:environment_id => environment, :content_view_version_id => version).first
          render :json => cvpe.nil? ? nil : {:name => cvpe.puppet_environment.name, :id => cvpe.puppet_environment.id}
        end
+
+        def content_hosts


I think having a separate endpoint for 'content host' information is not ideal. We've worked on unifying these concepts at the api and model layer and I think we should continue work towards that. This would mean that the list of attributes for csv would need to be pluggable, but i think thats a better route and could be used for our extensions of hostgroup as well.

tbrisker · 2017-06-11T08:20:53Z

@sjha4 Thanks for catching that, I've opened theforeman/foreman#4578 to fix the issue.
@jlsherrill I'm not sure how else we can approach this - since the host controller already has a csv response used for the export button on the host index. I'd be happy for any suggestions.

tbrisker · 2017-06-12T15:00:53Z

Pushed with a bit of improvement to the preloaded tables; still having an issue of having to count applicable errata by type per host, not sure if there is anything that can be done about that though.

jlsherrill · 2017-06-13T02:36:09Z

@jlsherrill I'm not sure how else we can approach this - since the host controller already has a csv response used for the export button on the host index. I'd be happy for any suggestions.

I'm suggesting that the same endpoint be used and return all the same information no matter which page they are on. When katello is installed, more information is returned on both pages. This could involve:

a) using the 'rails' way of alias method chaining the 'csv_columns' method to add things to the normal host controller array.
b) creating some sort of plugin endpoint to 'extend' the csv columns for a particular object

a) is quite trivial so i'm fine with that in the shorter term, but i do think its a better solution than creating two different host csv lists. The UI being split into two (Hosts and content hosts) is a bug not a feature, so I'd like to limit it as much as possible

tbrisker · 2017-06-14T11:52:57Z

@jlsherrill There are two problems with your suggested approach:

That would cause both the host and content host export button to return the same data, both of which won't match the table displayed in the UI.
Due to the amount of related tables, exporting any large amount of hosts would lead to a very slow response. We already have this issue with the errata types, since there is no direct relation atm as I mentioned before.

jlsherrill · 2017-06-14T13:13:22Z

I see, so the intent is to just let the user download the information in the table rather than download information about 'hosts'. If thats the case I'm okay with it!

jlsherrill · 2017-06-14T13:24:43Z

still having an issue of having to count applicable errata by type per host, not sure if there is anything that can be done about that though.

Yes, currently for the hosts api we're still having to do an N+1 query. I don't know of a way in rails to do this as there is no attribute on the model that can represent the various counts. The only solution I know of to eliminate them completely would be to cache the values on the objects themselves which i think is becoming more and more of a necessity.

jlsherrill · 2017-06-14T13:44:19Z

Current test failure is just due to permissions, your new route needs to be added here: https://github.com/Katello/katello/blob/master/lib/katello/permissions/host_permissions.rb#L36

I opened http://projects.theforeman.org/issues/20019 to pull the errata counts into the various objects as a cache.

tbrisker · 2017-06-14T13:55:08Z

@jlsherrill thanks, added the missing permission

houndci-bot · 2017-06-29T13:01:05Z

test/controllers/foreman/hosts_controller_test.rb

+      assert_equal "attachment; filename=\"hosts-#{Date.today}.csv\"", response.headers["Content-Disposition"]
+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal Katello::Concerns::HostsControllerExtensions::CSV_HEADER.join(',')+"\n", buf.next


Surrounding space missing for operator +.

houndci-bot · 2017-06-29T13:01:05Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

+                    'applicable_errata.bugfix.size', 'applicable_errata.enhancement.size',
+                    'applicable_rpms.size', :operatingsystem, :lifecycle_environment, :content_view,
+                    'subscription_facet.registered_at', 'subscription_facet.last_checkin']
+      CSV_HEADER= ['Name', 'Subscription Status', 'Installable Updates - Security',


Surrounding space missing for operator =.
Freeze mutable objects assigned to constants.

houndci-bot · 2017-06-29T13:01:05Z

app/controllers/katello/concerns/hosts_controller_extensions.rb

@@ -3,7 +3,17 @@ module Concerns
    module HostsControllerExtensions
      extend ActiveSupport::Concern
      include ForemanTasks::Triggers
+      CSV_FIELDS = [:name, :subscription_status_label, 'applicable_errata.security.size',


Freeze mutable objects assigned to constants.

I did, bad dog!

tbrisker · 2017-06-29T13:04:04Z

Added tests, optimized table loading as best as possible atm

houndci-bot · 2017-06-29T15:13:04Z

test/controllers/foreman/hosts_controller_test.rb

+      assert_equal "attachment; filename=\"hosts-#{Date.today}.csv\"", response.headers["Content-Disposition"]
+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal "Name,Subscription Status,Installable Updates - Security,Installable Updates - Bug Fixes,Installable Updates - Enhancements,Installable Updates - Package Count,OS,Environment,Content View,Registered,Last Checkin\n", buf.next


Line is too long. [243/200]

houndci-bot · 2017-06-29T15:13:04Z

test/controllers/foreman/hosts_controller_test.rb

+      assert_equal "attachment; filename=\"hosts-#{Date.today}.csv\"", response.headers["Content-Disposition"]
+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal "Name,Subscription Status,Installable Updates - Security,Installable Updates - Bug Fixes,Installable Updates - Enhancements,Installable Updates - Package Count,OS,Environment,Content View,Registered,Last Checkin\n", buf.next


Line is too long. [243/200]

houndci-bot · 2017-06-29T15:13:04Z

test/controllers/foreman/hosts_controller_test.rb

+      assert_equal "attachment; filename=\"hosts-#{Date.today}.csv\"", response.headers["Content-Disposition"]
+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal "Name,Subscription Status,Installable Updates - Security,Installable Updates - Bug Fixes,Installable Updates - Enhancements,Installable Updates - Package Count,OS,Environment,Content View,Registered,Last Checkin\n", buf.next


Line is too long. [243/200]

houndci-bot · 2017-06-29T15:18:53Z

test/controllers/foreman/hosts_controller_test.rb

+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal "Name,Subscription Status,Installable Updates - Security,Installable Updates - Bug"+
+        " Fixes,Installable Updates - Enhancements,Installable Updates - Package Count,OS,Environment,"+


Surrounding space missing for operator +.
Use \ instead of + or << to concatenate those strings.

houndci-bot · 2017-06-29T15:18:54Z

test/controllers/foreman/hosts_controller_test.rb

+      assert_equal "attachment; filename=\"hosts-#{Date.today}.csv\"", response.headers["Content-Disposition"]
+      buf = response.stream.instance_variable_get(:@buf)
+      assert buf.is_a? Enumerator
+      assert_equal "Name,Subscription Status,Installable Updates - Security,Installable Updates - Bug"+


Surrounding space missing for operator +.
Use \ instead of + or << to concatenate those strings.

jlsherrill · 2017-06-30T18:20:46Z

...astion_katello/app/assets/javascripts/bastion_katello/content-hosts/views/content-hosts.html

@@ -5,6 +5,9 @@ <h2 translate>Content Hosts</h2>
  </div>

  <div data-block="list-actions">
+    <a href="/hosts/content_hosts.csv?{{csvQuery}}" class="btn btn-default">
+      <span translate>Export</span>
+    </a>


seeing a small issue with using this. It causes the URL to change which messes up things such as:
a) clicking export a 2nd time doesn't work
b) clicking 'search' does cause a 2nd download

adding target="_self" seems to fix it. @waldenraines is that appropriate here?

Try ng-href instead of href. If that doesn't work I'm fine with this.

ng-href didn't fix it, added target.

jlsherrill

ACK thanks @tbrisker

theforeman-bot added Needs testing Not yet reviewed labels Jun 7, 2017

houndci-bot reviewed Jun 7, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from cb3632e to 45a7d7e Compare June 7, 2017 13:19

akofink reviewed Jun 7, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from 45a7d7e to ac4fe21 Compare June 8, 2017 08:11

jlsherrill reviewed Jun 8, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from ac4fe21 to f287420 Compare June 12, 2017 14:59

tbrisker force-pushed the 19954 branch from f287420 to d952468 Compare June 14, 2017 13:54

tbrisker force-pushed the 19954 branch from d952468 to e1e197b Compare June 29, 2017 13:00

houndci-bot reviewed Jun 29, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from e1e197b to 1cc2a17 Compare June 29, 2017 13:02

tbrisker force-pushed the 19954 branch from 1cc2a17 to a66563f Compare June 29, 2017 15:09

houndci-bot reviewed Jun 29, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from a66563f to dceb4b2 Compare June 29, 2017 15:16

houndci-bot reviewed Jun 29, 2017

View reviewed changes

tbrisker force-pushed the 19954 branch from dceb4b2 to 81e805f Compare June 29, 2017 16:09

jlsherrill reviewed Jun 30, 2017

View reviewed changes

Fixes #19954 - Add CSV export on content host index

bb0bfa3

tbrisker force-pushed the 19954 branch from 81e805f to bb0bfa3 Compare July 2, 2017 12:09

jlsherrill approved these changes Jul 3, 2017

View reviewed changes

jlsherrill merged commit 6362c73 into Katello:master Jul 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #19954 - Add CSV export on content host index #6814

Fixes #19954 - Add CSV export on content host index #6814

tbrisker commented Jun 7, 2017 •

edited

Loading

mention-bot commented Jun 7, 2017

houndci-bot Jun 7, 2017

houndci-bot Jun 7, 2017

houndci-bot Jun 7, 2017

houndci-bot Jun 7, 2017

houndci-bot Jun 7, 2017

akofink Jun 7, 2017

tbrisker Jun 7, 2017

sjha4 commented Jun 8, 2017

jlsherrill Jun 8, 2017

tbrisker commented Jun 11, 2017

tbrisker commented Jun 12, 2017

jlsherrill commented Jun 13, 2017

tbrisker commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

tbrisker commented Jun 14, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

tbrisker Jun 29, 2017

tbrisker commented Jun 29, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

houndci-bot Jun 29, 2017

jlsherrill Jun 30, 2017

waldenraines Jun 30, 2017

tbrisker Jul 2, 2017

jlsherrill left a comment

Fixes #19954 - Add CSV export on content host index #6814

Fixes #19954 - Add CSV export on content host index #6814

Conversation

tbrisker commented Jun 7, 2017 • edited Loading

mention-bot commented Jun 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjha4 commented Jun 8, 2017

Choose a reason for hiding this comment

tbrisker commented Jun 11, 2017

tbrisker commented Jun 12, 2017

jlsherrill commented Jun 13, 2017

tbrisker commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

jlsherrill commented Jun 14, 2017

tbrisker commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbrisker commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlsherrill left a comment

Choose a reason for hiding this comment

tbrisker commented Jun 7, 2017 •

edited

Loading