From 9d6c376f1c06fdd029f28c22b586d09902d4fe23 Mon Sep 17 00:00:00 2001 From: Nathaniel Hammond Date: Thu, 22 Jun 2023 10:43:08 +0100 Subject: [PATCH] Deprecated `setEmail` and `$email` as an attribute on the `Order` element --- CHANGELOG.md | 2 +- src/controllers/CartController.php | 3 +- src/elements/Order.php | 46 +++---- src/elements/db/OrderQuery.php | 15 ++- src/services/Carts.php | 7 +- src/services/Customers.php | 36 +++--- .../unit/elements/order/OrderCustomerTest.php | 112 ++++++++++++++++++ .../order/OrderMarkAsCompleteTest.php | 5 +- tests/unit/services/CartsTest.php | 44 ++++++- tests/unit/services/OrdersTest.php | 7 +- 10 files changed, 211 insertions(+), 66 deletions(-) create mode 100644 tests/unit/elements/order/OrderCustomerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 04f13ad331..38445e4f85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## 4.3.0 - Unreleased -- PLACEHOLDER +- Deprecated `craft\commerce\elements\Order::setEmail()`. `Order::setCustomer()` should be used instead. ## Unreleased diff --git a/src/controllers/CartController.php b/src/controllers/CartController.php index 3754a074a7..6d4c32c0d5 100644 --- a/src/controllers/CartController.php +++ b/src/controllers/CartController.php @@ -221,7 +221,8 @@ public function actionUpdateCart(): ?Response $email = $this->request->getParam('email'); if ($email && ($this->_cart->getEmail() === null || $this->_cart->getEmail() != $email)) { try { - $this->_cart->setEmail($email); + $user = Craft::$app->getUsers()->ensureUserByEmail($email); + $this->_cart->setCustomer($user); } catch (\Exception $e) { $this->_cart->addError('email', $e->getMessage()); } diff --git a/src/elements/Order.php b/src/elements/Order.php index 0717f60694..ab99e4741f 100644 --- a/src/elements/Order.php +++ b/src/elements/Order.php @@ -1109,20 +1109,6 @@ class Order extends Element */ private ?string $_paymentCurrency = null; - /** - * @var string|null - * @see Order::setEmail() To set the order email - * @see Order::getEmail() To get the email - * --- - * ```php - * echo $order->email; - * ``` - * ```twig - * {{ order.email }} - * ``` - */ - private ?string $_email = null; - /** * @var Transaction[]|null * @see Order::getTransactions() @@ -1148,7 +1134,7 @@ class Order extends Element * {{ order.customer }} * ``` */ - private User|null|false $_customer; + private User|null|false $_customer = null; /** * @var float|null @@ -1327,7 +1313,6 @@ public function attributes(): array $names[] = 'customerId'; $names[] = 'paymentCurrency'; $names[] = 'paymentAmount'; - $names[] = 'email'; $names[] = 'isPaid'; $names[] = 'itemSubtotal'; $names[] = 'itemTotal'; @@ -1411,6 +1396,7 @@ public function fields(): array }; } + $fields['email'] = 'email'; $fields['paidStatusHtml'] = 'paidStatusHtml'; $fields['customerLinkHtml'] = 'customerLinkHtml'; $fields['orderStatusHtml'] = 'orderStatusHtml'; @@ -1482,7 +1468,6 @@ protected function defineRules(): array [['paymentSourceId'], 'number', 'integerOnly' => true], [['paymentSourceId'], 'validatePaymentSourceId'], - [['email'], 'email'], [['number', 'user', 'orderCompletedEmail'], 'safe'], ]); @@ -2281,10 +2266,6 @@ public function getCustomer(): ?User } } - if ($this->_customer) { - $this->_email = $this->_customer->email; - } - return $this->_customer ?: null; } @@ -2298,7 +2279,8 @@ public function setCustomer(?User $customer = null): void $this->_customer = $customer; if ($this->_customer) { $this->_customerId = $this->_customer->id; - $this->_email = $this->_customer->email; + } else { + $this->_customerId = null; } } @@ -2307,7 +2289,7 @@ public function setCustomer(?User $customer = null): void */ public function getUser(): ?User { - Craft::$app->getDeprecator()->log('Order::getUser()', 'The `Order::getUser()` is deprecated, use the `Order::getCustomer()` instead.'); + Craft::$app->getDeprecator()->log('Order::getUser()', 'The `Order::getUser()` is deprecated, use `Order::getCustomer()` instead.'); return $this->getCustomer(); } @@ -2316,37 +2298,37 @@ public function getUser(): ?User * * @param string|null $email * @throws Exception + * @deprecated in 4.3.0. Use [[setCustomer()]] instead. */ public function setEmail(?string $email): void { + Craft::$app->getDeprecator()->log(__METHOD__, '`Order::setEmail()` has been deprecated use `Order::setCustomer()` instead.'); if (!$email) { $this->_customer = null; $this->_customerId = null; - $this->_email = null; return; } - if ($this->_email === $email) { + if ($this->_customer && $this->_customer->email === $email) { return; } $user = Craft::$app->getUsers()->ensureUserByEmail($email); - $this->_email = $email; $this->setCustomer($user); } /** - * Returns the email for this order. Will always be the registered users email if the order's customer is related to a user. + * Returns the email for this order. Will always be the customer's email if they exist. + * @return string|null */ public function getEmail(): ?string { - if ($user = $this->getCustomer()) { - $this->_email = $user->email; - } - - return $this->_email ?? null; + return $this->getCustomer()?->email ?? null; } + /** + * @return bool + */ public function getIsPaid(): bool { return !$this->hasOutstandingBalance() && $this->isCompleted; diff --git a/src/elements/db/OrderQuery.php b/src/elements/db/OrderQuery.php index 17ee7689ec..81f5d5556c 100644 --- a/src/elements/db/OrderQuery.php +++ b/src/elements/db/OrderQuery.php @@ -1412,6 +1412,14 @@ public function withTransactions(bool $value = true): OrderQuery */ public function populate($rows): array { + // @TODO remove at next breaking change + // Remove `email` key from each row. + array_walk($rows, function(&$row) { + if (array_key_exists('email', $row)) { + unset($row['email']); + } + }); + /** @var Order[] $orders */ $orders = parent::populate($rows); @@ -1463,7 +1471,10 @@ protected function beforePrepare(): bool 'commerce_orders.couponCode', 'commerce_orders.orderStatusId', 'commerce_orders.dateOrdered', + + // @TODO remove at next breaking change 'commerce_orders.email', + 'commerce_orders.isCompleted', 'commerce_orders.datePaid', 'commerce_orders.currency', @@ -1536,7 +1547,9 @@ protected function beforePrepare(): bool } if (isset($this->email) && $this->email) { - $this->subQuery->andWhere(Db::parseParam('commerce_orders.email', $this->email, '=', true)); + // Join and search the users table for email address + $this->subQuery->leftJoin(CraftTable::USERS . ' users', '[[users.id]] = [[commerce_orders.customerId]]'); + $this->subQuery->andWhere(Db::parseParam('users.email', $this->email, '=', true)); } // Allow true ot false but not null diff --git a/src/services/Carts.php b/src/services/Carts.php index f055185c2c..6a5f763582 100644 --- a/src/services/Carts.php +++ b/src/services/Carts.php @@ -153,10 +153,9 @@ public function getCart(bool $forceSave = false): Order $this->_cart->paymentCurrency = $this->_getCartPaymentCurrencyIso(); $this->_cart->origin = Order::ORIGIN_WEB; - if ($currentUser) { - if ($this->_cart->getCustomer() === null || ($currentUser->email && $currentUser->email !== $this->_cart->email)) { - $this->_cart->setEmail($currentUser->email); // Will ensure the customer is also set - } + // Switch the cart customer if needed + if ($currentUser && ($this->_cart->getCustomer() === null || ($currentUser->email && $currentUser->email !== $this->_cart->getEmail()))) { + $this->_cart->setCustomer($currentUser); } $hasIpChanged = $originalIp != $this->_cart->lastIp; diff --git a/src/services/Customers.php b/src/services/Customers.php index 563f121c51..95e1197f60 100644 --- a/src/services/Customers.php +++ b/src/services/Customers.php @@ -312,33 +312,29 @@ public function transferCustomerData(User $fromCustomer, User $toCustomer): bool */ private function _activateUserFromOrder(Order $order): void { - if (!$order->email) { + $user = $order->getCustomer(); + if (!$user || $user->getIsCredentialed()) { return; } - $user = Craft::$app->getUsers()->ensureUserByEmail($order->email); - - if (!$user->getIsCredentialed()) { - if (!$user->fullName) { - $user->fullName = $order->getBillingAddress()?->fullName ?? $order->getShippingAddress()?->fullName ?? ''; - } + if (!$user->fullName) { + $user->fullName = $order->getBillingAddress()?->fullName ?? $order->getShippingAddress()?->fullName ?? ''; + } - $user->username = $order->email; - $user->pending = true; - $user->setScenario(Element::SCENARIO_ESSENTIALS); + $user->pending = true; + $user->setScenario(Element::SCENARIO_ESSENTIALS); - if (Craft::$app->getElements()->saveElement($user)) { - Craft::$app->getUsers()->assignUserToDefaultGroup($user); - $emailSent = Craft::$app->getUsers()->sendActivationEmail($user); + if (Craft::$app->getElements()->saveElement($user)) { + Craft::$app->getUsers()->assignUserToDefaultGroup($user); + $emailSent = Craft::$app->getUsers()->sendActivationEmail($user); - if (!$emailSent) { - Craft::warning('"registerUserOnOrderComplete" used to create the user, but couldn’t send an activation email. Check your email settings.', __METHOD__); - } - } else { - $errors = $user->getErrors(); - Craft::warning('Could not create user on order completion.', __METHOD__); - Craft::warning($errors, __METHOD__); + if (!$emailSent) { + Craft::warning('"registerUserOnOrderComplete" used to create the user, but couldn’t send an activation email. Check your email settings.', __METHOD__); } + } else { + $errors = $user->getErrors(); + Craft::warning('Could not create user on order completion.', __METHOD__); + Craft::warning($errors, __METHOD__); } } } diff --git a/tests/unit/elements/order/OrderCustomerTest.php b/tests/unit/elements/order/OrderCustomerTest.php new file mode 100644 index 0000000000..8404d5030f --- /dev/null +++ b/tests/unit/elements/order/OrderCustomerTest.php @@ -0,0 +1,112 @@ + + * @since 4.3.0 + */ +class OrderCustomerTest extends Unit +{ + /** + * @var UnitTester + */ + protected $tester; + + /** + * @return array + */ + public function _fixtures(): array + { + return [ + 'orders' => [ + 'class' => OrdersFixture::class, + ], + ]; + } + + /** + * @param string $email + * @return void + * @throws Exception + * @throws InvalidConfigException + * @dataProvider emailDataProvider + */ + public function testSetEmail(string $email): void + { + \Craft::$app->set('deprecator', $this->make(Deprecator::class, [ + 'log' => function(string $key, string $message, ?string $file = null, ?int $line = null) { + self::once(); + self::assertEquals(Order::class . '::setEmail', $key); + }, + ])); + + $order = new Order(); + $order->setEmail($email); + + self::assertEquals($email, $order->getEmail()); + self::assertNotNull($order->getCustomer()); + self::assertEquals($email, $order->getCustomer()->email); + } + + /** + * @return array[] + */ + public function emailDataProvider(): array + { + return [ + 'existing-credentialed-user' => ['email' => 'customer1@crafttest.com'], + 'existing-inactive-user' => ['email' => 'inactive.user@crafttest.com'], + ]; + } + + /** + * @param string $email + * @return void + * @dataProvider customerDataProvider + */ + public function testSetCustomer(string $email): void + { + $user = \Craft::$app->getUsers()->getUserByUsernameOrEmail($email); + $order = new Order(); + $order->setCustomer($user); + + self::assertEquals($email, $order->getEmail()); + self::assertNotNull($order->getCustomer()); + self::assertEquals($email, $order->getCustomer()->email); + self::assertEquals($user->id, $order->getCustomer()->id); + self::assertEquals($user->id, $order->getCustomerId()); + + // Test remove customer + $order->setCustomer(); + self::assertNull($order->getCustomer()); + self::assertNull($order->getCustomerId()); + self::assertNull($order->getEmail()); + } + + /** + * @return array[] + */ + public function customerDataProvider(): array + { + return [ + 'existing-credentialed-user' => ['email' => 'customer1@crafttest.com'], + 'existing-inactive-user' => ['email' => 'inactive.user@crafttest.com'], + ]; + } +} diff --git a/tests/unit/elements/order/OrderMarkAsCompleteTest.php b/tests/unit/elements/order/OrderMarkAsCompleteTest.php index 76c3b0f805..60a3e9a6da 100644 --- a/tests/unit/elements/order/OrderMarkAsCompleteTest.php +++ b/tests/unit/elements/order/OrderMarkAsCompleteTest.php @@ -60,7 +60,8 @@ public function testUpdatedProperties(): void { $order = new Order(); $email = 'test@newemailaddress.xyz'; - $order->setEmail($email); + $user = \Craft::$app->getUsers()->ensureUserByEmail($email); + $order->setCustomer($user); /** @var Order $order */ $completedOrder = $this->tester->grabFixture('orders')->getElement('completed-new'); $lineItem = $completedOrder->getLineItems()[0]; @@ -80,7 +81,7 @@ public function testUpdatedProperties(): void self::assertEquals($email, $order->orderCompletedEmail); $this->_deleteElementIds[] = $order->id; - $this->_deleteElementIds[] = $order->getCustomerId(); + $this->_deleteElementIds[] = $user->id; } /** diff --git a/tests/unit/services/CartsTest.php b/tests/unit/services/CartsTest.php index ceedb49fb9..aac96a20af 100644 --- a/tests/unit/services/CartsTest.php +++ b/tests/unit/services/CartsTest.php @@ -13,6 +13,7 @@ use craft\commerce\Plugin; use craft\commerce\services\Carts; use craftcommercetests\fixtures\CustomerAddressFixture; +use craftcommercetests\fixtures\CustomerFixture; use UnitTester; /** @@ -31,6 +32,9 @@ class CartsTest extends Unit public function _fixtures(): array { return [ + 'customer' => [ + 'class' => CustomerFixture::class, + ], 'customerAddresses' => [ 'class' => CustomerAddressFixture::class, ], @@ -60,17 +64,16 @@ public function testGetCartAutoSetAddresses(string $email, bool $autoSet, bool $ }, ])); + $user = Craft::$app->getUsers()->getUserByUsernameOrEmail($email); if ($loggedIn) { - $user = Craft::$app->getUsers()->getUserByUsernameOrEmail($email); Craft::$app->getUser()->setIdentity($user); Craft::$app->getUser()->getIdentity()->password = $user->password; } $newCart = new Order(); - $newCart->setEmail($email); + $newCart->setCustomer($user); $newCart->number = $cartNumber; Craft::$app->getElements()->saveElement($newCart, false); - // Plugin::getInstance()->getCarts()->__set('cart', $newCart); $cart = Plugin::getInstance()->getCarts()->getCart(); @@ -97,4 +100,39 @@ public function getCartDataProvider(): array 'logged-in-user-auto-set-addresses' => ['cred.user@crafttest.com', true, true, true, true], ]; } + + public function testGetCartSwitchCustomer(): void + { + $cartNumber = Plugin::getInstance()->getCarts()->generateCartNumber(); + Plugin::getInstance()->set('carts', $this->make(Carts::class, [ + 'getSessionCartNumber' => function() use ($cartNumber) { + return $cartNumber; + }, + ])); + + $inactiveUser = $this->tester->grabFixture('customer')->getElement('inactive-user'); + $credUser = $this->tester->grabFixture('customer')->getElement('credentialed-user'); + $originalIdentity = Craft::$app->getUser()->getIdentity(); + Craft::$app->getUser()->setIdentity($credUser); + Craft::$app->getUser()->getIdentity()->password = $credUser->password; + + + $order = new Order(); + $order->number = $cartNumber; + $order->setCustomer($inactiveUser); + + Craft::$app->getElements()->saveElement($order, false); + self::assertEquals($inactiveUser->id, $order->getCustomerId()); + + $cart = Plugin::getInstance()->getCarts()->getCart(); + + // assert customer has changed; + self::assertNotEquals($inactiveUser->id, $cart->getCustomerId()); + self::assertEquals($credUser->id, $cart->getCustomerId()); + self::assertEquals($credUser->email, $cart->getEmail()); + + // Reset data + Craft::$app->getUser()->setIdentity($originalIdentity); + Craft::$app->getElements()->deleteElement($cart, true); + } } diff --git a/tests/unit/services/OrdersTest.php b/tests/unit/services/OrdersTest.php index d7ce3c5fb1..5b3e9a08f8 100644 --- a/tests/unit/services/OrdersTest.php +++ b/tests/unit/services/OrdersTest.php @@ -98,12 +98,15 @@ public function testGetOrdersByCustomer(): void public function testGetOrdersByEmail(): void { - $orders = $this->service->getOrdersByEmail($this->fixtureData->getElement('completed-new')->email); + /** @var Order $orderFixture */ + $orderFixture = $this->fixtureData->getElement('completed-new'); + $email = $orderFixture->getEmail(); + $orders = $this->service->getOrdersByEmail($email); self::assertIsArray($orders); self::assertCount(3, $orders); foreach ($orders as $order) { - self::assertEquals($this->fixtureData->getElement('completed-new')->email, $order->email); + self::assertEquals($email, $order->getEmail()); } } }