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 ability to get variants for a specific product in productVariants query #786

Conversation

dkostenko-jsninja
Copy link
Contributor

In Vendure we have 2 ways to receive product variants by using product and productVariants queries.

product query finds all product variants for a specific product and in case the product has a lot of variants, the performance of this query will be very slow.

productVariants query has options for pagination and filtering but by using this query we cannot receive variants only for a specific product.

Changes in this pull request will fix this problem. So, in the result, we will have the ability to fetch either all variants or only for the specific product by using the productVariants query.

@michaelbromley
Copy link
Member

Hi,
thanks for the contribution! Out of interest, how many product variants are you dealing with?

Could you rebase this PR onto the master branch? - there were some failing tests which I've now fixed, so then we can make sure your changes are passing in CI too.

@dkostenko-jsninja dkostenko-jsninja force-pushed the product-variants-pagination branch from cce19c7 to 11004e8 Compare March 24, 2021 13:52
@dkostenko-jsninja
Copy link
Contributor Author

Hello!

I've just done a rebase onto the master branch.

The problem exists for any product which has hundreds of variants.

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.

Generally looks good - just see my comment about a possible way to do this with less code duplication. Also, an e2e test for this would be good - do you feel confident to add one? If so it would go here

const rawConnection = this.connection.rawConnection;
const qb = this.connection.getRepository(ctx, ProductVariant).createQueryBuilder('productvariant');

const filter = parseFilterParams(rawConnection, ProductVariant, options.filter);
Copy link
Member

Choose a reason for hiding this comment

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

I notice that quite a significant amount of logic here is being duplicated from the ListQueryBuilder.build() method. I am wondering whether it would be possible to use the ListQueryBuilder, and then add the required clauses (ie. .andWhere('productvariant.productId = :productId', { productId }) to the returned SelectQueryBuilder object.

Did you try that approach yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello! Thank you! I agree that would be better to use ListQueryBuilder.build() here. I will do it.

@dkostenko-jsninja dkostenko-jsninja force-pushed the product-variants-pagination branch from 11004e8 to c1f5216 Compare March 31, 2021 14:04
@dkostenko-jsninja
Copy link
Contributor Author

Hello! I implemented ListQueryBuilder.build() in ProductVariantService.getVariantsByProductId() and added e2e test for "get product variants by product id" case.

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.

Thanks, the changes look good!

Just one (very minor) change to the e2e test, and then rebase to latest master because my own bugs will make the ealsticsearch e2e test fail at the current commit 🙃

packages/core/e2e/product.e2e-spec.ts Outdated Show resolved Hide resolved
@dkostenko-jsninja dkostenko-jsninja force-pushed the product-variants-pagination branch from c1f5216 to 0ef5a62 Compare April 2, 2021 13:19
@dkostenko-jsninja
Copy link
Contributor Author

Hello! I did a rebase and replaced productId: '1' with productId: 'T_1' in product.e2e-spec.ts

@michaelbromley michaelbromley merged commit 1da0592 into vendure-ecommerce:master Apr 2, 2021
@michaelbromley
Copy link
Member

Thank you for your contribution! 👍

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