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 #26531 - Search Purpose Addons #8152

Merged
merged 1 commit into from Jun 18, 2019

Conversation

Projects
None yet
4 participants
@sseelam2
Copy link
Member

commented Jun 10, 2019

This PR is created to search for hosts using purpose addons. Addons can be viewed under System Purpose inside Host's content.
To test this PR:

  • Create a Custom Product
  • Set some addons on it by connecting to your local postgres candlepin db
select uuid from cp2_products where name = 'CustomProduct';
insert into cp2_product_attributes(name, value, product_uuid) values ('addons', 'Test Addon1,Test Addon2', 'REPLACEME');
  • Restart the tomcat service to refresh the product cache systemctl restart tomcat

Now you should be able to see these addons under System Purpose. Add these addons to the host by selecting them and clicking on save.
To perform search on addons, go to Hosts and in the search box, type -
addon = "Name of the addon you have added to the host" and press enter.
You should be able to see the hosts which have the addon that you have searched for. Also while performing a search, the text should be auto completed.``

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 10, 2019

Can one of the admins verify this patch?

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 10, 2019

Issues: #26531

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

[test katello]

4 similar comments
@jturel

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

[test katello]

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

[test katello]

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

[test katello]

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 12, 2019

[test katello]

@ianballou

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I've given this a functional test and I can confirm that it works! Just one correction, the search term was addon = ... rather than addons = .... Auto complete worked too.

@sseelam2

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

Just one correction, the search term was addon = ... rather than addons = .... Auto complete worked too.

Ah. Thank you for noticing that. I'll edit my comment.

@@ -53,7 +57,6 @@ def import_database_attributes(consumer_params = candlepin_consumer.consumer_att
self.update_installed_products(consumer_params['installedProducts']) if consumer_params.key?('installedProducts')
self.purpose_role = consumer_params['role'] unless consumer_params['role'].nil?
self.purpose_usage = consumer_params['usage'] unless consumer_params['usage'].nil?
self.purpose_addons = consumer_params['addOns'] unless consumer_params['addOns'].nil?

This comment has been minimized.

Copy link
@jturel

jturel Jun 15, 2019

Member

I looked through the code and there are some instances where we update the subscription facet attributes not based on what was passed in (via UI update or client host checkin). In these situations we update the subscription facet based on the values in candlepin. Notice the signature of the method which does this:

def import_database_attributes(consumer_params = candlepin_consumer.consumer_attributes)

So, I think we need to restore this behavior that was removed. Since we no longer have purpose_addons as an attribute you can use the same approach as the host extensions with purpose_addon_ids

This comment has been minimized.

Copy link
@sseelam2

sseelam2 Jun 15, 2019

Author Member

Yes. That makes sense. I have updated the subscription_facet.rb and also have added the related test. Could you please review.

This comment has been minimized.

Copy link
@jturel

jturel Jun 17, 2019

Member

Looks good. Can you add the assertion back which tests the functionality?

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 15, 2019

There were the following issues with the commit message:

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

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

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 15, 2019

There were the following issues with the commit message:

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

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

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 16, 2019

[test katello]

@@ -18,7 +18,6 @@ module ApiPieExtensions

included do
include ApiPieExtensions

This comment has been minimized.

Copy link
@jturel

jturel Jun 17, 2019

Member

This might sound picky but it's better to not introduce unnecessary/unrelated changes. The space that was here should be put back :)

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

There were the following issues with the commit message:

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

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

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 17, 2019

There were the following issues with the commit message:

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

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

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 17, 2019

[test katello]

add_foreign_key :katello_subscription_facet_purpose_addons, :katello_subscription_facets, column: :subscription_facet_id, name: :katello_sub_facet_purpose_addon_facet_id
add_foreign_key :katello_subscription_facet_purpose_addons, :katello_purpose_addons, column: :purpose_addon_id, name: :katello_sub_facet_purpose_addon_purpose_addon_id

Katello::Host::SubscriptionFacet.pluck(:id, :purpose_addons).each do |facet|

This comment has been minimized.

Copy link
@jturel

jturel Jun 18, 2019

Member

I was most concerned if this migration would work properly or not - and it seems to work great. Here's what I did:

Set up some data:

irb(main):010:0> Katello::Host::SubscriptionFacet.where.not(purpose_addons: []).pluck(:id, :purpose_addons)
=> [[265, ["TestAddon1", "TestAddon2"]], [257, ["TestAddon1", "TestAddon2"]], [271, ["TestAddon3"]], [274, ["TestAddon1"]]]

Applied code change & migrated db, got these results:

irb(main):003:0> Katello::PurposeAddon.pluck(:name)
=> ["TestAddon1", "TestAddon2", "TestAddon3"]

Katello::Host::SubscriptionFacet.find(265).purpose_addons.pluck(:name)
=> ["TestAddon1", "TestAddon2"]

Katello::Host::SubscriptionFacet.find(257).purpose_addons.pluck(:name)
=> ["TestAddon1", "TestAddon2"]

Katello::Host::SubscriptionFacet.find(271).purpose_addons.pluck(:name)
=> ["TestAddon3"]

Katello::Host::SubscriptionFacet.find(274).purpose_addons.pluck(:name)
=> ["TestAddon1"]

Katello::SubscriptionFacetPurposeAddon.pluck(:subscription_facet_id, :purpose_addon_id)
=> [[265, 1], [265, 2], [257, 1], [257, 2], [271, 3], [274, 1]]

This comment has been minimized.

Copy link
@sseelam2

sseelam2 Jun 18, 2019

Author Member

Yes. This looks like the tables are set up the way we want. 👍

@@ -211,7 +211,7 @@
:content_facet_attributes => [:content_view_id, :lifecycle_environment_id, :content_source_id,
:host, :kickstart_repository_id],
:subscription_facet_attributes => [:release_version, :autoheal, :purpose_usage, :purpose_role, :service_level, :host,
{:installed_products => [:product_id, :product_name, :arch, :version]}, :facts, :hypervisor_guest_uuids => [], :purpose_addons => []]
{:installed_products => [:product_id, :product_name, :arch, :version]}, :facts, :hypervisor_guest_uuids => [], :purpose_addon_ids => []]

