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 #22205 - restore blueprint actions for notifications #5159

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

ares
Copy link
Member

@ares ares commented Jan 9, 2018

self.actions defaults to {}, the logic with conditional overiding is correct but the condition is wrong

@theforeman-bot
Copy link
Member

Issues: #22205

@tbrisker
Copy link
Member

tbrisker commented Jan 9, 2018

@ares please rebase on latest develop to fix failing minitest issue

@ares
Copy link
Member Author

ares commented Jan 10, 2018

thanks @tbrisker, rebased

@mmoll
Copy link
Contributor

mmoll commented Jan 10, 2018

another rebase for the audited pin is needed now...

@ekohl
Copy link
Member

ekohl commented Jan 11, 2018

Does this also need the 1.17 milestone?

@ares ares added this to the 1.17.0 milestone Jan 11, 2018
@ares
Copy link
Member Author

ares commented Jan 11, 2018

it's set in redmine, but I assigned the milestone too, I'll rebase soon

@ares
Copy link
Member Author

ares commented Jan 11, 2018

rebased

subject,
notification_blueprint.actions
).actions if notification_blueprint.actions.any?
).actions if notification_blueprint.actions.any? && self.actions.blank?
Copy link
Member

Choose a reason for hiding this comment

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

Not a ruby developer, but multi line statements with guards are hard to parse for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not making the problem worse, since this will be cherrypicked I'd probably keep it as it is, I can open separate PR with refactoring if you prefer

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Member Author

Choose a reason for hiding this comment

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

#5170 addresses it

Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @ares !

@tbrisker tbrisker merged commit 099d4ea into theforeman:develop Jan 16, 2018
@tbrisker
Copy link
Member

Cherry-picked to 1.17-stable as well as d11899e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants