Skip to content

[Shopify] Fix #7724: Resolve Option 1 Name from existing Shopify Variant records #8577

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.

Sign up for GitHub

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

Open
tVisionDeb wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[Shopify] Fix #7724: Resolve Option 1 Name from existing Shopify Variant records #8577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b87123b
[Shopify] Fix #7724: Resolve Option 1 Name from existing Shopify Vari…
tVisionDeb May 7, 2026
a1c5593
fix: address bot review feedback for PR #8577
tVisionDeb Jun 11, 2026
5dcfce4
fix: remove copilot-instructions.md and add SetLoadFields in ResolveO…
tVisionDeb Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions src/Apps/W1/Shopify/App/src/Products/Codeunits/ShpfyProductExport.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,8 @@ codeunit 30178 "Shpfy Product Export"
var
Product: Record "Shpfy Product";
ItemAsVariant: Boolean;
JustResolvedOption1Name: Boolean;
CachedOption1Name: Text[50];
begin
if Shop."Sync Prices" or OnlyUpdatePrice then
if Item.Blocked or Item."Sales Blocked" then
Expand Down Expand Up @@ -433,9 +435,13 @@ codeunit 30178 "Shpfy Product Export"
ShopifyVariant."Tariff No." := Item."Tariff No.";
ShopifyVariant."Country/Region of Origin Code" := GetCountryISOCode(Item."Country/Region of Origin Code");
end;
if ShopifyVariant."Option 1 Name" = '' then
ShopifyVariant."Option 1 Name" := 'Variant';
if ShopifyVariant."Option 1 Name" = 'Variant' then
Comment thread
onbuyuka marked this conversation as resolved.
if ShopifyVariant."Option 1 Name" = '' then begin
if CachedOption1Name = '' then
CachedOption1Name := ResolveOption1Name(ShopifyVariant."Product Id", Shop.Code);
ShopifyVariant."Option 1 Name" := CachedOption1Name;
JustResolvedOption1Name := true;
end;
if (ShopifyVariant."Option 1 Name" = 'Variant') or JustResolvedOption1Name then
if ItemAsVariant then
ShopifyVariant."Option 1 Value" := Item."No."
else
Expand Down Expand Up @@ -881,6 +887,33 @@ codeunit 30178 "Shpfy Product Export"
end;
end;

local procedure ResolveOption1Name(ProductId: BigInteger; ShopCode: Code[20]): Text[50]
var
ShpfyVariant: Record "Shpfy Variant";
VariantOption1Name: Text[50];
FirstVariantName: Boolean;
begin
ShpfyVariant.SetRange("Shop Code", ShopCode);
ShpfyVariant.SetRange("Product Id", ProductId);
Comment thread
onbuyuka marked this conversation as resolved.
ShpfyVariant.SetLoadFields("Option 1 Name");
if not ShpfyVariant.FindSet() then

@github-actions github-actions Bot Jun 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

ResolveOption1Name reads all fields unnecessarily

ResolveOption1Name calls FindSet() on "Shpfy Variant" without first calling SetLoadFields. For products with many variants, this fetches all columns from the table when only "Option 1 Name" is needed, increasing I/O and memory pressure.

Recommendation:

  • Add ShpfyVariant.SetLoadFields("Option 1 Name") before FindSet() to limit the data read from the database.
Suggested change
if not ShpfyVariant.FindSet() then
ShpfyVariant.SetRange("Shop Code", ShopCode);
ShpfyVariant.SetRange("Product Id", ProductId);
ShpfyVariant.SetLoadFields("Option 1 Name");
if not ShpfyVariant.FindSet() then
exit('Variant');

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

@tVisionDeb tVisionDeb Jun 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in commit 5dcfce4: added \ShpfyVariant.SetLoadFields(\Option 1 Name\)\ before \FindSet().

exit('Variant');
FirstVariantName := true;
repeat
if ShpfyVariant."Option 1 Name" <> '' then
if FirstVariantName then begin
VariantOption1Name := ShpfyVariant."Option 1 Name";
FirstVariantName := false;
end
else
if VariantOption1Name <> ShpfyVariant."Option 1 Name" then
exit('Variant');
until ShpfyVariant.Next() = 0;
if not FirstVariantName then
exit(VariantOption1Name);
exit('Variant');
end;

local procedure RevertVariantChanges(VariantId: BigInteger)
var
ShopifyVariant: Record "Shpfy Variant";
Expand Down
261 changes: 261 additions & 0 deletions src/Apps/W1/Shopify/Test/Products/ShpfyVariantOption1NameTest.Codeunit.al
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------

namespace Microsoft.Integration.Shopify.Test;

using Microsoft.Integration.Shopify;
using Microsoft.Inventory.Item;
using System.TestLibraries.Utilities;

/// <summary>
/// Tests for Issue #7724: Option 1 Name resolution in FillInProductVariantData.
/// </summary>
codeunit 139649 "Shpfy Variant Option1Name Test"
{
Subtype = Test;
TestType = IntegrationTest;
TestPermissions = Disabled;
TestHttpRequestPolicy = BlockOutboundRequests;

var
Shop: Record "Shpfy Shop";
InitializeTest: Codeunit "Shpfy Initialize Test";
LibraryAssert: Codeunit "Library Assert";
IsInitialized: Boolean;

[Test]
procedure UnitTestOption1NameInheritedFromShopifyOriginProduct()
var
Item: Record Item;
ItemVariant: Record "Item Variant";
ShopifyProduct: Record "Shpfy Product";
ExistingVariant: Record "Shpfy Variant";
NewVariant: Record "Shpfy Variant";
ProductExport: Codeunit "Shpfy Product Export";
begin
// [SCENARIO #7724] Shopify-origin product with one existing variant having Option 1 Name = 'Size'
// -> new BC variant export inherits 'Size' and Option 1 Value = ItemVariant.Code.
Initialize();

// [GIVEN] An item with a variant
CreateItem(Item);
CreateItemVariant(Item, ItemVariant);

// [GIVEN] A Shopify product with one existing variant carrying Option 1 Name = 'Size'
CreateShopifyProduct(ShopifyProduct, Item.SystemId);
CreateShopifyVariant(ExistingVariant, ShopifyProduct, Item.SystemId, 'Size', 'Test');

// [GIVEN] A new blank Shpfy Variant for the same product (being created from BC)
CreateBlankShopifyVariant(NewVariant, ShopifyProduct, Item.SystemId);

// [WHEN] FillInProductVariantData is called
ProductExport.SetShop(Shop);
ProductExport.FillInProductVariantData(NewVariant, Item, ItemVariant);

// [THEN] Option 1 Name is resolved to 'Size' (from existing Shopify variant)
LibraryAssert.AreEqual('Size', NewVariant."Option 1 Name", 'Option 1 Name should be inherited from existing Shopify variant');

// [THEN] Option 1 Value is set to ItemVariant.Code
LibraryAssert.AreEqual(ItemVariant.Code, NewVariant."Option 1 Value", 'Option 1 Value should be set to ItemVariant.Code');
end;

[Test]
procedure UnitTestOption1NameDefaultsToVariantForBCOriginProduct()
var
Item: Record Item;
ItemVariant: Record "Item Variant";
ShopifyProduct: Record "Shpfy Product";
ExistingVariant: Record "Shpfy Variant";
NewVariant: Record "Shpfy Variant";
ProductExport: Codeunit "Shpfy Product Export";
begin
// [SCENARIO #7724] BC-origin product where existing variants already carry Option 1 Name = 'Variant'
// -> new variant still defaults to 'Variant' (regression guard).
Initialize();

// [GIVEN] An item with a variant
CreateItem(Item);
CreateItemVariant(Item, ItemVariant);

// [GIVEN] A Shopify product with an existing BC-origin variant (Option 1 Name = 'Variant')
CreateShopifyProduct(ShopifyProduct, Item.SystemId);
CreateShopifyVariant(ExistingVariant, ShopifyProduct, Item.SystemId, 'Variant', 'VAR1');

// [GIVEN] A new blank Shpfy Variant for the same product
CreateBlankShopifyVariant(NewVariant, ShopifyProduct, Item.SystemId);

// [WHEN] FillInProductVariantData is called
ProductExport.SetShop(Shop);
ProductExport.FillInProductVariantData(NewVariant, Item, ItemVariant);

// [THEN] Option 1 Name stays 'Variant'
LibraryAssert.AreEqual('Variant', NewVariant."Option 1 Name", 'Option 1 Name should default to Variant for BC-origin products');

// [THEN] Option 1 Value is set to ItemVariant.Code
LibraryAssert.AreEqual(ItemVariant.Code, NewVariant."Option 1 Value", 'Option 1 Value should be set to ItemVariant.Code');
end;

[Test]
procedure UnitTestOption1NameDefaultsToVariantWhenNoExistingVariants()
var
Item: Record Item;
ItemVariant: Record "Item Variant";
ShopifyProduct: Record "Shpfy Product";
NewVariant: Record "Shpfy Variant";
ProductExport: Codeunit "Shpfy Product Export";
begin
// [SCENARIO #7724] Product with no existing Shpfy Variant rows -> still defaults to 'Variant'.
Initialize();

// [GIVEN] An item with a variant
CreateItem(Item);
CreateItemVariant(Item, ItemVariant);

// [GIVEN] A Shopify product with no existing variants in BC
CreateShopifyProduct(ShopifyProduct, Item.SystemId);

// [GIVEN] A new blank Shpfy Variant (only one in table for this product)
CreateBlankShopifyVariant(NewVariant, ShopifyProduct, Item.SystemId);

// [WHEN] FillInProductVariantData is called
ProductExport.SetShop(Shop);
ProductExport.FillInProductVariantData(NewVariant, Item, ItemVariant);

// [THEN] Option 1 Name defaults to 'Variant'
LibraryAssert.AreEqual('Variant', NewVariant."Option 1 Name", 'Option 1 Name should default to Variant when no existing variants exist');

// [THEN] Option 1 Value is set to ItemVariant.Code
LibraryAssert.AreEqual(ItemVariant.Code, NewVariant."Option 1 Value", 'Option 1 Value should be set to ItemVariant.Code');
end;

[Test]
procedure UnitTestOption1NameDefaultsToVariantWhenAmbiguous()
var
Item: Record Item;
ItemVariant: Record "Item Variant";
ShopifyProduct: Record "Shpfy Product";
ExistingVariant1: Record "Shpfy Variant";
ExistingVariant2: Record "Shpfy Variant";
NewVariant: Record "Shpfy Variant";
ProductExport: Codeunit "Shpfy Product Export";
begin
// [SCENARIO #7724] Two existing variants with different Option 1 Names (ambiguous) -> falls back to 'Variant'.
Initialize();

// [GIVEN] An item with a variant
CreateItem(Item);
CreateItemVariant(Item, ItemVariant);

// [GIVEN] A Shopify product with two existing variants having conflicting Option 1 Names
CreateShopifyProduct(ShopifyProduct, Item.SystemId);
CreateShopifyVariant(ExistingVariant1, ShopifyProduct, Item.SystemId, 'Color', 'Red');
CreateShopifyVariant(ExistingVariant2, ShopifyProduct, Item.SystemId, 'Size', 'Large');

// [GIVEN] A new blank Shpfy Variant for the same product
CreateBlankShopifyVariant(NewVariant, ShopifyProduct, Item.SystemId);

// [WHEN] FillInProductVariantData is called
ProductExport.SetShop(Shop);
ProductExport.FillInProductVariantData(NewVariant, Item, ItemVariant);

// [THEN] Option 1 Name falls back to 'Variant' due to ambiguity
LibraryAssert.AreEqual('Variant', NewVariant."Option 1 Name", 'Option 1 Name should fall back to Variant when existing variants have conflicting names');

// [THEN] Option 1 Value is set to ItemVariant.Code
LibraryAssert.AreEqual(ItemVariant.Code, NewVariant."Option 1 Value", 'Option 1 Value should be set to ItemVariant.Code');
end;

[Test]
procedure UnitTestOption1NameAndValuePreservedOnUpdatePath()
var
Item: Record Item;
ItemVariant: Record "Item Variant";
ShopifyProduct: Record "Shpfy Product";
ExistingVariant: Record "Shpfy Variant";
ProductExport: Codeunit "Shpfy Product Export";
begin
// [SCENARIO #7724] Update path: existing Shpfy Variant with a custom Option 1 Name (e.g. 'Color')
// -> name and value remain untouched after FillInProductVariantData.
Initialize();

// [GIVEN] An item with a variant
CreateItem(Item);
CreateItemVariant(Item, ItemVariant);

// [GIVEN] A Shopify product with one fully-populated variant (already imported from Shopify)
CreateShopifyProduct(ShopifyProduct, Item.SystemId);
CreateShopifyVariant(ExistingVariant, ShopifyProduct, Item.SystemId, 'Color', 'Red');

// [WHEN] FillInProductVariantData is called on the already-populated variant (update path)
ProductExport.SetShop(Shop);
ProductExport.FillInProductVariantData(ExistingVariant, Item, ItemVariant);

// [THEN] Option 1 Name is NOT overwritten - it keeps 'Color'
LibraryAssert.AreEqual('Color', ExistingVariant."Option 1 Name", 'Option 1 Name must not be overwritten on the update path');

// [THEN] Option 1 Value is NOT overwritten - it keeps 'Red'
LibraryAssert.AreEqual('Red', ExistingVariant."Option 1 Value", 'Option 1 Value must not be overwritten on the update path');
end;

local procedure Initialize()
begin
if IsInitialized then
exit;

IsInitialized := true;
Shop := InitializeTest.CreateShop();
Commit();
end;

local procedure CreateItem(var Item: Record Item)
var
ProductInitTest: Codeunit "Shpfy Product Init Test";
begin
Item := ProductInitTest.CreateItem();
end;

local procedure CreateItemVariant(Item: Record Item; var ItemVariant: Record "Item Variant")
begin
Comment thread
onbuyuka marked this conversation as resolved.
ItemVariant.Init();
ItemVariant.Validate("Item No.", Item."No.");
ItemVariant.Code := CopyStr(Item."No." + 'V1', 1, MaxStrLen(ItemVariant.Code));
ItemVariant.Description := 'Test Variant';
ItemVariant.Insert(true);
end;

local procedure CreateShopifyProduct(var ShopifyProduct: Record "Shpfy Product"; ItemSystemId: Guid)
begin
ShopifyProduct.Init();
ShopifyProduct.Id := Random(2147483647);
ShopifyProduct."Item SystemId" := ItemSystemId;
ShopifyProduct."Shop Code" := Shop.Code;
ShopifyProduct."Has Variants" := true;
ShopifyProduct.Insert(false);
end;

local procedure CreateShopifyVariant(var ShopifyVariant: Record "Shpfy Variant"; ShopifyProduct: Record "Shpfy Product"; ItemSystemId: Guid; Option1Name: Text[50]; Option1Value: Text[255])
begin
ShopifyVariant.Init();
ShopifyVariant.Id := Random(2147483647);
ShopifyVariant."Product Id" := ShopifyProduct.Id;
ShopifyVariant."Item SystemId" := ItemSystemId;
ShopifyVariant."Shop Code" := Shop.Code;
ShopifyVariant."Option 1 Name" := Option1Name;
ShopifyVariant."Option 1 Value" := CopyStr(Option1Value, 1, MaxStrLen(ShopifyVariant."Option 1 Value"));
ShopifyVariant.Insert(false);
end;

local procedure CreateBlankShopifyVariant(var ShopifyVariant: Record "Shpfy Variant"; ShopifyProduct: Record "Shpfy Product"; ItemSystemId: Guid)
begin
ShopifyVariant.Init();
ShopifyVariant.Id := Random(2147483647);
ShopifyVariant."Product Id" := ShopifyProduct.Id;
ShopifyVariant."Item SystemId" := ItemSystemId;
ShopifyVariant."Shop Code" := Shop.Code;
ShopifyVariant."Option 1 Name" := '';
ShopifyVariant."Option 1 Value" := '';
ShopifyVariant.Insert(false);
end;
}
Loading