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

feat: display tax in regular checkout #2773

Merged
merged 11 commits into from
Sep 29, 2023
Merged

feat: display tax in regular checkout #2773

merged 11 commits into from
Sep 29, 2023

Conversation

asadali145
Copy link
Contributor

@asadali145 asadali145 commented Sep 28, 2023 •
edited
Loading

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
    • Desktop screenshots
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

Fixes #2769

What's this PR do?

  • Apply taxes on the checkout page.
  • Display taxes in Email Receipt and Dashboard Receipt View.

How should this be manually tested?

NOTE: I will write tests for the frontend in a followup PR. Also, I was not able to test the Cybersource checkout as my Payments were declined each time. I tested with the discounts then I tested the Receipt by deleting Coupon redemptions.

  • Create a couple of courses and products.
  • Try to enroll and Verify that the tax is calculated and it is as per the tax rate. Tax should be visible in the checkout order summary as per the designs.
  • Proceed with the payment in Cybersource.
  • Once enrollment is complete, Verify that the Email Receipt has Tax listed as per the designs and the values are correct.
  • View Receipt in the Dashboard and verify the Tax and other details as per the designs.
  • Now repeat the same process after applying some discount codes.

Screenshots (if appropriate)

Checkout:
Screen Shot 2023-09-29 at 7 45 04 PM

Email:
Screen Shot 2023-09-29 at 7 46 13 PM

Dashboard Receipt:
Screen Shot 2023-09-29 at 7 45 41 PM
Screen Shot 2023-09-29 at 7 45 52 PM

@arslanashraf7 arslanashraf7 mentioned this pull request Sep 29, 2023
Merged
8 tasks
Base automatically changed from jkachel/tax-rates to master September 29, 2023 13:01
@asadali145 asadali145 marked this pull request as ready for review September 29, 2023 15:11
@asadali145
Copy link
Contributor Author

I am keeping an eye on the tests. Hopefully, they will pass this time. You can start reviewing this @jkachel

@asadali145
Copy link
Contributor Author

Looking into the CI deployment failure.

@arslanashraf7
Copy link
Contributor

Looking into the CI deployment failure.

Are they similar? I left a comment about that in other PR #2772 (comment).

@asadali145
Copy link
Contributor Author

Are they similar? I left a comment about that in other PR #2772 (comment).

Yeah, this one is also due to the migration inconsistency. We can ignore it. I have tested it thoroughly.

jkachel
jkachel requested changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

Code looks good, and functional tests were successful - had one minor issue with the email, and this should be good to go, I think.

mail/templates/product_order_receipt/body.html Outdated
Comment on lines 33 to 34
<strong style="font-weight: 700;">Tax:</strong> ${{ line.tax_paid }}<br>
<strong style="font-weight: 700;">Total Paid:</strong> ${{ line.total_paid }}
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to be quantized - in testing I got a Tax line that was $21.675000 (which is right, but too many decimals).

FWIW - I added a couple of template tags in the mail app to handle this too; they do work differently than the route you've taken though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be fixed in 426b61e

jkachel
jkachel approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@jkachel jkachel left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@asadali145 asadali145 merged commit b10831c into master Sep 29, 2023
3 checks passed
This was referenced Sep 29, 2023
Closed
Closed
@asadali145 asadali145 mentioned this pull request Oct 2, 2023
Merged
3 tasks
This was referenced Oct 2, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkachel jkachel jkachel approved these changes

@arslanashraf7 arslanashraf7 Awaiting requested review from arslanashraf7

@pdpinch pdpinch Awaiting requested review from pdpinch

@mbilalmughal mbilalmughal Awaiting requested review from mbilalmughal

Assignees
No one assigned
Labels
Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Display tax collection amounts on xPRO checkout page and receipts #2518
4 participants
@asadali145 @arslanashraf7 @jkachel @odlbot