[5.x] Fix applying default variant to provisional drafts

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 - Fixed a bug where all catalog pricing could be removed if a catalog pricing rule was deleted before its catalog pricing job ran. ([#4374](https://github.com/craftcms/commerce/issues/4374))
 - Fixed a bug where non-admin users couldn’t create inventory locations. ([#4376](https://github.com/craftcms/commerce/issues/4376))
+- Fixed a bug where saving a provisional product draft could result in two variants being flagged as the default. ([#4361](https://github.com/craftcms/commerce/issues/4361))
 - Fixed [high-severity](https://github.com/craftcms/cms/security/policy#severity--remediation) RCE vulnerabilities. (GHSA-2m9x-wwwf-f798, GHSA-4rf5-rp2q-cwvw)
 - Fixed a [low-severity](https://github.com/craftcms/cms/security/policy#severity--remediation) authorization bypass vulnerability. (GHSA-xh76-fg84-9j86)
 

src/elements/Product.php

@@ -1167,9 +1167,10 @@ protected function cpEditUrl(): ?string
      */
     public function getDefaultVariant(bool $includeDisabled = false): ?Variant
     {
-        $defaultVariant = $this->getVariants($includeDisabled)->firstWhere('id', $this->defaultVariantId);
+        $variants = $this->getVariants($includeDisabled);
+        $defaultVariant = $variants->firstWhere('id', $this->defaultVariantId);
 
-        return $defaultVariant ?: $this->getVariants($includeDisabled)->first();
+        return $defaultVariant ?: $variants->first();
     }
 
     /**
@@ -1662,7 +1663,13 @@ public function afterSave(bool $isNew): void
             $record->typeId = $this->typeId;
 
             $defaultVariant = $this->getDefaultVariant();
-            $record->defaultVariantId = $defaultVariant->id ?? null;
+            $defaultVariantId = $defaultVariant->id ?? null;
+
+            if ($defaultVariantId && $this->getIsCanonical() && $defaultVariant->getIsDerivative()) {
+                $defaultVariantId = $defaultVariant->getCanonicalId();
+            }
+
+            $record->defaultVariantId = $defaultVariantId;
             $record->defaultSku = $defaultVariant?->getSkuAsText() ?? '';
             $record->defaultPrice = $defaultVariant?->getBasePrice() ?? 0.0;
             $record->defaultHeight = $defaultVariant->height ?? 0.0;
@@ -1671,7 +1678,7 @@ public function afterSave(bool $isNew): void
             $record->defaultWeight = $defaultVariant->weight ?? 0.0;
 
             // Make sure to update the object
-            $this->defaultVariantId = $defaultVariant->id ?? null;
+            $this->defaultVariantId = $defaultVariantId;
             $this->defaultSku = $defaultVariant?->getSkuAsText();
             $this->defaultPrice = $defaultVariant?->getBasePrice() ?? 0.0;
             $this->defaultHeight = $defaultVariant->height ?? 0;
@@ -1691,6 +1698,27 @@ public function afterSave(bool $isNew): void
 
             $this->setDirtyAttributes($dirtyAttributes);
 
+            if ($this->getIsCanonical()) {
+                // @TODO Remove in Commerce 6.0 if the `isDefault` column is removed from the variants table
+                $staleDefaultCondition = ['and', ['primaryOwnerId' => $this->id], ['isDefault' => true]];
+                if ($defaultVariantId) {
+                    $staleDefaultCondition[] = ['not', ['id' => $defaultVariantId]];
+                }
+                Craft::$app->getDb()->createCommand()->update(
+                    Table::VARIANTS,
+                    ['isDefault' => false],
+                    $staleDefaultCondition
+                )->execute();
+
+                if ($defaultVariantId) {
+                    Craft::$app->getDb()->createCommand()->update(
+                        Table::VARIANTS,
+                        ['isDefault' => true],
+                        ['and', ['id' => $defaultVariantId], ['isDefault' => false]]
+                    )->execute();
+                }
+            }
+
             if ($this->getIsCanonical() &&
                 isset($this->typeId) &&
                 $productType->isStructure

src/elements/Variant.php

@@ -1069,6 +1069,7 @@ public function afterSave(bool $isNew): void
 
             $record->primaryOwnerId = $this->getPrimaryOwnerId();
 
+            // @TODO Remove in Commerce 6.0 if the `isDefault` column is removed from the variants table
             if ($this->getOwner()->getIsCanonical()) {
                 $record->isDefault = $this->isDefault;
             }

src/elements/actions/SetDefaultVariant.php

@@ -88,6 +88,7 @@ public function performAction(ElementQueryInterface $query): bool
             ['id' => $product->id]
         )->execute();
 
+        // @TODO Remove in Commerce 6.0 if the `isDefault` column is removed from the variants table
         if ($product->getIsCanonical()) {
             // Remove previous default
             Craft::$app->getDb()->createCommand()->update(

src/elements/db/VariantQuery.php

@@ -554,7 +554,12 @@ protected function beforePrepare(): bool
         }
 
         if (isset($this->isDefault)) {
-            $this->subQuery->andWhere(Db::parseBooleanParam('isDefault', $this->isDefault, false));
+            $isDefaultCondition = '[[commerce_variants.id]] = [[commerce_products.defaultVariantId]]';
+            if ($this->isDefault) {
+                $this->subQuery->andWhere(new Expression($isDefaultCondition));
+            } else {
+                $this->subQuery->andWhere(new Expression("not ($isDefaultCondition) or [[commerce_products.defaultVariantId]] is null"));
+            }
         }
 
         if (isset($this->minQty)) {

tests/unit/elements/product/ProductDefaultVariantTest.php

@@ -0,0 +1,205 @@
+<?php
+/**
+ * @link https://craftcms.com/
+ * @copyright Copyright (c) Pixel & Tonic, Inc.
+ * @license https://craftcms.github.io/license/
+ */
+
+namespace craftcommercetests\unit\elements\product;
+
+use Codeception\Test\Unit;
+use Craft;
+use craft\commerce\db\Table;
+use craft\commerce\elements\actions\SetDefaultVariant;
+use craft\commerce\elements\Product;
+use craft\commerce\elements\Variant;
+use craft\db\Query;
+use craft\db\Table as CraftTable;
+use craft\helpers\Db;
+use craftcommercetests\fixtures\ProductFixture;
+use DateTime;
+
+/**
+ * ProductDefaultVariantTest
+ *
+ * @author Pixel & Tonic, Inc. <support@pixelandtonic.com>
+ */
+class ProductDefaultVariantTest extends Unit
+{
+    /**
+     * @var \UnitTester
+     */
+    protected $tester;
+
+    /**
+     * @return array
+     */
+    public function _fixtures(): array
+    {
+        return [
+            'products' => [
+                'class' => ProductFixture::class,
+            ],
+        ];
+    }
+
+    public function testDefaultVariantSetOnProvisionalDraftIsApplied(): void
+    {
+        [$product, $variantA, $variantB] = $this->_createProduct('draft-default');
+
+        try {
+            $draft = Craft::$app->getDrafts()->createDraft($product, provisional: true);
+
+            foreach ([$variantB->id => 1, $variantA->id => 2] as $variantId => $sortOrder) {
+                Db::update(CraftTable::ELEMENTS_OWNERS, ['sortOrder' => $sortOrder], [
+                    'ownerId' => $draft->id,
+                    'elementId' => $variantId,
+                ]);
+            }
+
+            $action = new SetDefaultVariant();
+            self::assertTrue($action->performAction(
+                Variant::find()->ownerId($draft->id)->id($variantB->id)->status(null)
+            ));
+
+            self::assertSame($variantA->id, $this->_defaultVariantId($product->id));
+            self::assertSame([$variantA->id], $this->_flaggedVariantIds($product->id));
+
+            $draft = Product::find()->id($draft->id)->drafts()->provisionalDrafts()->status(null)->one();
+            self::assertNotNull($draft);
+            Craft::$app->getDrafts()->applyDraft($draft);
+
+            self::assertSame($variantB->id, $this->_defaultVariantId($product->id));
+            self::assertSame([$variantB->id], $this->_flaggedVariantIds($product->id));
+            self::assertSame(
+                [$variantB->id, $variantA->id],
+                Variant::find()->productId($product->id)->status(null)->orderBy(['sortOrder' => SORT_ASC])->ids()
+            );
+            self::assertSame($variantB->id, Product::find()->id($product->id)->status(null)->one()?->getDefaultVariant()?->id);
+        } finally {
+            Craft::$app->getElements()->deleteElementById($product->id, Product::class, null, true);
+        }
+    }
+
+    public function testDefaultVariantSetToDerivativeVariantIsApplied(): void
+    {
+        [$product, $variantA, $variantB] = $this->_createProduct('draft-derivative');
+
+        try {
+            $draft = Craft::$app->getDrafts()->createDraft($product, provisional: true);
+
+            $draftVariantB = Variant::find()->ownerId($draft->id)->id($variantB->id)->status(null)->one();
+            self::assertNotNull($draftVariantB);
+            $derivativeVariantB = Craft::$app->getElements()->duplicateElement($draftVariantB, [
+                'canonicalId' => $variantB->id,
+                'primaryOwner' => $draft,
+                'owner' => $draft,
+                'sortOrder' => $draftVariantB->getSortOrder(),
+            ]);
+            Db::delete(CraftTable::ELEMENTS_OWNERS, ['elementId' => $variantB->id, 'ownerId' => $draft->id]);
+            self::assertNotSame($variantB->id, $derivativeVariantB->id);
+
+            $action = new SetDefaultVariant();
+            self::assertTrue($action->performAction(
+                Variant::find()->ownerId($draft->id)->id($derivativeVariantB->id)->status(null)
+            ));
+            self::assertSame($derivativeVariantB->id, $this->_defaultVariantId($draft->id));
+
+            $draft = Product::find()->id($draft->id)->drafts()->provisionalDrafts()->status(null)->one();
+            self::assertNotNull($draft);
+            Craft::$app->getDrafts()->applyDraft($draft);
+
+            self::assertSame($variantB->id, $this->_defaultVariantId($product->id));
+            self::assertSame([$variantB->id], $this->_flaggedVariantIds($product->id));
+            self::assertSame($variantB->id, Product::find()->id($product->id)->status(null)->one()?->getDefaultVariant()?->id);
+        } finally {
+            Craft::$app->getElements()->deleteElementById($product->id, Product::class, null, true);
+        }
+    }
+
+    public function testLegacyIsDefaultFlagsAreClearedWhenThereIsNoDefaultVariant(): void
+    {
+        [$product, $variantA, $variantB] = $this->_createProduct('no-default');
+
+        try {
+            foreach ([$variantA, $variantB] as $variant) {
+                $variant->enabled = false;
+                Craft::$app->getElements()->saveElement($variant, false);
+            }
+
+            $product = Product::find()->id($product->id)->status(null)->one();
+            Craft::$app->getElements()->saveElement($product, false);
+
+            self::assertNull($this->_defaultVariantId($product->id));
+            self::assertSame([], $this->_flaggedVariantIds($product->id));
+        } finally {
+            Craft::$app->getElements()->deleteElementById($product->id, Product::class, null, true);
+        }
+    }
+
+    /**
+     * @param string $handle
+     * @return array{0: Product, 1: Variant, 2: Variant}
+     */
+    private function _createProduct(string $handle): array
+    {
+        $product = new Product();
+        $product->title = "Default Variant Test Product $handle";
+        $product->typeId = 2001;
+        $product->slug = "default-variant-test-product-$handle";
+        $product->enabled = true;
+        $product->enabledForSite = true;
+        $product->postDate = new DateTime('now');
+
+        $variantA = new Variant();
+        $variantA->title = "Default Variant Test A $handle";
+        $variantA->sku = "default-variant-test-a-$handle";
+        $variantA->basePrice = 10;
+        $variantA->sortOrder = 1;
+        $variantA->isDefault = true;
+
+        $variantB = new Variant();
+        $variantB->title = "Default Variant Test B $handle";
+        $variantB->sku = "default-variant-test-b-$handle";
+        $variantB->basePrice = 20;
+        $variantB->sortOrder = 2;
+        $variantB->isDefault = false;
+
+        $product->setVariants([$variantA, $variantB]);
+        self::assertTrue(Craft::$app->getElements()->saveElement($product, false));
+
+        $date = Db::prepareDateForDb(new DateTime('-1 hour'));
+        Db::update(CraftTable::ELEMENTS, ['dateCreated' => $date, 'dateUpdated' => $date], [
+            'id' => [$product->id, $variantA->id, $variantB->id],
+        ], updateTimestamp: false);
+
+        $product = Product::find()->id($product->id)->status(null)->one();
+        self::assertNotNull($product);
+
+        return [$product, $variantA, $variantB];
+    }
+
+    private function _defaultVariantId(int $productId): ?int
+    {
+        $defaultVariantId = (new Query())
+            ->select(['defaultVariantId'])
+            ->from([Table::PRODUCTS])
+            ->where(['id' => $productId])
+            ->scalar();
+
+        return $defaultVariantId ? (int)$defaultVariantId : null;
+    }
+
+    /**
+     * @return int[]
+     */
+    private function _flaggedVariantIds(int $productId): array
+    {
+        return array_map('intval', (new Query())
+            ->select(['id'])
+            ->from([Table::VARIANTS])
+            ->where(['primaryOwnerId' => $productId, 'isDefault' => true])
+            ->orderBy(['id' => SORT_ASC])
+            ->column());
+    }
+}

tests/unit/elements/variant/VariantQueryTest.php

@@ -23,6 +23,7 @@
 use craft\elements\User;
 use craftcommercetests\fixtures\ProductFixture;
 use craftcommercetests\fixtures\ShippingCategoryFixture;
+use DateTime;
 use UnitTester;
 
 /**
@@ -563,4 +564,61 @@ public function testEditableIgnoresLegacyEditProductTypePermission(): void
             Craft::$app->getUser()->setIdentity($originalIdentity);
         }
     }
+
+    /**
+     * @return void
+     */
+    public function testIsDefaultDerivesFromDefaultVariantId(): void
+    {
+        $product = new Product();
+        $product->title = 'IsDefault Query Test Product';
+        $product->typeId = 2000;
+        $product->slug = 'is-default-query-test-product';
+        $product->enabled = true;
+        $product->enabledForSite = true;
+        $product->postDate = new DateTime('now');
+
+        $variantA = new Variant();
+        $variantA->title = 'IsDefault Query Variant A';
+        $variantA->slug = 'is-default-query-variant-a';
+        $variantA->sku = 'is-default-query-a';
+        $variantA->basePrice = 10;
+        $variantA->sortOrder = 0;
+        $variantA->isDefault = true;
+
+        $variantB = new Variant();
+        $variantB->title = 'IsDefault Query Variant B';
+        $variantB->slug = 'is-default-query-variant-b';
+        $variantB->sku = 'is-default-query-b';
+        $variantB->basePrice = 20;
+        $variantB->sortOrder = 1;
+        $variantB->isDefault = false;
+
+        $product->setVariants([$variantA, $variantB]);
+        Craft::$app->getElements()->saveElement($product, false);
+
+        try {
+            self::assertSame($variantA->id, Variant::find()->productId($product->id)->isDefault(true)->one()?->id);
+            self::assertSame($variantB->id, Variant::find()->productId($product->id)->isDefault(false)->one()?->id);
+
+            Craft::$app->getDb()->createCommand()->update(
+                Table::VARIANTS,
+                ['isDefault' => true],
+                ['id' => $variantB->id]
+            )->execute();
+            Craft::$app->getDb()->createCommand()->update(
+                Table::VARIANTS,
+                ['isDefault' => false],
+                ['id' => $variantA->id]
+            )->execute();
+
+            self::assertSame($variantA->id, Variant::find()->productId($product->id)->isDefault(true)->one()?->id);
+            self::assertSame($variantB->id, Variant::find()->productId($product->id)->isDefault(false)->one()?->id);
+
+            self::assertTrue(Variant::find()->id($variantA->id)->one()?->isDefault);
+            self::assertFalse(Variant::find()->id($variantB->id)->one()?->isDefault);
+        } finally {
+            Craft::$app->getElements()->deleteElementById($product->id, Product::class, null, true);
+        }
+    }
 }