fix(medusa): Export/import fixes including export fields that contains new line char #2150

adrien2p · 2022-09-05T13:01:26Z

What

The actual export behaviour does not take into account potential new lines and/or double quote usage as part of the data.
The idea is to escape any new line char as well as double quote in those same cells since they have to be surrounded by double quote a per the csv rfc.

Also, the export column naming has been aligned with the import to add more consistency. The delimited as not been changed from , to ; as it can be done later also to add more consistency between export/import

Also, fixes some import issues around

transaction conflict
non awaited methods that fails and therefore are failing out of transaction (after releasing) and breaks the flow
files not created but still trying to use them to read the operations from during the job process
file naming miss match due to enum way of working
csv parsing failure that is not properly catched

From my tests, I do not get a job stuck in a state anymore

How

Create a new util that take care of that task and allow to externalise that needs which could be used else where.
Also create an util that revert the formatting before being inserted in the DB so we have the export and import managed

Tests

Add unit tests for the new util and update the existing tests for the product export in order to take into account new line char in the test data.

Also update current integration tests

NOTE
Im another PR:

I think that we should have a common descriptor between the export and import which would hold the naming of the column for each property which could be consume by both strategy and avoid having different column naming for the same prop in both. The idea is that if you export the data to bulk update some prop or something, you would like to be able to re import that file without having to change the column names or the separator. wdyt @fPolic @olivermrbl
If you agree on that then I will create a linear issue

FIXES CORE-344

changeset-bot · 2022-09-05T13:01:30Z

🦋 Changeset detected

Latest commit: 3b3db22

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

This PR includes changesets to release 1 package

Name	Type
@medusajs/medusa	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

integration-tests/api/src/services/local-file-service.js

packages/medusa/src/strategies/batch-jobs/product/import.ts

fPolic

Nice work @adrien2p, added a single discussion point

olivermrbl

Update: Nvm.!

olivermrbl

Great job! Overall LGTM - just have a couple of comments on clean up and a though 👍

packages/medusa/src/services/product-variant.ts

packages/medusa/src/strategies/batch-jobs/product/import.ts

olivermrbl · 2022-09-09T08:20:48Z

packages/medusa/src/strategies/batch-jobs/product/import.ts

+      })
+  }
+
+  private static buildFilename(


thought(non-blocking): Would it make sense to extract this to a util as the functionality can be converted to more generic logic in a quite straightforward manner

withExt could be the actual extension instead of a flag

it si very coupled to the import naming. for a global util we should come up with another approach no?

srindom

Two small fixes :)

Also a thought: are there any considerations to take into account regarding the host environment? E.g., should the fact that Windows uses \r\n for newlines and UNIX-based OS'es use \n be something we account for?

I tried opening the output here in Excel and it results in the cell containing all the escaped characters. Is it possible to instead have the new line characters be taken into account by excel so the result looks like below?

packages/medusa/src/strategies/batch-jobs/product/index.ts

adrien2p · 2022-09-09T09:24:38Z

Two small fixes :)

Also a thought: are there any considerations to take into account regarding the host environment? E.g., should the fact that Windows uses \r\n for newlines and UNIX-based OS'es use \n be something we account for?

I tried opening the output here in Excel and it results in the cell containing all the escaped characters. Is it possible to instead have the new line characters be taken into account by excel so the result looks like below?

By any chance did you try to open the data from the snapshot? because you can't test it that way since jest double the escape of the data which result in this. I ve added a check in the integration tests so that you can see how it works.
Also, for the test it needs to be escaped to validate on one line otherwise it is a multiline string. But the output as you can see is correct 👍

I will export some more file just to re check just in case 😀

For the new line char, we are not aware of the OS of the user, the only os we know is the one on which the server is running. I Do you have an idea?

adrien2p · 2022-09-09T09:57:17Z

I ve improved the export such that the result looks like that :) @srindom I think I ve went too far in the escape 👍 well spotted

srindom · 2022-09-09T10:00:12Z

I ve improved the export such that the result looks like that :) @srindom I think I ve too far in the escape 👍 well spotted

Perfect 😍

srindom · 2022-09-09T10:02:11Z

For the new line char, we are not aware of the OS of the user, the only os we know is the one on which the server is running. I Do you have an idea?

Don't have an idea - and it might also be an overoptimization at this point; let's not spent more time on this now 👍

adrien2p · 2022-09-09T10:12:16Z

For the new line char, we are not aware of the OS of the user, the only os we know is the one on which the server is running. I Do you have an idea?

Don't have an idea - and it might also be an overoptimization at this point; let's not spent more time on this now 👍