This comment has been minimized.

Copy link
@jturel

jturel Jun 18, 2019

Member

Whenever I update purpose addons from the UI I see this in the logs:

2019-06-18T01:03:01 [D|app|ffe23364] Unpermitted parameter: :purpose_addons

It would be nice if we could omit this. I think we need to add back purpose_addons here, and I think we need purpose_addon_ids too.

This comment has been minimized.

Copy link
@sseelam2

sseelam2 Jun 18, 2019

Author Member

Adding the :purpose_addons => [] back does not seem to help because it takes more precedence over purpose_addon_ids, so an instance of a String is being accepted rather than Katello::PurposeAddon and this breaks the functionality.

This comment has been minimized.

Copy link
@sseelam2

sseelam2 Jun 18, 2019

Author Member

As you suggested, deleting the :purpose_addons key after updating the :purpose_addon_ids in the before action under hosts_controller_extensions.rb work.

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Works really well, left just one comment on some logging around unpermitted params.

@theforeman-bot

This comment has been minimized.

Copy link

commented Jun 18, 2019

There were the following issues with the commit message:

  • length of the first commit message line for eb40c08 exceeds 65 characters
  • ee921e9 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

@sseelam2 sseelam2 force-pushed the sseelam2:purpose_addons_search branch from ee921e9 to d88e0e4 Jun 18, 2019

@jturel

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

[test katello]

@jturel jturel self-assigned this Jun 18, 2019

@jturel

jturel approved these changes Jun 18, 2019

Copy link
Member

left a comment

ACK. Nice work @sseelam2 and thanks for testing it out @ianballou !

@jturel jturel merged commit c31deec into Katello:master Jun 18, 2019

2 checks passed

katello Build finished. 4266 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.