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

feat(core): Add CSV import in multiple languages #1199

Merged

Conversation

heiko-r
Copy link
Contributor

@heiko-r heiko-r commented Oct 29, 2021

It turned out a little more complex than anticipated. I tried to maintain as much compatibility with the monolingual import as possible, but this required adding an optional mainLanguage parameter to parseProducts, which is used to specify the language to use for the Product.name, Product.slug, etc. properties. So even though the ParsedProduct and ParsedProductVariant interfaces have additional properties now, I think it should be compatible with most, if not all, existing usages. Not sure if you consider this a breaking change.

Also a note: I pushed with --no-verify, because there's an error during e2e testing:

@vendure/core: FAIL e2e/custom-fields.e2e-spec.ts (11.346 s)
@vendure/core:   ● Custom fields › validation › invalid datetime
@vendure/core:     expect(received).toEqual(expected) // deep equality
@vendure/core:     Expected: StringContaining "The custom field 'validateDateTime' value [2019-01-01T05:25:00.000Z] is less than the minimum [2019-01-01T08:30]"
@vendure/core:     Received: "fail is not defined"
@vendure/core:       13 |     };
@vendure/core:       14 | }
@vendure/core:     > 15 |
@vendure/core:          | ^
@vendure/core:       at Object.<anonymous> (utils/assert-throws-with-message.ts:15:33)

It wasn't me! 😬 It was already broken before I made any changes! 😬😉 (and I also didn't investigate it any further yet)

@michaelbromley
Copy link
Member

Thank you! I will review this at the beginning of next week.

Copy link
Member

@michaelbromley michaelbromley left a comment

Choose a reason for hiding this comment

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

This is very good! I made a bunch of comments, some of which might then require some changes.

The addition of the mainLanguage arg is not a breaking change since it is optional and I would be very surprised if anyone is using that method directly in their custom code.

Re the "fail is not defined" - I think it may be related to jestjs/jest#11698

@michaelbromley michaelbromley merged commit 4754954 into vendure-ecommerce:minor Nov 5, 2021
@michaelbromley
Copy link
Member

Love all that deleted code in the second commit 😄

Thank you for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants