-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
docs for tax-inclusive pricing #2159
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.
Looks great!
I just have a couple of minor comments 😄
If tax inclusivity is enabled for the current currency (which is indicated by which region is selected): | ||
|
||
- `original_price` will include the tax amount by default. | ||
- `original_price_includes_tax` will be set to `true`. | ||
- `original_price_incl_tax` will have the same amount as `original_price`. | ||
- `original_tax` is automatically calculated by Medusa. |
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.
Since region prices take precendence over currency prices this is not necessarily right, only if the default price is not specified in terms of the region 😄
- `subtotal`: The total of the line item’s price subtracting the amount in `original_tax_total`. | ||
- `origial_total`: The `subtotal` including the `original_tax_total` amount. | ||
|
||
If tax inclusivity is enabled for the line item, the amount of `tax_total` is calculated using [Medusa’s formula for tax inclusive pricing](#tax-amount-calculation-formula) based on the line item’s tax rates. The calculation takes into account any discounts applied on the item, which means the discount amount is deducted from the original price. |
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.
should we explicitly write that if the includes_tax
flag is set the unit_price
field is including tax?
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.
Good point! I'll add it in
:::info | ||
|
||
When a product is added to the cart, a line item is created to represent that product in the cart. | ||
|
||
::: |
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.
suggestion: think we can omit this. Wdyt?
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 think keeping this one (and the one below) helps those who don't necessarily know much about the architecture get a brief understanding what the terms mean, especially since we don't have any glossary/terminology page that explains what the different terms are
:::info | ||
|
||
When a shipping option is selected, a shipping method is created based on that shipping option. You can learn more about this in the [Shipping Architecture documentation](../shipping/overview.md). | ||
|
||
::: |
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.
suggestion: same as above - think this can be omitted
@shahednasser I realise now that this is opened against |
@olivermrbl no worries! Will do that |
**Why** - It should be possible to update variant props without having to send the prices array with every update
* add feature flag for tax inclusive pricing * update db model for TIP * add migration * set featureflag column decorators * remove unused prop * update tests to reflect feature_flags as any array * fix types * reference key from featureFlag file * use feature flag key in models * fix copy paste mistake * unify spelling * Create gorgeous-experts-guess.md * feat(medusa): create/update endpoints of currency/region/price-lists/shipping-options should allow to pass includes_tax * test(integration): continue to add some integration test * test(integration): continue to add some integration test * test(unit): Fix region service tests * fix(medusa): API unit tests flags management * feat(medusa): Minor cleanup * style(medusa): Fix typo * fix(medusa): rebase * feat(medusa): Replace old tag with the new one * feat(medusa): revert flag * feat(medusa): Cleanup * feat(medusa): feedback * feat(medusa): Rename currency retrieve method * test(medudsa): fix unit tests * chore(medusa): fix oas * feat(medusa): ShippingMethod should include tax setting from parent option (#2021) * feat(medusa): Shipping method should includes tax from parent options * feat(medusa): Condition the includes tax flag to the availability of the feature and add some other tests * test(integration): Move cart/order ff test in separate files * fix: snapshots folder * fix(integration): snapshots * Create calm-baboons-sit.md * test(integration): file naming Co-authored-by: Carlos R. L. Rodrigues <[email protected]> * Feat/tax inclusive pricing extend price selection strategy (#2087) * initial changes to price selection strategy including unit tests * typing for tax calculation * update types and remove region and currency from prices results * fix casing * include tax calculation in priceselectionstrategy * integration tests for tax inclusive pricing price calculations * fix build * include tax inclusive considerations when calculating tax fields for variants * include only "includes_tax" fields from currency and region joins * test to see errors in pipelines * conditionally join featureflagged fields * add "includes_tax" to price list factory * add tests for tax inclusive price list prices and currency prices * fix unit tests * refactor pricing array checks to expect arraycontaining * undo error handler * Feat/tax inclusive pricing flag on generated lineitems (#2108) * include tax inclusive pricing flag on generated lineitems * initial addition of tax inclusivity for lineitem service * add generate test to ensure that includes_tax is set when returned from price selection strategy * add integration test for generating lineitem including tax * add test for negative tax inclusion * add tests for mixed pricing * add negative test for setting tax exclusivity * restructure the setting of includes_tax on lineitems * fix: update cwd to be correct in cart test * feat(medusa): Line item totals calculations (#2123) * feat(medusa): Update totals and tax calculation way to calculate the totals * feat(medusa): remove region feetching from decorate total * feat(medusa): cleanup * test(medusa): fix tax calculation tests * comment * test(integration): cleanup * test(integration): cleanup * fix(medusa): return service missing await * feat(medusa): cleanup * feat(medusa): cleanup * test(integration): fix data * feat(medusa): improve tax calculation readability * test(medusa): improve tax calculation structure case Co-authored-by: Sebastian Rindom <[email protected]> * Feat(medusa): tax inclusive pricing in shipping method tax (#2125) * initial implementation and test * include tax inclusive calculations for getting shipping options * remove inaccurate comment * remove console log * refactor how prices and taxes are set for shipping methods * fix integration tests * remove verbose flag * fix integration tests * remove console log * format util * use util in price service and tax strategy * fix faulty integration test * undo tax calculation strategy changes in favor or Carlos' pr * undo changes to tax calculation strategy tests * round tax amount * feat(medusa): cleanup calculate tax amount utils and its usage (#2136) * feat(medusa): Refund line totals calculation (#2139) Rely on the update of the following pr #2136 **WIP Missing integration tests** **What** Update the totals calculation on the refund line to include the notion of tax inclusive **Test** - Update and add new tests around the refund Fixes CORE-482 * feat(medusa): Tax inclusive discounts calculations (#2137) **What** - Calculate line adjustments correctly taking into account the tax inclusivity - fix totals getLineItemTotals by adjusting the sub total with the original tax amount instead of the tax amount when the unit price includes the taxes **Tests** - The tests create a cart with a percentage discount of 15%, the cart includes 2 items mixing the tax inclusive and validate the items on the result cart as well as the totals on each item. I ve based my calculation validation based on what we have done + some articles around discount apply on price without taxes to validate the output., FIXES CORE-477 * Chore: shipping methods tax inclusive total (#2130) * chore: calculate tax inclusive shipping methods * chore: additional tests and check undefined tax_rate (#2157) * chore: additional tests and check undefined tax_rate * fix: naming + correct price type check * fix: remove price_includes_tax from type * fix: remove price_includes_tax from type Co-authored-by: Philip Korsholm <[email protected]> Co-authored-by: adrien2p <[email protected]> Co-authored-by: Carlos R. L. Rodrigues <[email protected]> Co-authored-by: Philip Korsholm <[email protected]> Co-authored-by: Sebastian Rindom <[email protected]> Co-authored-by: Carlos R. L. Rodrigues <[email protected]>
**Issue number:** #2068 **What:** - removed unused query-builder service files - medusa/src/services/query-builder.js - medusa/src/services/__mocks__/query-builder.js - deleted export from medusa/src/services/index.ts - (extra) deleted documentation files related to QueryBuilderService (QueryBuilderService.md)
…rders (#2167) * feat(webshipper): support personal customs no in orders * docs: update readme with personal customs number info
**What** - return `fileKey` in the response after the file is uploaded to Spaces Co-authored-by: Oliver Windall Juhl <[email protected]>
…s new line char (#2150)
* chore: centrilize eslint rules
**What** - add order editing entities - add repositories - add a feature flag for the order editing feature - add the migrations file RESOLVES CORE-490
Change title in Create a Service documentation
@olivermrbl I've changed base and rebased master 👍🏻 |
@srindom is this all good to merge? |
@shahednasser @srindom Would it make sense to hold off with this part of the documentation until we remove the feature flags? Given that we've decided not to make any public announcements around the feature + you'll have to actively use a feature flag (which is not evident); it might create more harm than good Alternatively, we should add a note stating it's upcoming and will be release within a month? |
@olivermrbl - I think it is fine to include this in docs so that people can explore the features; we will highlight that it is experimental so people should use it at their own risk. |
@srindom Fine by me! |
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.
LGTM!
Closes DOCS-212