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 #27050 - Activation Keys to support System Purpose #8168
Conversation
Can one of the admins verify this patch? |
Issues: #27050 |
[test katello] |
There were the following issues with the commit message:
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 |
[test katello] |
@sseelam2 this looks really good - can you squash into a single commit? I'll merge after that |
fef9d08
to
4a625c2
Compare
Thank you for reviewing @jturel. I've squashed it into a single commit. |
@@ -0,0 +1,11 @@ | |||
class AddActivationKeyPurposeAddons < ActiveRecord::Migration[5.2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you lost the code which renamed this migration
@@ -45,3 +45,14 @@ library_dev_staging_view_key: | |||
environment_id: <%= ActiveRecord::FixtureSet.identify(:dev) %> | |||
content_view_id: <%= ActiveRecord::FixtureSet.identify(:library_dev_staging_view) %> | |||
auto_attach: true | |||
|
|||
key_purpose_attributes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need to name this 'key_' ... we already know it's an activation key :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct. But I'm following a pattern which exists. I renamed it to purpose_attributes_key.
test/models/activation_key_test.rb
Outdated
@@ -8,6 +8,7 @@ def setup | |||
@dev_view = katello_content_views(:library_dev_view) | |||
@lib_view = katello_content_views(:library_view) | |||
@pool_one = katello_pools(:pool_one) | |||
@purpose_attributes = katello_activation_keys(:key_purpose_attributes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe 'purpose_key' would be a better name for this variable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yah. That sounds good. Renamed it. 👍
4a625c2
to
c31deec
Compare
570ef49
to
c31deec
Compare
[test katello] |
d2c334f
to
ce13e35
Compare
[test katello] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a model PR in terms of meeting the requirements of the issue with a concise implementation and complete test coverage. Great work @sseelam2 . ACK!
Thanks @jturel for guiding me. 👍 |
This PR was created to use the functionality of Candlepin to support System Purpose on Activation Keys.
Since the UI isn't set up yet for the System Purpose under Activation Key, please use the api /katello/api/v2/activation_keys/1? to test the update and create functionality.
To test this PR:
Create and update purpose_role, purpose_usage and purpose_addons in an activation key using the api endpoint. This change should reflect in both Katello and Candlepin db.
View Katello::ActivationKey to see changes in the Katello db.
View Katello::Resources::Candlepin::ActivationKey.get to see changes made to Candlepin.