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

ElasticSearch plugin: ability to get all Product assets #1054

Closed
adr7dev opened this issue Aug 26, 2021 · 5 comments
Closed

ElasticSearch plugin: ability to get all Product assets #1054

adr7dev opened this issue Aug 26, 2021 · 5 comments

Comments

@adr7dev
Copy link

adr7dev commented Aug 26, 2021

Is your feature request related to a problem? Please describe.
It's quite common to have more than one asset in product listings (e.g. when your frontend needs switching between two images on hover). So in that case featuredAsset is not enough. When you store the assets in Product, you want to have access to it from ProductVariant.

For now there is a problem using ElasticsearchPlugin to get product.assets as the array does't exist due to not being joined on fetch (see the example below).

customProductMappings: {
    secondaryFeaturedAssetPreview: {
        graphQlType: 'String',
        valueFn: (product) => {
            // product.assets doesn't exist
            // so you can't return product assets info
        },
    },
}

Example query for above scenario:

{
  search(input: { collectionId: 1, groupByProduct: true }) {
    items {
      productName
      customMappings {
        ...on CustomProductMappings {
          secondaryFeaturedAssetPreview
        }
      }
    }
  }
}

Describe the solution you'd like
Any ability to get Product assets without making indexing much slower.

Describe alternatives you've considered.
Actually the case when you use all assets in listings is very rare. Many people need just one more except the featured. So maybe the better way is to add secondary featured asset? But this is not a minimalist approach - basically one featured image is enough.

@Izayda
Copy link
Contributor

Izayda commented Sep 7, 2021

+1 for that feature

@michaelbromley
Copy link
Member

To solve this we would need to introduce some point which allows an async function so you can do a DB join for the data you need when indexing. I don't want to join all assets by default, as this would be an unnecessary penalty for those who do not need them. So the best solution is to make it so you can execute arbitrary code during the indexing process.

One way would be to make the customMappings valueFn async and pass the Injector. This would allow you to query for the assets of that product. One possible drawback is that it might slow down indexing too much. Needs checking.

Another way might be to allow modification of the relations being selected here. This would keep the valueFn sync and would not require an extra DB query.

@Izayda
Copy link
Contributor

Izayda commented Sep 17, 2021

I can get this

@Izayda
Copy link
Contributor

Izayda commented Sep 29, 2021

Hi! I have several ideas of implementing #1054:

  1. Allow to add relations here
    product.variants.map(v => v.id),
    to access them in valueFn
  2. Add parameter maxAssetsCount to index maxAssetsCount assets of variant in regular assets field

First implementation is more generic, but we don't have list type in customMappings. I plan to use search method to generate feed for google merchant etc, so, for me, i'll need to create nine custom fields.

Second implementation is less generic, but it can easily expose all assets like in product query

@michaelbromley
Copy link
Member

Hi @Izayda
The first idea sounds worth exploring. Support for lists in customMappings can be implemented first, and then this feature built on top. I prefer that to the 2nd option, because it is inevitable that people will at some point want to index other relations.

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

No branches or pull requests

3 participants