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

Jump to bottom

Display admin order page instead of shopfront order page to avoid error 500 #12772

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Display admin order page instead of shopfront order page to avoid error 500 #12772

merged 1 commit into from
Aug 22, 2024

Conversation

drummer83
Copy link
Contributor

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.
image

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.
image

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).
image

What should we test?

  • Make a subscription order fail for different reasons, e.g. No stock at all, insufficient stock, failed payment, already processed etc.
  • Check the 'summary of recently placed subscription orders' email.
  • Click the link to the order mentioned in the email.
  • Verify that the edit order page for that order is shown.
  • Verify that the emails sent to customers remain unchanged.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

@drummer83 drummer83 marked this pull request as ready for review August 14, 2024 13:14
mkllnk
mkllnk approved these changes Aug 15, 2024
View reviewed changes
app/views/subscription_mailer/_summary_detail.html.haml
@@ -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
Copy link
Member

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.

Copy link
Member

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.

dacook
dacook approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@dacook dacook left a 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!

app/views/subscription_mailer/_summary_detail.html.haml
@@ -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
Copy link
Member

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.

@drummer83
Copy link
Contributor Author

@RachL @filipefurtad0 Could one of you two please test this one as I'm the author?
Thanks!

@filipefurtad0 filipefurtad0 self-assigned this Aug 19, 2024
@filipefurtad0 filipefurtad0 added no-staging-UK A tag which does not trigger deployments, indicating a server is being used no-staging-FR A tag which does not trigger deployments, indicating a server is being used pr-staged-fr staging.coopcircuits.fr and removed no-staging-UK A tag which does not trigger deployments, indicating a server is being used no-staging-FR A tag which does not trigger deployments, indicating a server is being used labels Aug 22, 2024
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Aug 22, 2024 •
edited
Loading

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:

  • the link on the 'summary of recently placed subscription orders' email points to the respective order edit page. The order is still on cart state, as expected (pic below)

image

  • verified that this PR introduces no changes to customers emails.

Awesome 💪
Good to go!!

@filipefurtad0 filipefurtad0 merged commit ef9ca33 into openfoodfoundation:master Aug 22, 2024
57 checks passed
@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Aug 22, 2024
@drummer83 drummer83 deleted the E500_sub branch August 22, 2024 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkllnk mkllnk mkllnk approved these changes

@dacook dacook dacook approved these changes

Assignees

@drummer83 drummer83

@filipefurtad0 filipefurtad0

Labels
None yet
Projects
OFN Delivery board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subscriptions] Error 500 when clicking link to order of a failed subscription due to stock zero
4 participants
@drummer83 @filipefurtad0 @mkllnk @dacook