Order emails include ineligible adjustments #1555

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

greinacker commented May 17, 2012

Currently, if you have say 2 promotions:

  • 5% off orders over $100
  • 10% off orders over $200

If you have an order for $240, you'll have a 10% discount. But the order confirmation email will show:

Subtotal: $240.00
  Promotion (5 percent off) -$12.00
  Promotion (10 percent over 200) -$24.00
  Shipping $0.00
Order Total: $216.00

The total is correct, but it lists a promotion that's not actually applied.

The attached fix changes things to only show the eligible promotions:

Subtotal: $270.00
  Promotion (10 percent over 200) -$27.00
  Shipping $0.00
Order Total: $243.00

Targeted at 1-1-stable, but master has the same issue as well.

Contributor

greinacker commented May 17, 2012

By the way, the workaround for this is pretty simple (just provide a new confirm_email.text.erb in the app), so if you just want to merge into master that's probably fine...

Contributor

citrus commented May 17, 2012

Could this be related to #1551 at all?

Contributor

greinacker commented May 17, 2012

Certainly possible...you might try the patch and see if it suppresses the ineligible promotions you're seeing...

Contributor

citrus commented May 17, 2012

It seems to me that this patch is in the wrong place, no? Shouldn't order.adjustments only return adjustments that have already been applied to the order? As in an order's adjustments shouldn't ever contain ineligible adjustments, right?

Contributor

greinacker commented May 17, 2012

Well, that I don't know...I modeled this fix off of what I saw in, say, _order_details.html.erb, which uses this technique to filter down the adjustments that will be displayed.

https://github.com/spree/spree/blob/1-1-stable/core/app/views/spree/admin/shared/_order_details.html.erb

Contributor

citrus commented May 17, 2012

Hmm.. that just seems kind of backwards to me... What does the rest of the spree team have to say on this?

/cc @BDQ @cmar @LBRapid

Member

BDQ commented May 18, 2012

@citrus - I think ineligible adjustments do stay associated with an order now (in case they become eligible again).

I think maybe the order adjustments association should be scoped to only include eligible adjustments, and we add a new scope for ineligible ones?

@cmar - you refactored a lot of the promotions / adjustments stuff, does this make sense?

Member

cmar commented May 18, 2012

This is a good fix. Thanks @greinacker

Adjustments are added to the orders based on events defined in the promotion. Once it is attached it can go in and out of eligibility. The emails should scope the events by eligibility.

Owner

schof commented May 20, 2012

Fix looks good but what about a spec for this?

Contributor

greinacker commented May 20, 2012

@schof I'd be happy to add a spec...but when I made the change, it wasn't immediately obvious how to go about it. Since testing the fix relies on having promotions, and promotions are a) not in spree_core and b) not required by spree_core, it seemed like it would add some unwanted dependencies.

I also looked in order_mailer_spec.rb, thinking maybe I could stub out two promotions and test that way, but a) I don't think I know enough about how exactly promotions work to stub it, and b) again it seemed like a dependency that doesn't belong in spree_core tests.

And the test could go into promo, but then we're testing core functionality in a promo spec, and that seemed odd also.

So anyway, not sure how to proceed on that.

radar closed this in 4a9d7fb May 21, 2012

Member

radar commented May 21, 2012

Fixed (with spec) in @4a9d7fbc752f368cb8a5f41af21908e79420e920 for 1-1-stable and @9c71c50a4e3ad7a8f0510b8a9cb8ff79924a91b2 for master.

@greinacker: Adjustments are a core function, and can be tested without needing to involve promo at all. Please see the test code in the commits to see how this is done.

Contributor

greinacker commented May 21, 2012

@radar ah! That totally makes sense. I was so focused on promotions (since that's where I originally noticed the problem) it didn't occur to me that one could test it using a regular adjustment. Thanks!

@tvdeyen tvdeyen pushed a commit to magiclabs/spree that referenced this pull request Jun 1, 2012

@radar radar Ensure that order confirmation and cancel emails do not include ineli…
…gible adjustments

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