I agree for deffering it to later, the only think I can come up with is that as part of the job creation, the client send the os on which it is running using that https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform the only problem with that is that if another user authenticate and get the file, the user could be on another platform 😂 otherwise we can generate both version and when the user download it we return the appropriate version depending on the user platform which seams the best solution i can see on top of my head for now

…properly

adrien2p · 2022-09-12T07:38:07Z

@srindom is everything ok and ready to be merged for you? if yes, then I ll merge it 👍

srindom

LGTM!

…s new line char (#2150)

* docs for tax-inclusive pricing * changes based on feedback * changes based on feedback * fix link * fix: make prices optional param when updating a variant (#2155) **Why** - It should be possible to update variant props without having to send the prices array with every update * feat(medusa): Tax-inclusive pricing (#2131) * 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 <rodrigolr@gmail.com> * 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 <skrindom@gmail.com> * 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 <philip.korsholm@hotmail.com> Co-authored-by: adrien2p <adrien.deperetti@gmail.com> Co-authored-by: Carlos R. L. Rodrigues <rodrigolr@gmail.com> Co-authored-by: Philip Korsholm <88927411+pKorsholm@users.noreply.github.com> Co-authored-by: Sebastian Rindom <skrindom@gmail.com> Co-authored-by: Carlos R. L. Rodrigues <37986729+carlos-r-l-rodrigues@users.noreply.github.com> * Remove unused QueryBuilderService (#2104) **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) * docs for tax-inclusive pricing * changes based on feedback * changes based on feedback * feat(medusa-fulfillment-webshipper): Support personal customs no in orders (#2167) * feat(webshipper): support personal customs no in orders * docs: update readme with personal customs number info * fix(medusa-file-spaces): return `fileKey` for Spaces upload (#2171) **What** - return `fileKey` in the response after the file is uploaded to Spaces Co-authored-by: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com> * fix(medusa): Export/import fixes including export fields that contains new line char (#2150) * feat(medusa-js,medusa-react,medusa): Add missing Currency endpoints (#2185) * fix(medusa): Resent notification replaces parent notification * chore: Bump medusa-file-minio * fix(medusa): allow address updates on carts w/o existing address (#2176) * chore: Bump minor version of plugins * chore: Centralise ESLint rules (#2162) * chore: centrilize eslint rules * feat: order editing data model (#2184) **What** - add order editing entities - add repositories - add a feature flag for the order editing feature - add the migrations file RESOLVES CORE-490 * fix link * fix(medusa): Check for Sales Channel on product import (#2202) * docs: change title in Create a Service documentation (#2201) Change title in Create a Service documentation * chore(release): Publish * added links to sidebar Co-authored-by: Sebastian Rindom <skrindom@gmail.com> Co-authored-by: Oliver Windall Juhl <59018053+olivermrbl@users.noreply.github.com> Co-authored-by: Philip Korsholm <philip.korsholm@hotmail.com> Co-authored-by: adrien2p <adrien.deperetti@gmail.com> Co-authored-by: Carlos R. L. Rodrigues <rodrigolr@gmail.com> Co-authored-by: Philip Korsholm <88927411+pKorsholm@users.noreply.github.com> Co-authored-by: Carlos R. L. Rodrigues <37986729+carlos-r-l-rodrigues@users.noreply.github.com> Co-authored-by: Dorian <46839236+DorianMazur@users.noreply.github.com> Co-authored-by: Frane Polić <16856471+fPolic@users.noreply.github.com> Co-authored-by: Kasper Fabricius Kristensen <45367945+kasperkristensen@users.noreply.github.com> Co-authored-by: Bhargava Prabu Reddy <prabu0reddy777@gmail.com> Co-authored-by: olivermrbl <oliver@mrbltech.com> Co-authored-by: sabakhilji <52318459+sabakhilji@users.noreply.github.com>

adrien2p added the type: bug label Sep 5, 2022

adrien2p self-assigned this Sep 5, 2022

adrien2p closed this Sep 5, 2022

adrien2p reopened this Sep 5, 2022

adrien2p changed the title ~~fix(medusa): Product export should escaped double quote and new line properly~~ fix(medusa): Export/import fixes including export fields that contains new line char Sep 7, 2022

adrien2p force-pushed the fix/product-export-including-new-line-char branch from a343a4e to 8157cd0 Compare September 7, 2022 09:36

adrien2p marked this pull request as ready for review September 7, 2022 12:11

adrien2p requested a review from a team as a code owner September 7, 2022 12:11

adrien2p requested review from fPolic and olivermrbl September 7, 2022 12:12

adrien2p force-pushed the fix/product-export-including-new-line-char branch from be67243 to 784d4ad Compare September 7, 2022 13:00

adrien2p requested a review from carlos-r-l-rodrigues September 7, 2022 13:13

carlos-r-l-rodrigues reviewed Sep 7, 2022

View reviewed changes

integration-tests/api/src/services/local-file-service.js Show resolved Hide resolved

fPolic reviewed Sep 7, 2022

View reviewed changes

packages/medusa/src/strategies/batch-jobs/product/import.ts Outdated Show resolved Hide resolved

fPolic reviewed Sep 7, 2022

View reviewed changes

packages/medusa/src/strategies/batch-jobs/product/import.ts Outdated Show resolved Hide resolved

fPolic reviewed Sep 7, 2022

View reviewed changes

adrien2p force-pushed the fix/product-export-including-new-line-char branch from a5f04a8 to 784d4ad Compare September 8, 2022 07:52

adrien2p requested a review from pKorsholm September 8, 2022 07:59

fPolic approved these changes Sep 8, 2022

View reviewed changes

olivermrbl reviewed Sep 8, 2022

View reviewed changes

olivermrbl approved these changes Sep 9, 2022

View reviewed changes

srindom requested changes Sep 9, 2022

View reviewed changes

packages/medusa/src/strategies/batch-jobs/product/index.ts Outdated Show resolved Hide resolved

packages/medusa/src/strategies/batch-jobs/product/index.ts Outdated Show resolved Hide resolved

adrien2p requested review from srindom and olivermrbl September 9, 2022 10:53

adrien2p added 12 commits September 9, 2022 15:48

fix(medusa): Product export should escaped double quote and new line …

bab1ccd

…properly

cleanup

d950404

WIP

20344c8

fix(medusa): import/export strategy

1a42ab8

fix(medusa) Import products

9567b20

test(medusa): Fix tests

e6b0e77

feat(medusa): Address feedback

78f0c5a

feat(medusa): Cleanup process method

65ee627

feat(medusa): Ensure preprocessing is using the proper transaction

fb26901

test(integration): Improve test case

f90c404

test(integration): Improve test case

d2716d2

address cleanup

724e552

adrien2p force-pushed the fix/product-export-including-new-line-char branch from 37058ef to 724e552 Compare September 9, 2022 13:48

Create small-walls-poke.md

3b3db22

srindom approved these changes Sep 12, 2022

View reviewed changes

adrien2p merged commit b6161d2 into develop Sep 12, 2022

shahednasser pushed a commit that referenced this pull request Sep 14, 2022

fix(medusa): Export/import fixes including export fields that contain…

a4b026a

…s new line char (#2150)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(medusa): Export/import fixes including export fields that contains new line char #2150

fix(medusa): Export/import fixes including export fields that contains new line char #2150

adrien2p commented Sep 5, 2022 •

edited

Loading

changeset-bot bot commented Sep 5, 2022 •

edited

Loading

fPolic left a comment

olivermrbl left a comment •

edited

Loading

olivermrbl left a comment

olivermrbl Sep 9, 2022

olivermrbl Sep 9, 2022

adrien2p Sep 9, 2022 •

edited

Loading

srindom left a comment

adrien2p commented Sep 9, 2022 •

edited

Loading

adrien2p commented Sep 9, 2022 •

edited

Loading

srindom commented Sep 9, 2022

srindom commented Sep 9, 2022

adrien2p commented Sep 9, 2022 •

edited

Loading

adrien2p commented Sep 12, 2022

srindom left a comment

fix(medusa): Export/import fixes including export fields that contains new line char #2150

fix(medusa): Export/import fixes including export fields that contains new line char #2150

Conversation

adrien2p commented Sep 5, 2022 • edited Loading

changeset-bot bot commented Sep 5, 2022 • edited Loading

🦋 Changeset detected

fPolic left a comment

Choose a reason for hiding this comment

olivermrbl left a comment • edited Loading

Choose a reason for hiding this comment

olivermrbl left a comment

Choose a reason for hiding this comment

olivermrbl Sep 9, 2022

Choose a reason for hiding this comment

olivermrbl Sep 9, 2022

Choose a reason for hiding this comment

adrien2p Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

srindom left a comment

Choose a reason for hiding this comment

adrien2p commented Sep 9, 2022 • edited Loading

adrien2p commented Sep 9, 2022 • edited Loading

srindom commented Sep 9, 2022

srindom commented Sep 9, 2022

adrien2p commented Sep 9, 2022 • edited Loading

adrien2p commented Sep 12, 2022

srindom left a comment

Choose a reason for hiding this comment

adrien2p commented Sep 5, 2022 •

edited

Loading

changeset-bot bot commented Sep 5, 2022 •

edited

Loading

olivermrbl left a comment •

edited

Loading

adrien2p Sep 9, 2022 •

edited

Loading

adrien2p commented Sep 9, 2022 •

edited

Loading

adrien2p commented Sep 9, 2022 •

edited

Loading

adrien2p commented Sep 9, 2022 •

edited

Loading