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(core): Default search plugin query for non-default language fails #2515

Conversation

hans-rollingridges-dev
Copy link
Contributor

@hans-rollingridges-dev hans-rollingridges-dev commented Nov 7, 2023

Description

The - default search plugin - search query for a non-standard language fails. If there is a product variant with non-standard translation, the standard language variant will also be included in the search results. I.e. resulting in search result records with duplicate product variant ID. Depending on the logic for sorting and grouping by, the outcome will be different.

Breaking changes

Screenshots

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • 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 Nov 7, 2023

Deploy Preview for effervescent-donut-4977b2 canceled.

Name Link
🔨 Latest commit 4117a5b
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/655dcc4f22d5640008773df9

@hans-rollingridges-dev hans-rollingridges-dev force-pushed the fix/default-search-plugin-query-for-non-default-language-fails branch from 7f6d297 to a0c86c2 Compare November 7, 2023 15:37
@hans-rollingridges-dev
Copy link
Contributor Author

Draft PR. Improving e2e tests.

@hans-rollingridges-dev
Copy link
Contributor Author

@michaelbromley , is there a benchmark test available to measure the PR impact on shop search query performance?
There's another solution which does not require a subquery. However it requires group by and having clause for each supported strategy.

@michaelbromley
Copy link
Member

We have a load testing setup described here: https://github.com/vendure-ecommerce/vendure/tree/master/packages/dev-server#load-testing

There's no script specifically for testing the search, but you could create one based on one of the existing scripts.

@hans-rollingridges-dev hans-rollingridges-dev force-pushed the fix/default-search-plugin-query-for-non-default-language-fails branch from 6096726 to 72cdabf Compare November 17, 2023 16:01
@hans-rollingridges-dev hans-rollingridges-dev marked this pull request as ready for review November 17, 2023 16:02
@hans-rollingridges-dev hans-rollingridges-dev changed the title Fix/default search plugin query for non-default language fails fix(core): Default search plugin query for non-default language fails Nov 19, 2023
@hans-rollingridges-dev hans-rollingridges-dev force-pushed the fix/default-search-plugin-query-for-non-default-language-fails branch from 9e40120 to c2a688e Compare November 22, 2023 09:22
@michaelbromley michaelbromley merged commit fb0ea13 into vendure-ecommerce:master Nov 22, 2023
18 checks passed
@michaelbromley
Copy link
Member

Thank you @hans-rollingridges-dev for your excellent 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