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

Change code to invoke mailers with ids #2808

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@petergoldstein
Contributor

petergoldstein commented Apr 3, 2013

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.

This is the master branch version of pull request #2806

Switch the mailers to be invoked using ids, not model objects. This m…
…akes it easier to support asynchronous mail jobs, as only simple ids need to be persisted.
@joneslee85

This comment has been minimized.

Show comment
Hide comment
@joneslee85

joneslee85 Apr 3, 2013

Member

IMHO we should ditch the backward-comparability check if order is object or id. The rest looks good to merge

Member

joneslee85 commented Apr 3, 2013

IMHO we should ditch the backward-comparability check if order is object or id. The rest looks good to merge

@msevestre

This comment has been minimized.

Show comment
Hide comment
@msevestre

msevestre Apr 3, 2013

In that case it should be stated in the release-notes to avoid pain during
upgrade.
On Apr 3, 2013 4:15 AM, "Trung Lê" notifications@github.com wrote:

IMHO we should ditch the backward-comparability check if order is object
or id. The rest looks good to merge


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/2808#issuecomment-15823904
.

msevestre commented Apr 3, 2013

In that case it should be stated in the release-notes to avoid pain during
upgrade.
On Apr 3, 2013 4:15 AM, "Trung Lê" notifications@github.com wrote:

IMHO we should ditch the backward-comparability check if order is object
or id. The rest looks good to merge


Reply to this email directly or view it on GitHubhttps://github.com/spree/spree/pull/2808#issuecomment-15823904
.

@petergoldstein

This comment has been minimized.

Show comment
Hide comment
@petergoldstein

petergoldstein Apr 3, 2013

Contributor

@joneslee85 I'm fine either way - I put in the backwards compatibility mostly because I'm using 1-2-stable and originally submitted this against that branch. I wanted to minimize compatibility issues on a stable branch.

I'm happy to make that change (which mostly involves updating tests). Just let me know. Thanks.

Contributor

petergoldstein commented Apr 3, 2013

@joneslee85 I'm fine either way - I put in the backwards compatibility mostly because I'm using 1-2-stable and originally submitted this against that branch. I wanted to minimize compatibility issues on a stable branch.

I'm happy to make that change (which mostly involves updating tests). Just let me know. Thanks.

@@ -139,9 +139,14 @@
order.stub :has_available_shipment
order.stub :restock_items!
mail_message = mock "Mail::Message"
Spree::OrderMailer.should_receive(:cancel_email).with(order).and_return mail_message
order_id = nil
Spree::OrderMailer.should_receive(:cancel_email) { |*args|

This comment has been minimized.

@radar

radar Apr 4, 2013

Member

I didn't know you could do this. Neat.

@radar

radar Apr 4, 2013

Member

I didn't know you could do this. Neat.

This comment has been minimized.

@petergoldstein

petergoldstein Apr 7, 2013

Contributor

Yeah, I like this trick. This technique is especially handy when an object is being created in the process of the test and it's not easy to mock the creation method.

@petergoldstein

petergoldstein Apr 7, 2013

Contributor

Yeah, I like this trick. This technique is especially handy when an object is being created in the process of the test and it's not easy to mock the creation method.

@petergoldstein

This comment has been minimized.

Show comment
Hide comment
@petergoldstein

petergoldstein Apr 7, 2013

Contributor

@joneslee85 @msevestre Any conclusions on the open question of backwards compatibility? Thanks.

Contributor

petergoldstein commented Apr 7, 2013

@joneslee85 @msevestre Any conclusions on the open question of backwards compatibility? Thanks.

@radar

This comment has been minimized.

Show comment
Hide comment
@radar

radar Apr 9, 2013

Member

A couple of notes on this:

  1. There is no find! method on AR-based classes. find is the correct method to use, as that will raise the ActiveRecord::RecordNotFound exception that I think you are looking for.
  2. respond_to?(:id) will actually return true for all objects on Ruby 1.8. I've fixed this by changing the code to do an is_a?(Spree::Shipment) or is_a?(Spree::Order) check.

The reason there's so many commits above is because it took me a couple of tries before I realised what was happening.

Member

radar commented Apr 9, 2013

A couple of notes on this:

  1. There is no find! method on AR-based classes. find is the correct method to use, as that will raise the ActiveRecord::RecordNotFound exception that I think you are looking for.
  2. respond_to?(:id) will actually return true for all objects on Ruby 1.8. I've fixed this by changing the code to do an is_a?(Spree::Shipment) or is_a?(Spree::Order) check.

The reason there's so many commits above is because it took me a couple of tries before I realised what was happening.

@radar radar closed this in 8d90ae1 Apr 10, 2013

@imajes imajes deleted the TheRealReal:feature/change_mailers_to_use_ids_on_master branch May 6, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment