medusajs / medusa

The world's most flexible commerce platform.
https://medusajs.com
MIT License
26.23k stars 2.66k forks source link

Product Export can shuffle the option values #5712

Closed mortenengel closed 12 months ago

mortenengel commented 1 year ago

Bug report

Describe the bug

When exporting product from Medusa, the values for the options can be shuffled around in the resulting CSV file. The products were initially uploaded through the same file as well.

System information

Medusa version (including plugins): "@medusajs/admin": "7.1.1", "@medusajs/cache-inmemory": "1.8.8", "@medusajs/cache-redis": "1.8.8", "@medusajs/event-bus-local": "1.9.6", "@medusajs/event-bus-redis": "1.8.9", "@medusajs/icons": "1.0.0", "@medusajs/medusa": "1.16.0", "@medusajs/ui": "1.0.0", Node.js version: 18:17 Database: PostGres Operating system: Linux

Steps to reproduce the behavior

  1. Export products with more than 1 option
  2. See if it happens (seems random)

Expected behavior

Option values created in the correct column

Screenshots

output from /api/admin/products: image

output in CSV: image

Code snippets

-

Additional context

I will do a PR for this soon, just need a bit of time on my hands. It seems fairly easy to fix in appendOptionsDescriptors of https://github.com/medusajs/medusa/blob/develop/packages/medusa/src/strategies/batch-jobs/product/export.ts We just need to start choosing the first X column ids to iterate over, and then select the values by id rather than by index.

fPolic commented 1 year ago

hey @mortenengel, thanks for reporting this one. Indeed, the fix should be to select variant option value by a product option_id instead of index in the appendOptionsDescriptors method. Let us know if we can assist with the PR.

mortenengel commented 1 year ago

Hi @fPolic

I'm looking at it now - but I see it's not quite as simple as I initially imagined. Since taking the option name and the option value happens in seperate function calls, fetching the matching column value to the chosen name column ID is not trivial. There is the potential of matching what has been done with prices in getProductRelationsDynamicColumnsShape to find all options - but that would also dramatically change the output format of the file (since then we would end up with one specific column for each global option, rather than just a numbered list, going to the max possible number of options on any one product).

My best bet for fixing this without changing the output of the file, is instead to keep on going by index - but sorting the options on product/variant by option ID before taking the index. I am however unsure what will happen in the case where some variants may not have values for all options?

Do you agree on this approach, or would you prefer that I solve it in a different manner?

Edit: I was using the word header which was confusing. I meant the name column


    for (let i = 0; i < maxOptionsCount; ++i) {
      const columnNameNameBuilder = (this.columnsDefinition["Option Name"]!
        .exportDescriptor as DynamicProductExportDescriptor)!
        .buildDynamicColumnName

      const columnNameName = columnNameNameBuilder(i)

      this.columnsDefinition[columnNameName] = {
        name: columnNameName,
        exportDescriptor: {
          accessor: (productOption: Product) =>
            productOption?.options?.sort((a, b) => a.id.localeCompare(b.id))[i]?.title ?? "",
          entityName: "product",
        },
      }

      const columnNameValueBuilder = (this.columnsDefinition["Option Value"]!
        .exportDescriptor as DynamicProductExportDescriptor)!
        .buildDynamicColumnName

      const columnNameNameValue = columnNameValueBuilder(i)

      this.columnsDefinition[columnNameNameValue] = {
        name: columnNameNameValue,
        exportDescriptor: {
          accessor: (variant: ProductVariant) =>
            variant?.options?.sort((a, b) => a.option_id.localeCompare(b.option_id))[i]?.value ?? "",
          entityName: "variant",
        },
      }
    }
  }```
fPolic commented 1 year ago

hey @mortenengel, with the suggested approach we could end up with a wrong combinations of options<>values if a variant doesn't have option values for some product options.

The fix would be to change the variant option value selection on L:503 to rely on the option id as you suggested initially:

 accessor: (variant: ProductVariant, product?: Product) =>
    variant?.options.find(
      (ov) => ov.option_id === product!.options[i]?.id
    )?.value ?? "",

To achieve this we would also need to modify the variant accessor function to accept Product as a second argument since it is needed to compute the line and then pass the product when the accessor is called on L:600: columnSchema.accessor(variant, product)

mortenengel commented 1 year ago

@fPolic great!

I did not consider the posibility of adding extra arguments to the accessor - that's definitely a much cleaner solution.

I will test later and make the PR.

mortenengel commented 1 year ago

Added pull request: https://github.com/medusajs/medusa/pull/5781

I decided to make a new entityname "productAndVariant" - to keep the lines clean. By just adding product as a hidden second argument to the variant descriptor, we could risk that someone else made an untyped descriptor with some other second argument and we would get a clash.