Spree 2 Upgrade - MailMethod - Fixed bug on order_mailer_decorator (only visible in spree 2) #2587
Conversation
… order id in paramater order, order.number will fail. This is only seen in spree 2 because only spree 2 calls this method with order id. Bug introduced here: openfoodfoundation@32d2adc#diff-aa82d768109073ea8ad7858146630be4R9. Spree change introduced here: spree/spree@8d90ae1#diff-1e2ba31309f1f1abd2a4b626036d316fR94
luisramos0
changed the title
| @@ -6,8 +6,8 @@ | |||
| def cancel_email(order, resend = false) | |||
| @order = find_order(order) | |||
There was a problem hiding this comment.
but by creating an order ivar we could be possibly shadowing any ivar already set in the OrderMailer definition (very likely given the name of the class). IMO creating a local var does the job and it is less risky.
We could rename the order argument to order_or_order_id. Isn't that what is happening?
There was a problem hiding this comment.
very interesting little comment...
because I dont think your main point is valid, we have to set and keep @order because it's used in the mailer views.
BUT
you made me realise that after this
0de691b
we can just remove this cancel_email method :-) and use the spree order_mailer one.
Your comment still applies… but it’s now spree code. I think we will keep (in this case, the not so good) spree code as it is… this wrt to cancel_email method.
I really liked your order_or_order_id variable name so I decided to use it in the remaining methods (I love making code smells more explicit like this function parameter that takes all types of variations without indication).
As I was refactoring... I also extracted email subject related copy pasted logic to a new build_subject method.
And as I was improving this space, I also removed MailMethod dependency from both this order_mailer and the subscription mailer specs.
I kept the initial commit as it makes historical sense and added 2 new commits with what I mention in this comment.
|
I'll wait for @mkllnk or someone else to review the last version fo this PR. |
| subject += "#{Spree::Config[:site_name]} #{t('order_mailer.confirm_email.subject')} ##{@order.number}" | ||
| def confirm_email_for_customer(order_or_order_id, resend = false) | ||
| @order = find_order(order_or_order_id) | ||
| subject = build_subject("#{t('order_mailer.confirm_email.subject')}", resend) |
There was a problem hiding this comment.
You don't need the string interpolation any more:
subject = build_subject(t('order_mailer.confirm_email.subject'), resend)There are a couple more cases in this file.
There was a problem hiding this comment.
ah, true, well spotted! Fixed.
luisramos0
force-pushed
the
spree2_order_mailer_decorator_bug
branch
from
f7caf64 to
9c9fa75
Compare
…e is equivalent after ofn PR#2589). Also refactored remaining order_mailer_decorator methods by extracting duplicated code.
…ndency to old mail method
luisramos0
force-pushed
the
spree2_order_mailer_decorator_bug
branch
from
9c9fa75 to
8004abb
Compare
When cancel_email is called with order id in paramater order, order.number will fail. This is only seen in spree 2 because only spree 2 calls this method with order id. Bug introduced here: 32d2adc#diff-aa82d768109073ea8ad7858146630be4R9. Spree change introduced here: spree/spree@8d90ae1#diff-1e2ba31309f1f1abd2a4b626036d316fR94
We dont need to put this in master because this is not a problem in spree 1 because cancel_email is always called with order (not order id) in spree 1.
What? Why?
Related to #2020
Detected while fixing mailmethod related spec errors.
This error appears in spec/controllers/spree/orders_controller_spec.rb:407 (but only seen after PR #2574 fixes are applied as well).
What should we test?
After PR #2574 is applied, this PR gets spec/controllers/spree/orders_controller_spec.rb:404 to green.
How is this related to the Spree upgrade?
This is part of getting the spree2 branch build to green.
Dependencies
This is related to PR #2574 but can go first.