diff --git a/CHANGELOG.md b/CHANGELOG.md index a9be6244af..12f67f92a3 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/elements/Product.php b/src/elements/Product.php index acf2965d21..d62fed4e0c 100644 --- a/src/elements/Product.php +++ b/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 diff --git a/src/elements/Variant.php b/src/elements/Variant.php index d6583b6805..f7441bc8f4 100755 --- a/src/elements/Variant.php +++ b/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; } diff --git a/src/elements/actions/SetDefaultVariant.php b/src/elements/actions/SetDefaultVariant.php index 1aea16d4a7..ce8b847e71 100644 --- a/src/elements/actions/SetDefaultVariant.php +++ b/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( diff --git a/src/elements/db/VariantQuery.php b/src/elements/db/VariantQuery.php index 5cb6fe0d75..88dd350cd6 100755 --- a/src/elements/db/VariantQuery.php +++ b/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)) { diff --git a/tests/unit/elements/product/ProductDefaultVariantTest.php b/tests/unit/elements/product/ProductDefaultVariantTest.php new file mode 100644 index 0000000000..5e98e8f972 --- /dev/null +++ b/tests/unit/elements/product/ProductDefaultVariantTest.php @@ -0,0 +1,205 @@ + + */ +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()); + } +} diff --git a/tests/unit/elements/variant/VariantQueryTest.php b/tests/unit/elements/variant/VariantQueryTest.php index 35818f2ac8..e128cb3081 100644 --- a/tests/unit/elements/variant/VariantQueryTest.php +++ b/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); + } + } }