Change code to invoke mailers with ids

[petergoldstein]

It's generally preferable to invoke mailers using ids. Using ids as opposed to full-blown model objects makes it easier to swap in asynchronous mailers, as only the ids need to be persisted in the back end store supporting the asynchronous mailers.

This PR includes the following changes:

1. Spree::OrderMailer and Spree::ShipmentMailer can be invoked using either models or their ids.
2. Methods that call methods on Spree::OrderMailer or Spree::ShipmentMailer now pass model ids as opposed to model objects
3. Changes to the specs to validate this behavior.

[radar]

Hi @petergoldstein. This looks good to me. Would you also be able to supply a patch for master as well please?

[petergoldstein]

Sure. I'll work up another PR for master. I should have it sometime tomorrow.

Will I still be able to get this merged to 1-2-stable? We're currently on the 1.2.x branch, so we need this merged there.

Thanks.

[radar]

I am extremely hesitant to merge code like this to only one branch, especially an older branch. If the PR was for master I would be fine with it, but it's for 1-2-stable. If I pull this in and people use 1-2-stable and then later upgrade to a branch where it's not there, there will be anger, which is bad.

If you desperately need it right this moment then the best option is using your Spree fork until its been added.

On Wed, Apr 3, 2013 at 6:02 PM, Peter Goldstein notifications@github.com
wrote:

Sure. I'll work up another PR for master. I should have it sometime tomorrow.
Will I still be able to get this merged to 1-2-stable? We're currently on the 1.2.x branch, so we need this merged there.

Thanks.

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

[petergoldstein]

@radar I've provided a master version of this PR as #2808 . I'm not sure exactly what the merge strategy is across branches - I'd just like to make sure that this gets merged down to at least 1-2-stable, as that's the branch I'm using. Happy to provide PRs against the requisite branches to make that happen.

[radar]

I've added this to 1-2-stable and your work on #2808 to 1-3-stable and master. I will run it all over the CI server now and see what it says. Thanks for your work Peter :)

[radar]

Hey Peter, 

This work has now been added to the 1-3-stable, 1-2-stable and master branches. Thank you for doing it for us.

On Thu, Apr 4, 2013 at 2:17 PM, Peter Goldstein notifications@github.com
wrote:

@radar I've provided a master version of this PR as #2808 . I'm not sure exactly what the merge strategy is across branches - I'd just like to make sure that this gets merged down to at least 1-2-stable, as that's the branch I'm using. Happy to provide PRs against the requisite branches to make that happen.

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