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 asset importer assigns asset on default channel when importing on non default channel #2801

Conversation

mschipperheyn
Copy link
Collaborator

@mschipperheyn mschipperheyn commented Apr 19, 2024

Description

Adds RequestContext to asset import for collections.

The populator loads assets into collections. However, the RequestContext is not passed along. This means that if you import collections on any channel that is not default, the assets get assigned to the default channel only.

As a result a channel specific collection import fails because an empty asset array results that is assumed to be non empty, and referenced.

Resolves issue #2787

I adjusted test on the populate spec and was surprised that even on the 'populating a non-default channel' the test passed without my changes. On further inspection it seems that that test uses the default channel for its work instead of the intended channel-2. So, it seems to be not validating what it's supposed to validate. That might be because I'm running a filtered test or because the test has a somewhat unique setup that destroys the server after the additional test channel is created. Initially I thought it might a cache refresh issue but later I realized that channel-2 wasn't in the database during the test. So, writing the test became a bit of a rabbit hole and I bailed after a while. Perhaps someone can enlighten me on the test setup and how it's supposed to work.

BTW: after running npm install, testing doesn't work out of the box. had to remove package-lock.json to be able to run tests at all bc of darwin related errors on optional dependencies

Breaking changes

None

Screenshots

You can add screenshots here if applicable.

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • [x ] I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

netlify bot commented Apr 19, 2024

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit a06409c
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/66226cd757aa33000857cfd5

@michaelbromley michaelbromley merged commit c7a28b7 into vendure-ecommerce:master Apr 22, 2024
16 checks passed
@michaelbromley
Copy link
Member

Thanks for the fix. Regarding the e2e tests - if you like you can share a branch with the changes you made and when I have time I could look into it.

@mschipperheyn mschipperheyn deleted the fix/channel-collection-asset-import branch May 2, 2024 01:40
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