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.

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

[Customer account & order] Change order totals to account for refunds #2179

Merged
merged 4 commits into from
Jan 30, 2023

Conversation

ludoboludo
Copy link
Contributor

@ludoboludo ludoboludo commented Dec 16, 2022

PR Summary:

In our customer pages, the total for orders never reflects refunds. So it creates a poor experience for the merchant's clients seeking confirmation that their order was in fact refunded.

Why are these changes introduced?

Fixes #2154

What approach did you take?

I switched our use of order.total_price to order.total_net_amount. In our docs, you can see the difference with order.total_price.

When I compare now the order from the admin and the customer account page, it reflects properly the information:

Screenshot of the main-account page

Screenshot of the main-order page

Other considerations

Gotta make sure we're not showing totals that are off. But based on the information from the admin it looks right.

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

It will change the totals on customer order pages and account summary.
It also adds a Refunded row when there has been a refund.

Testing steps/scenarios

  • You can test by creating an account, completing an order (test order), then partially or fully refunding it in the admin.
  • Then make sure the information reflected on your account page is correct.

Demo links

Checklist

colspan="2"
data-label="{{ 'customer.order.total_refunded' | t }}"
>
{{ order.total_refunded_amount | money_with_currency }}
Copy link
Member

Choose a reason for hiding this comment

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

How certain is the math here? Like are we 100% sure that total_net_amount - total_refunded_amount = total_price?

If you haven't spoken from someone on the Orders team, we should pull someone in for 👀. Refunds can get really edge-case-y.

Copy link
Contributor Author

@ludoboludo ludoboludo Dec 19, 2022

Choose a reason for hiding this comment

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

Ill double check but from what I'm seeing, some theme partners are using order.total_net_amount as well 👍 (Maestrooo)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if the math worked well when you partially refund shipping on top of that and it worked well but the only thing is that we don't display the subsequent refunds. Unsure if it's relevant to buyers to be aware if there was multiple attempt to refund partially an order. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like maybe it's added noise, I'm not sure how useful it is to be displaying each refund individually 🤔 Plus if we do that then it means we will also need a total which will add even more info/content to the order.

On the order object, there are the transactions information available but it doesn't contain much info. We can check if a transaction is a refund and output its amount, but again not sure how useful. Here is how it could look:

Screenshot

Here is the information available for transactions:

Screenshot

I think if we could have had more details about what was refunded in that transaction it'd be more useful but right now it doesn't seem to add much. The total refund amount seem enough.

Copy link
Contributor

@melissaperreault melissaperreault Dec 20, 2022

Choose a reason for hiding this comment

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

The total refund amount seem enough

Aligned for now! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

For the maths and all, might be worth investigating how we communicate/implement these on the email confirmation that is sent. That might be an additional place to validate were good or not. 🙌

@eugenekasimov eugenekasimov self-requested a review December 19, 2022 18:49
@eugenekasimov
Copy link
Contributor

eugenekasimov commented Dec 19, 2022

Everything looks good to me! 👍

  • You can test by creating an account, completing an order (test order), then partially or fully refunding it in the admin.
  • Then make sure the information reflected on your account page is correct.

The only minor thing I could think of was maybe we should confirm it with UX folks?
I mean this: total_refunded: "Refunded".

@melissaperreault melissaperreault self-requested a review December 19, 2022 20:29
@melissaperreault
Copy link
Contributor

The only minor thing I could think of was maybe we should confirm it with UX folks?
I mean this: total_refunded: "Refunded".

I am good with the term "Refunded" it aligns with how we communicate this information in the admin to merchants. This language is clear and have the same meaning for buyers. 👌

In this case we have both; Partially refunded or Refunded 👍

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

Thank you Ludo for putting this together. 🎉

The only observation I have is how we can better communicate the Refunded or Partially refunded amount. Displaying a negative sign in front of the value will help do the math for customers. It would help bring more clarity as they read the table from the right only. It would follow the admin presentation as well.

colspan="2"
data-label="{{ 'customer.order.total_refunded' | t }}"
>
{{ order.total_refunded_amount | money_with_currency }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if the math worked well when you partially refund shipping on top of that and it worked well but the only thing is that we don't display the subsequent refunds. Unsure if it's relevant to buyers to be aware if there was multiple attempt to refund partially an order. 🤔

@@ -80,7 +80,7 @@
{{ order.fulfillment_status_label }}
</td>
<td headers="RowOrder ColumnTotal" role="cell" data-label="{{ 'customer.orders.total' | t }}">
{{ order.total_price | money_with_currency }}
{{ order.total_net_amount | money_with_currency }}
Copy link
Contributor Author

@ludoboludo ludoboludo Dec 20, 2022

Choose a reason for hiding this comment

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

Let me know what you think is best @melissaperreault:

Screenshot

Copy link
Contributor

@melissaperreault melissaperreault Dec 20, 2022

Choose a reason for hiding this comment

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

Outside the currency symbol - to validate if this would be different for other languages but it's best like that and aligns with the admin too 🙌 🎉

Copy link
Contributor

@melissaperreault melissaperreault left a comment

Choose a reason for hiding this comment

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

I tested a full refund and a partial refund scenario. The placement and the clarity is good to me with the added negative sign in front. It really helps!

I cannot speak if this is the right implementation code to account for translations with the negative sign, but otherwise from Theme design UX layout, this improvement is good! Thanks Ludo! 🚀

eugenekasimov
eugenekasimov previously approved these changes Dec 20, 2022
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

It's looking good, Ludo!

The current implementation of negative sign looks fine too, I don't see any disadvantage of the way it added.

@eugenekasimov eugenekasimov self-requested a review December 22, 2022 17:05
@ludoboludo ludoboludo self-assigned this Jan 9, 2023
@KaichenWang
Copy link
Contributor

Looks great!

As others have mentioned, it would be nice to follow up and more closely reflect the buyer email layout:

  • The total remains the total paid (unaffected by refunds)
  • Support for multiple refund amounts

Screenshot 2023-01-27 at 1 17 53 PM

@ludoboludo
Copy link
Contributor Author

ludoboludo commented Jan 27, 2023

I created a separate branch to explore the changes it would mean to match the email notification a bit more and created a PR with the changes: #2264
I'll have a chat with Meli to see what she think. I'd rather ship once with the right layout than twice 😅

@ludoboludo ludoboludo merged commit 53547c5 into main Jan 30, 2023
@ludoboludo ludoboludo deleted the customer-order-refund-fix branch January 30, 2023 15:00
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Customer order] Show refund amount and subtract it from the total
5 participants