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

Include ActionMailer::DeliverLater::Mixin in ActionMailer::Base by default #6

Open
dhh opened this Issue Jul 16, 2014 · 45 comments

Comments

Projects
None yet
5 participants
@dhh
Copy link
Collaborator

commented Jul 16, 2014

When you install this plugin, it should just be available. No need to add include ActionMailer::DeliverLater::Mixin by hand.

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2014

@cristianbica, do you want to take a stab at this?

@seuros

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2014

I will to this tomorrow.

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2014

👍

On Jul 16, 2014, at 4:06 PM, Abdelkader Boudih notifications@github.com wrote:

I will to this tomorrow.


Reply to this email directly or view it on GitHub.

@seuros

This comment has been minimized.

Copy link
Owner

commented Jul 16, 2014

Do you think it better if i do it with railtie ?

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 16, 2014

Yeah, that sounds like a good plan.

On Jul 16, 2014, at 4:08 PM, Abdelkader Boudih notifications@github.com wrote:

Do you think it better if i do it with railtie ?


Reply to this email directly or view it on GitHub.

@morgoth

This comment has been minimized.

Copy link

commented Aug 11, 2014

When I install this gem from github, my mailer still doesn't have deliver_later method.

AuthMailer.confirmation_instructions(user).deliver_later
> NoMethodError:
     undefined method `deliver_later' for #<Mail::Message:0x007ff3625c01b8>

Is there anything more required to make this work?

I'm using rails 4.1.4

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

I will look into it.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

So i located the bug not i'm not sure yet how to fix it.
When we include the mixing manually we have

DelayedMailer, ActionMailer::DeliverLater::Mixin, ActionMailer::Base, ActionView::Layouts, ....

with the initializer we get (i changed include to prepend)

DelayedMailer, #<Module:0x007f086be68d48>, #<Module:0x007f086c2a7aa8>, ActionMailer::DeliverLater::Mixin, ActionMailer::Base,....

I need to find a way to include the mixin before the unnamed module. The unamed module don't call super

@dhh @rafaelfranca @cristianbica any though ?

@seuros seuros reopened this Aug 11, 2014

@rafaelfranca

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2014

One thin we need to do is to fix this anonymous module to call super.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

I'm looking at the rails source since there is no extra gem that could have added these module.
We will have to patch rails 4.1 and 4.2, right ?

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2014

We need to fix it for 4.2 as it will be included in 4.2. I've created a sample app at https://github.com/cristianbica/actionmailer_deliver_later_sample_app. The issue seems to be that the default method_missing is called from ActionMailer::Base and not the one from the mixin

irb(main):002:0> TestMailer.ancestors
=> [TestMailer, #<Module:0x007f83a8231a08>, #<Module:0x007f83a823afb8>, #<Module:0x007f83a46b6cd0>, #<Module:0x007f83a46b6d70>, ActionMailer::Base, ActionMailer::DeliverLater::Mixin, ......]

irb(main):003:0> ActionMailer::Base.ancestors
=> [ActionMailer::Base, ActionMailer::DeliverLater::Mixin, ...]

irb(main):004:0> TestMailer.method(:method_missing).source_location
=> ["/Users/cristi/.rbenv/versions/2.0.0-p451/lib/ruby/gems/2.0.0/bundler/gems/rails-b45b99894a60/actionmailer/lib/action_mailer/base.rb", 550]

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

I'm sending a little fix right now @cristianbica

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

@cristianbica https://github.com/seuros/actionmailer-deliver_later/tree/prepend << this show that ActionMailer::Base is not the problem , the anonymous modules don't call super.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2014

Actually the problem is with ActionMailer::Base's method_missing:

[8] pry(main)> TestMailer.method(:method_missing)
=> #<Method: TestMailer(ActionMailer::Base).method_missing>

The real problem isn't rails as it is a ruby mixins issue. Ruby's include isn't suitable for this as you cannot override methods and you can't call super (see: http://stackoverflow.com/questions/4470108/when-monkey-patching-a-method-can-you-call-the-overridden-method-from-the-new-i). Ruby's Module#prepend fixed this issue and that's why it's working using prepend so I suggest go ahead and use prepend for this.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 11, 2014

That still won't fix the issue.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Actually it will fix it:

[1] pry(main)> TestMailer.test_email.class
=> ActionMailer::DeliverLater::MailMessageWrapper
[2] pry(main)> TestMailer.test_email.deliver_later
Enqueued ActionMailer::DeliverLater::Job (Job ID: 422c4472-2d94-4151-8074-7202008fe4dd) to Inline(active_jobs_mailers) with arguments: "TestMailer", "test_email", "deliver"

But ActiveSupport::Concern doesn't supports Module#prepend so I had to do:

diff --git a/lib/action_mailer/deliver_later/railtie.rb b/lib/action_mailer/deliver_later/railtie.rb
index 2a4711d..a3d3254 100644
--- a/lib/action_mailer/deliver_later/railtie.rb
+++ b/lib/action_mailer/deliver_later/railtie.rb
@@ -6,9 +6,12 @@ module Actionmailer
     class Railtie < Rails::Railtie # :nodoc:
       initializer 'actionmailer-deliver_later' do
         ActiveSupport.on_load(:action_mailer) do
-          include ActionMailer::DeliverLater::Mixin
+          prepend ActionMailer::DeliverLater::Mixin
+          class << self
+            prepend ActionMailer::DeliverLater::Mixin::ClassMethods
+          end
         end
       end
     end
   end
-end
\ No newline at end of file
+end
@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

While this works the Module#prepend is a ruby 2.0+ method and rails 4.2 will still support ruby 1.9.3. So we'll have to find another solution

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails

/cc @dhh @rafaelfranca

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2014

Sounds good to me. Do you want to prepare the PR for merging into Rails itself? Would like to get it in this week.

On Aug 11, 2014, at 22:51, Cristian Bica notifications@github.com wrote:

To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails

/cc @dhh @rafaelfranca


Reply to this email directly or view it on GitHub.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

Shouldn't we merge activejob first ? and yes, i can prepare a PR.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

Then we can turn this gem to support rails 4.1 only.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Yep. I'll give it a try later today. I think we should skip the integration tests as it adds a lot of dependencies (gems and system - redis, postgres, rabbitmq), has some sleep in it and it's kind of slow--
Cristian Bica

On Tue, Aug 12, 2014 at 8:52 AM, David Heinemeier Hansson
notifications@github.com wrote:

Sounds good to me. Do you want to prepare the PR for merging into Rails itself? Would like to get it in this week.

On Aug 11, 2014, at 22:51, Cristian Bica notifications@github.com wrote:

To make this work as a gem we need to monkey patch ActionMailer::Base but as the plan is to merge activejob and this into rails we can hook directly into the ActionMailer::Base implementation when merging. The current working solution is to include the mixin in you ActionMailer::Base subclasses and I think we should leave it as it is until merged in rails

/cc @dhh @rafaelfranca

Reply to this email directly or view it on GitHub.

Reply to this email directly or view it on GitHub:
#6 (comment)

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

That kills the git history, i think the git history should be included.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

I don't know how to do that and I don't think we need to add tens of commits to rails--
Cristian Bica

On Tue, Aug 12, 2014 at 12:02 PM, Abdelkader Boudih
notifications@github.com wrote:

That kills the git history, i think the git history should be included.

Reply to this email directly or view it on GitHub:
#6 (comment)

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

I already did it when i said i can prepare the PR, rails repository will see just one commit but the git history is preserved. I will push my changes when i return to my other computer.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Ok. There are some changes in Rakefile, railtie.rb, test/cases/logging_test.rb--
Cristian Bica

On Tue, Aug 12, 2014 at 12:33 PM, Abdelkader Boudih
notifications@github.com wrote:

I already did it when i said i can prepare the PR, rails repository will see just one commit but the git history is preserved. I will push my changes when i return to my other computer.

Reply to this email directly or view it on GitHub:
#6 (comment)

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

Thanks, i just want to know if we should remove the work you did in the integration testing. I want to keep it, but i dunno what your opinion and @dhh, @rafaelfranca are.

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Remove them and if rails team decides they should be added we'll add them--
Cristian Bica

On Tue, Aug 12, 2014 at 12:44 PM, Abdelkader Boudih
notifications@github.com wrote:

Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.

Reply to this email directly or view it on GitHub:
#6 (comment)

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

How did you merge AJ into rails while preserving git history? Format patch and search and replace on paths?--
Cristian Bica

On Tue, Aug 12, 2014 at 12:44 PM, Abdelkader Boudih
notifications@github.com wrote:

Sorry i should have skipped reading your message early. I see that you decided to remove the integration testing.

Reply to this email directly or view it on GitHub:
#6 (comment)

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Nice. Didn't knew about it. Let me know when you push to your rails fork and I can send you the changes to Rakefile and others--
Cristian Bica

On Tue, Aug 12, 2014 at 12:52 PM, Abdelkader Boudih
notifications@github.com wrote:

@cristianbica with git-subtree

Reply to this email directly or view it on GitHub:
#6 (comment)

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

Also ActionMailer deliver_later should probably be partially rewritten to move the classes under ActionMailer. You can take a look at rails repo jobs branch --
Cristian Bica

On Tue, Aug 12, 2014 at 12:56 PM, Cristian Bica cristian.bica@gmail.com
wrote:

Nice. Didn't knew about it. Let me know when you push to your rails fork and I can send you the changes to Rakefile and others--
Cristian Bica
On Tue, Aug 12, 2014 at 12:52 PM, Abdelkader Boudih
notifications@github.com wrote:

@cristianbica with git-subtree

Reply to this email directly or view it on GitHub:
#6 (comment)

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

About the merging ... from what I see in the past the practice is to ignore the git history. See the merge of strong_parameters at rails/rails#7251. Anyway if it's required to ditch the history you'll rebase and that's it :)

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

https://github.com/seuros/rails/tree/activejob.
I'm fine with both methods, i however like the subtree approche to keep sync with the original repo.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

@dhh / @rafaelfranca : We are merging activejob under rails at https://github.com/seuros/rails/tree/activejob and having some discussions at seuros/rails#1. Can you guys help us a little on clarifying some stuff?
thanks

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2014

I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.

Thanks for working together on this, guys!

@cristianbica, why wouldn't you want to keep the integration tests?

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2014

Can you guys open a PR for the merge? Remember, we also need to get activemodel-global_id merged.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian Bica

On Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:

I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!

@cristianbica, why wouldn't you want to keep the integration tests?

Reply to this email directly or view it on GitHub:
#6 (comment)

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2014

I don’t mind any of that stuff. It’s better to be comprehensively covered, than to be fast. We can always be fast if it doesn’t have to work :).

On Aug 12, 2014, at 8:34, Cristian Bica notifications@github.com wrote:

A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian Bica

On Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:

I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!

@cristianbica, why wouldn't you want to keep the integration tests?

Reply to this email directly or view it on GitHub:
#6 (comment)

Reply to this email directly or view it on GitHub.

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

:) ok. We'll probably need to update the contributing guides, rails dev box, right?--
Cristian Bica

On Tue, Aug 12, 2014 at 6:37 PM, David Heinemeier Hansson
notifications@github.com wrote:

I don’t mind any of that stuff. It’s better to be comprehensively covered, than to be fast. We can always be fast if it doesn’t have to work :).
On Aug 12, 2014, at 8:34, Cristian Bica notifications@github.com wrote:

A lot of development dependencies: all the adapters gems, redis, rabbitmq, postgres, beanstalkd. Also the tests are pretty slow as I have a 2 second sleep to wait for the adapter to process the jobs. --
Cristian Bica

On Tue, Aug 12, 2014 at 6:31 PM, David Heinemeier Hansson
notifications@github.com wrote:

I didn't know of git-subtree. It wasn't intentional to kill the history with the previous merges. I like preserving the history, if we can, and it looks like we can.
Thanks for working together on this, guys!

@cristianbica, why wouldn't you want to keep the integration tests?

Reply to this email directly or view it on GitHub:
#6 (comment)

Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub:
#6 (comment)

@seuros

This comment has been minimized.

Copy link
Owner

commented Aug 12, 2014

@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?
Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?

@cristianbica

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2014

I think we can use GlobalID as a gem dependency until it's merged--
Cristian Bica

On Tue, Aug 12, 2014 at 6:46 PM, Abdelkader Boudih
notifications@github.com wrote:

@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?

Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?

Reply to this email directly or view it on GitHub:
#6 (comment)

@dhh

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 12, 2014

I’d love for you guys to run with this entire project: activejob, actionmailer-deliver_later, and activemodel-global_id. Let’s get everything into the same PR, since it’s all one-big feature, and then we can solicit comments there.

The proposed timeline at the moment is that we’ll release 4.2.0.beta1 with this in place on Sunday, August 17.

Great work, once again, guys.

On Aug 12, 2014, at 8:46, Abdelkader Boudih notifications@github.com wrote:

@dhh, should we wait until @jeremy merge activemodel-global_id or can we do it ?
Once we have initial version ready @cristianbica or me will open the PR, is that ok for you ?


Reply to this email directly or view it on GitHub.

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.