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.

Sign up for GitHub

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

Spree 2 Upgrade - MailMethod - Fixed bug on order_mailer_decorator (only visible in spree 2) #2587

Merged
merged 3 commits into from
Sep 4, 2018
Merged

Spree 2 Upgrade - MailMethod - Fixed bug on order_mailer_decorator (only visible in spree 2) #2587

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
27f5e6d
Fixed bug on order_mailer_decorator. When cancel_email is called with…
luisramos0 Aug 25, 2018
921c8df
Removed unnecessary cancel_email from order_email_decorator (spree on…
luisramos0 Aug 31, 2018
8004abb
Improved Order mailer and Subscriptions mailer specs by removing depe…
luisramos0 Aug 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions app/mailers/spree/order_mailer_decorator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
def cancel_email(order, resend = false)
@order = find_order(order)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@luisramos0 luisramos0 Aug 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

subject = (resend ? "[#{t(:resend).upcase}] " : '')
subject += "#{Spree::Config[:site_name]} #{t('order_mailer.cancel_email.subject')} ##{order.number}"
mail(to: order.email, from: from_address, subject: subject)
subject += "#{Spree::Config[:site_name]} #{t('order_mailer.cancel_email.subject')} ##{@order.number}"
mail(to: @order.email, from: from_address, subject: subject)
end

def confirm_email_for_customer(order, resend = false)
Expand Down