Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #14933 - Content Host Counts column on the Errata page should o… #6017

Merged
merged 1 commit into from
May 16, 2016
Merged

Conversation

sean797
Copy link
Member

@sean797 sean797 commented May 5, 2016

…nly count hosts in the current org

If organization_id is supplied we only return applicable & available hosts in that org, if it isn't supplied we keep the current behavior and count all hosts across every org.

I#m not sure if we should be requiring organization_id to be supplied with this API GET /katello/api/errata ?

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for 9dd52ea57a70a498fc39eb5cea608f3f7e957887 exceeds 65 characters
  • commit message for 9dd52ea57a70a498fc39eb5cea608f3f7e957887 is not wrapped at 72nd column

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

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


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for 46b265dc952fbb5f8e7eb07b281ec14e38fe3de9 exceeds 65 characters
  • commit message for 46b265dc952fbb5f8e7eb07b281ec14e38fe3de9 is not wrapped at 72nd column

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

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


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for e11cce88f2019a8e4ecc308081591f3d7eca15e7 exceeds 65 characters

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

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


This message was auto-generated by Foreman's prprocessor

self.content_facets_applicable.joins(:host)
def hosts_applicable(org_id)
if org_id.present?
self.content_facets_applicable.where(:host_id => ::Host.where(:organization_id => org_id)).joins(:host)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could do something like:

 self.content_facets_applicable.joins(:host).where("#{::Host.table_name}.organization_id" => org_id)

and that avoids the sub query

@jlsherrill
Copy link
Member

@sean797 looks like a couple tests are failing, if you have any trouble feel free to ping me, i'm happy to help. Overall this looks good, just one minor comment.

@theforeman-bot
Copy link

There were the following issues with the commit message:

  • length of the first commit message line for 02f7ad80d2872f1ba38531b653a2d092790e08c7 exceeds 65 characters

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

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


This message was auto-generated by Foreman's prprocessor

@sean797
Copy link
Member Author

sean797 commented May 9, 2016

Thanks for review @jlsherrill, some help with the tests would be appreciated 👍 I'm not sure if theres a better way of doing params[:organization_id] that will make Katello::ErratumViewTest.test_show pass?

@sean797
Copy link
Member Author

sean797 commented May 12, 2016

[test] - old test results aren't there 👎 http://ci.theforeman.org/job/test_katello_pr_core/2313/

refute_includes @bugfix.hosts_available, @host_without_errata
assert_includes @security.hosts_available(::Organization.first.id), @host.content_facet
refute_includes @security.hosts_available(::Organization.first.id), @host_without_errata
refute_includes @bugfix.hosts_available(::Organization.first.id), @host_without_errata
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of the tests is failing because here you'd want to pass in @host.organization.id (rather than Organization.first.id.

Could you test for both, i want to make sure that the condition where id is passed in and when its not passed in are both covered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@host.organization work 👍

@jlsherrill
Copy link
Member

To fix your 2nd test failure, just change:

https://github.com/Katello/katello/blob/master/test/katello_test_helper.rb#L198-L199

to:

    Rabl::Renderer.new(filepath, resource, :view_path => "#{Katello::Engine.root}/app/views/",
                       :format => 'hash', :locals => {:resource => resource}, :scope => OpenStruct.new(:params => {})).render

@sean797
Copy link
Member Author

sean797 commented May 16, 2016

Thanks for you help with the tests @jlsherrill! There passing now 👍

@jlsherrill
Copy link
Member

ACK thanks @sean797 merging!

@jlsherrill jlsherrill merged commit 1bddf75 into Katello:master May 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants