-
-
Notifications
You must be signed in to change notification settings - Fork 731
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
Add 'Voucher:' before voucher code on order confirmation emails #12998
Add 'Voucher:' before voucher code on order confirmation emails #12998
Conversation
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.
Nice! Thank you.
I have a suggestion below. Are you up for that?
2097d0c
to
7c0fa56
Compare
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, that's a good solution.
But actually we prefer not to change all the locale files, only en.yml
. All other files are managed with Transifex and shouldn't be updated directly, so this could cause conflicts. Could you please remove those changes?
For reference:
https://github.com/openfoodfoundation/openfoodnetwork/wiki/Internationalisation-%28i18n%29#development
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.
Nice one 👍 Thanks @kernal053 🙏
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.
Thankyou!
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.
Hmm, sorry I didn't notice this earlier, but this will hide any adjustments that are not of type "Voucher".
I'm not sure, but I think other adjustments such as Enterprise Fees or tax might appear here.
If so, it would be worth adding a spec here: https://guide.openfoodnetwork.org/basic-features/shopfront/enterprise-fees
Are you willing to look into this?
Apologies, I overlook other adjustments. Will fix the code. Right now, we're only excluding And adding spec, I think |
Thanks for looking back into this.
I'm sorry, I pasted the wrong link. You're right, it was meant to be a link to |
d2fbfbd
to
2f66320
Compare
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.
Looks good, thanks @kernal053 🙏
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.
Great!
it "includes Shipping label" do | ||
expect(confirmation_email_for_customer.body).to include("Shipping:") | ||
expect(confirmation_email_for_shop.body).to include("Shipping:") | ||
expect(cancellation_email.body).to include("Shipping:") |
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.
I see that the shipping fee comes from the factory order_with_totals_and_distribution. Maybe it's worth a comment here explaining that, but never mind :)
2f66320
to
bad32e2
Compare
Hi @kernal053, I've tested your PR now and here are the results.
ResultsA few things are worth mentioning.
SummaryYour PR is solving the issue well. I will open new issues to address the remaining two minor things I've found. If you like, feel free to tackle them as well. Thank you so much again! 🙏 |
What? Why?
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
Preview (en)
Preview (es)