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

fix(medusa, medusa-js): Use price selection strategy for GET /admin/variants #2270

Merged
merged 14 commits into from
Oct 13, 2022

Conversation

kasperkristensen
Copy link
Contributor

What

  • Adds the use of price selection strategy to the endpoint GET /admin/variants
  • Updates medusa-js to reflect this change (expanding the parameters).

Testing

  • Adds a new integration test validating that returned variants are now of type PricedVariant, with the expected fields: original_price, calculated_price, etc.

Why

  • Our current RMA flows (in our admin dashboard) relied heavily on simply using order.tax_rate to calculate variant prices in the different RMA menus. As taxes in Medusa, have become feature complete this approach had become very naive and has several potential issues. Moving the responsibility for calculating the correct prices guarantees that we always show the correct prices to admins.

@kasperkristensen kasperkristensen requested a review from a team as a code owner September 28, 2022 16:54
@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2022

🦋 Changeset detected

Latest commit: cc4f06d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@medusajs/medusa Patch
@medusajs/medusa-js Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

Overall great work! Have added a couple of comments here and there 👍

Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

LGTM!

@kasperkristensen
Copy link
Contributor Author

@olivermrbl could you also accept that the requested changes were addressed as it is blocking the PR from being merged 😄

Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM!

Sorry for the slow f/u @kasperkristensen 😅

@kodiakhq kodiakhq bot merged commit 69e5797 into develop Oct 13, 2022
@kodiakhq kodiakhq bot deleted the fix/return-priced-variants-admin branch October 13, 2022 08:54
ymaheshwari1 pushed a commit to ymaheshwari1/medusa that referenced this pull request Oct 26, 2022
…variants` (medusajs#2270)

**What**
- Adds the use of price selection strategy to the endpoint `GET /admin/variants`
- Updates medusa-js to reflect this change (expanding the parameters).

**Testing**
- Adds a new integration test validating that returned variants are now of type PricedVariant, with the expected fields: original_price, calculated_price, etc.

**Why**
- Our current RMA flows (in our admin dashboard) relied heavily on simply using `order.tax_rate` to calculate variant prices in the different RMA menus. As taxes in Medusa, have become feature complete this approach had become very naive and has several potential issues. Moving the responsibility for calculating the correct prices guarantees that we always show the correct prices to admins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants