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 #23084 - activation-key copy fix #7345

Merged
merged 1 commit into from May 4, 2018
Merged

Fixes #23084 - activation-key copy fix #7345

merged 1 commit into from May 4, 2018

Conversation

ranjan
Copy link
Member

@ranjan ranjan commented Apr 30, 2018

Activation-key copy fails with "no implicit conversion of nil into String"

@theforeman-bot
Copy link

Issues: #23084

@sean797
Copy link
Member

sean797 commented May 1, 2018

Should we add a test for this? But I can confirm this works, backported to my production install.

@jlsherrill
Copy link
Member

While this works, i think its missing the root cause.

The problem i think is that here: https://github.com/Katello/katello/blob/master/app/lib/katello/resources/candlepin/activation_key.rb#L91

attrs_to_update and attrs_to_delete were both blank, so result is nil. This causes the json parsing to fail.

@ranjan
Copy link
Member Author

ranjan commented May 2, 2018

@jlsherrill you are right, but still we should skip the the unwanted method 'set_content_overrides' call if the content_overrides is blank.

@jlsherrill
Copy link
Member

@ranjan yeah thats fair, i'd like to see the other code handle that condition better though as well.

also, i'd prefer:
@new_activation_key.set_content_overrides(@activation_key.content_overrides) unless @activation_key.content_overrides.blank?

@jlsherrill
Copy link
Member

Copy link
Member

@jlsherrill jlsherrill left a comment

Choose a reason for hiding this comment

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

Thanks @ranjan !

@jlsherrill jlsherrill merged commit 556b48d into Katello:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants