-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Display admin order page instead of shopfront order page to avoid error 500 #12772
Conversation
@@ -6,7 +6,7 @@ | |||
|
|||
- separator = messages.values.any? ? ": " : ", " | |||
- orders.each_with_index do |order, i| | |||
%a{ href: order_url(order) }>= order.number | |||
%a{ href: spree.edit_admin_order_url(order) }>= order.number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for context, this partial is used in placement_summary_email and confirmation_summary_email which are both sent to the shop's contact email.
I just wanted to check that none of this is sent to customers. It looks fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking this. Perhaps in the future we should arrange mailer views according to their audience, with an admin/
folder for those that are sent to admin users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done on both documenting this, and fixing it!
@@ -6,7 +6,7 @@ | |||
|
|||
- separator = messages.values.any? ? ": " : ", " | |||
- orders.each_with_index do |order, i| | |||
%a{ href: order_url(order) }>= order.number | |||
%a{ href: spree.edit_admin_order_url(order) }>= order.number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking this. Perhaps in the future we should arrange mailer views according to their audience, with an admin/
folder for those that are sent to admin users.
@RachL @filipefurtad0 Could one of you two please test this one as I'm the author? |
Hey @drummer83 , Nice! Thanks for this PR. I could reproduce the bug with a stock restriction test case, and observe that after staging this PR:
Awesome 💪 |
What? Why?
The 'summary of recently placed subscription orders' email contained a broken link in case all product of an order are out of stock (stock = 0), which causes an error 500.
The linked /orders/Rxxx page cannot be used while an order is still in state 'cart'. The error 500 is triggered because the shipping method has not defined at this stage yet.
This PR addresses this problem by linking to the order's page in the admin area instead. This page exists as soon as the order is created, also in state 'cart'. I think that this page is the better place to link to anyways, because it provides all information and options to edit everything for the enterprise user (who is receiving the email).
What should we test?
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.
Dependencies
Documentation updates