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 #21734 - Remove alias_method_chain #300

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

xprazak2
Copy link
Contributor

No description provided.

@@ -7,8 +7,6 @@ module ActionTriggering
after_create :plan_hook_action
after_update :plan_hook_action
after_destroy :plan_hook_action
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this block get evaluated when the module gets prepended instead of getting included? If not, this needs to be converted to something like

def self.prepended(base)
  base.after_create :plan_hook_action
  base.after_update :plan_hook_action
  base.after_destroy :plan_hook_action
end

@xprazak2
Copy link
Contributor Author

I made changes as suggested

after_destroy :plan_hook_action

alias_method_chain :save, :dynflow_task_wrap
def prepended(base)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this works as expected: missing self.prepended. Try out this.

module Foo
  extend ActiveSupport::Concern

  def prepended(base)
     puts "hello from foo"
   end
end

class Bar
   prepend Foo
end

@@ -3,12 +3,10 @@ module Concerns
module ActionTriggering
extend ActiveSupport::Concern

included do
Copy link
Member

Choose a reason for hiding this comment

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

Since this is being included from other plugins, I think we should at least raise some expalanationary exception here, warning the users that they should use prepend instead of include

@xprazak2
Copy link
Contributor Author

I made changes as suggested

@mmoll
Copy link
Contributor

mmoll commented Dec 19, 2017

This is fully compatible with Rails 5.0, so this could go in right now, pending review (probably a good idea to create a 0.10-stable branch before that).

Copy link
Member

@iNecas iNecas left a comment

Choose a reason for hiding this comment

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

Waiitng for stable builds

@iNecas
Copy link
Member

iNecas commented Dec 20, 2017

I guess this will need a rebase in order the tests to work

@ehelms
Copy link
Member

ehelms commented Dec 20, 2017

@iNecas This appears to be green, so what are the tests it's waiting on?

@mmoll
Copy link
Contributor

mmoll commented Dec 20, 2017

after the master test went green a few hours ago, this PR test got triggered 👍

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.

LGTM and tests are green, can we :shipit: please?

@iNecas iNecas merged commit 85cd580 into theforeman:master Dec 20, 2017
@iNecas
Copy link
Member

iNecas commented Dec 20, 2017

We were waiting mainly for me finishing with 🎄 Marry Christmas, foreman-tasks-0.11.0 just got pushed to rubygems :)

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