Skip to content
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.

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

Checkout bug #351

Closed
wants to merge 13 commits into from
2 changes: 2 additions & 0 deletions doc/swagger.yml
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,8 @@ paths:
description: "Invalid input, validation failed."
schema:
$ref: "#/definitions/GeneralError"
403:
description: "Not logged in or wrong email"
/taxon-products-by-slug/{slug}:
get:
tags:
Expand Down
96 changes: 85 additions & 11 deletions spec/Handler/CompleteOrderHandlerSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -9,28 +9,66 @@
use SM\StateMachine\StateMachineInterface;
use Sylius\Component\Core\Model\CustomerInterface;
use Sylius\Component\Core\Model\OrderInterface;
use Sylius\Component\Core\Model\ShopUserInterface;
use Sylius\Component\Core\OrderCheckoutTransitions;
use Sylius\Component\Core\Repository\CustomerRepositoryInterface;
use Sylius\Component\Core\Repository\OrderRepositoryInterface;
use Sylius\Component\Resource\Factory\FactoryInterface;
use Sylius\ShopApiPlugin\Command\CompleteOrder;
use Sylius\ShopApiPlugin\Provider\CustomerProviderInterface;
use Sylius\ShopApiPlugin\Exception\WrongUserException;
use Sylius\ShopApiPlugin\Provider\LoggedInUserProviderInterface;

final class CompleteOrderHandlerSpec extends ObjectBehavior
{
function let(OrderRepositoryInterface $orderRepository, CustomerProviderInterface $customerProvider, StateMachineFactoryInterface $stateMachineFactory): void
{
$this->beConstructedWith($orderRepository, $customerProvider, $stateMachineFactory);
function let(
OrderRepositoryInterface $orderRepository,
CustomerRepositoryInterface $customerRepository,
FactoryInterface $customerFactory,
LoggedInUserProviderInterface $loggedInUserProvider,
StateMachineFactoryInterface $stateMachineFactory
): void {
$this->beConstructedWith($orderRepository, $customerRepository, $customerFactory, $loggedInUserProvider, $stateMachineFactory);
}

function it_handles_order_completion_for_guest_checkout(
CustomerInterface $customer,
CustomerRepositoryInterface $customerRepository,
FactoryInterface $customerFactory,
OrderInterface $order,
OrderRepositoryInterface $orderRepository,
StateMachineFactoryInterface $stateMachineFactory,
StateMachineInterface $stateMachine
): void {
$orderRepository->findOneBy(['tokenValue' => 'ORDERTOKEN'])->willReturn($order);

$customerRepository->findOneBy(['email' => 'example@customer.com'])->willReturn(null);
$customerFactory->createNew()->willReturn($customer);

$stateMachineFactory->get($order, OrderCheckoutTransitions::GRAPH)->willReturn($stateMachine);
$stateMachine->can('complete')->willReturn(true);

$order->setNotes(null)->shouldBeCalled();
$order->setCustomer($customer)->shouldBeCalled();
$stateMachine->apply('complete')->shouldBeCalled();

$this->handle(new CompleteOrder('ORDERTOKEN', 'example@customer.com'));
}

function it_handles_order_completion_for_existing_customer(
CustomerInterface $customer,
CustomerProviderInterface $customerProvider,
CustomerRepositoryInterface $customerRepository,
LoggedInUserProviderInterface $loggedInUserProvider,
ShopUserInterface $shopUser,
OrderInterface $order,
OrderRepositoryInterface $orderRepository,
StateMachineFactoryInterface $stateMachineFactory,
StateMachineInterface $stateMachine
): void {
$orderRepository->findOneBy(['tokenValue' => 'ORDERTOKEN'])->willReturn($order);
$customerProvider->provide('example@customer.com')->willReturn($customer);

$customerRepository->findOneBy(['email' => 'example@customer.com'])->willReturn($customer);
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to fetch customer from the database? Won't checking just the customer's mail be enough?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need the customer object later on anyways to add it to the order.

$shopUser->getCustomer()->willReturn($customer);
$loggedInUserProvider->provide()->willReturn($shopUser);

$stateMachineFactory->get($order, OrderCheckoutTransitions::GRAPH)->willReturn($stateMachine);
$stateMachine->can('complete')->willReturn(true);
Expand All @@ -44,14 +82,19 @@ function it_handles_order_completion_for_existing_customer(

function it_handles_order_completion_with_notes(
CustomerInterface $customer,
CustomerProviderInterface $customerProvider,
CustomerRepositoryInterface $customerRepository,
LoggedInUserProviderInterface $loggedInUserProvider,
ShopUserInterface $shopUser,
OrderInterface $order,
OrderRepositoryInterface $orderRepository,
StateMachineFactoryInterface $stateMachineFactory,
StateMachineInterface $stateMachine
): void {
$orderRepository->findOneBy(['tokenValue' => 'ORDERTOKEN'])->willReturn($order);
$customerProvider->provide('example@customer.com')->willReturn($customer);

$customerRepository->findOneBy(['email' => 'example@customer.com'])->willReturn($customer);
$shopUser->getCustomer()->willReturn($customer);
$loggedInUserProvider->provide()->willReturn($shopUser);

$stateMachineFactory->get($order, OrderCheckoutTransitions::GRAPH)->willReturn($stateMachine);
$stateMachine->can('complete')->willReturn(true);
Expand All @@ -63,11 +106,42 @@ function it_handles_order_completion_with_notes(
$this->handle(new CompleteOrder('ORDERTOKEN', 'example@customer.com', 'Some notes'));
}

function it_throws_an_exception_if_order_does_not_exist(OrderRepositoryInterface $orderRepository): void
{
function it_throws_an_exception_if_order_does_not_exist(
OrderRepositoryInterface $orderRepository
): void {
$orderRepository->findOneBy(['tokenValue' => 'ORDERTOKEN'])->willReturn(null);

$this->shouldThrow(\InvalidArgumentException::class)->during('handle', [new CompleteOrder('ORDERTOKEN', 'example@customer.com')]);
$this->shouldThrow(\InvalidArgumentException::class)
->during('handle', [new CompleteOrder('ORDERTOKEN', 'example@customer.com')])
;
}

function it_throws_an_exception_if_the_user_is_not_logged_in(
CustomerInterface $customer,
CustomerRepositoryInterface $customerRepository,
LoggedInUserProviderInterface $loggedInUserProvider,
CustomerInterface $loggedInCustomer,
ShopUserInterface $shopUser,
OrderInterface $order,
OrderRepositoryInterface $orderRepository,
StateMachineFactoryInterface $stateMachineFactory,
StateMachineInterface $stateMachine
): void {
$orderRepository->findOneBy(['tokenValue' => 'ORDERTOKEN'])->willReturn($order);

$customerRepository->findOneBy(['email' => 'example@customer.com'])->willReturn($customer);
$shopUser->getCustomer()->willReturn($loggedInCustomer);
$loggedInUserProvider->provide()->willReturn($shopUser);

$stateMachineFactory->get($order, OrderCheckoutTransitions::GRAPH)->willReturn($stateMachine);
$stateMachine->can('complete')->willReturn(true);

$order->setCustomer($customer)->shouldNotBeCalled();
$stateMachine->apply('complete')->shouldNotBeCalled();

$this->shouldThrow(WrongUserException::class)
Copy link
Member

Choose a reason for hiding this comment

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

👍 but I'm not sure about naming. WDYT about CheckingOutOnBehalfOfAnotherUserException?

Copy link
Member Author

Choose a reason for hiding this comment

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

A little too descriptive but sure, I can change that. I would have a catchier name in mind something like IdentityTheftException

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes it possible to use it also in other places.

->during('handle', [new CompleteOrder('ORDERTOKEN', 'example@customer.com', 'Some notes')])
;
}

function it_throws_an_exception_if_order_cannot_be_addressed(
Expand Down
53 changes: 35 additions & 18 deletions src/Controller/Checkout/CompleteOrderAction.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@
use FOS\RestBundle\View\View;
use FOS\RestBundle\View\ViewHandlerInterface;
use League\Tactician\CommandBus;
use Sylius\Component\Core\Model\ShopUserInterface;
use Sylius\ShopApiPlugin\Command\CompleteOrder;
use Sylius\ShopApiPlugin\Exception\WrongUserException;
use Sylius\ShopApiPlugin\Provider\LoggedInUserProviderInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
use Symfony\Component\Security\Core\Exception\TokenNotFoundException;

final class CompleteOrderAction
{
Expand All @@ -21,37 +22,53 @@ final class CompleteOrderAction
/** @var CommandBus */
private $bus;

/** @var TokenStorageInterface */
private $tokenStorage;
/** @var LoggedInUserProviderInterface */
private $loggedInUserProvider;

public function __construct(ViewHandlerInterface $viewHandler, CommandBus $bus, TokenStorageInterface $tokenStorage)
{
public function __construct(
ViewHandlerInterface $viewHandler,
CommandBus $bus,
LoggedInUserProviderInterface $loggedInUserProvider
) {
$this->viewHandler = $viewHandler;
$this->bus = $bus;
$this->tokenStorage = $tokenStorage;
$this->loggedInUserProvider = $loggedInUserProvider;
}

public function __invoke(Request $request): Response
{
$email = $this->provideUserEmail($request);

$this->bus->handle(new CompleteOrder(
$request->attributes->get('token'),
$email,
$request->request->get('notes')
));
try {
$this->bus->handle(
new CompleteOrder(
$request->attributes->get('token'),
$email,
$request->request->get('notes')
)
);
} catch (WrongUserException $notLoggedInException) {
return $this->viewHandler->handle(
View::create(
'You need to be logged in with the same user that wants to complete the order',
Response::HTTP_UNAUTHORIZED
)
);
} catch (TokenNotFoundException $notLoggedInException) {
return $this->viewHandler->handle(
View::create('You need to be logged in', Response::HTTP_UNAUTHORIZED)
);
}

return $this->viewHandler->handle(View::create(null, Response::HTTP_NO_CONTENT));
}

private function provideUserEmail(Request $request): string
{
$user = $this->tokenStorage->getToken()->getUser();

if ($user instanceof ShopUserInterface) {
return $user->getCustomer()->getEmail();
try {
return $this->loggedInUserProvider->provide()->getEmail();
} catch (TokenNotFoundException $tokenNotFoundException) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to base our login on exception? I would prefer to use if statement and add a new method in loggedInUserProvider/ allow a null return in provide

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that is how the user provider is implemented. I am sure we could also add another method to the LoggedInUserProvider that checks if a user is logged in or not without throwing an exception.

Copy link
Member

Choose a reason for hiding this comment

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

Would be awesome. I don't like base business logic on exceptions

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add that to the interface and then to the UPDATE.md file.

return $request->request->get('email');
}

return $request->request->get('email');
}
}
11 changes: 11 additions & 0 deletions src/Exception/WrongUserException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

namespace Sylius\ShopApiPlugin\Exception;

use Exception;

class WrongUserException extends Exception
{
}
58 changes: 51 additions & 7 deletions src/Handler/CompleteOrderHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,47 @@
namespace Sylius\ShopApiPlugin\Handler;

use SM\Factory\FactoryInterface as StateMachineFactory;
use Sylius\Component\Core\Model\CustomerInterface;
use Sylius\Component\Core\Model\OrderInterface;
use Sylius\Component\Core\OrderCheckoutTransitions;
use Sylius\Component\Core\Repository\CustomerRepositoryInterface;
use Sylius\Component\Core\Repository\OrderRepositoryInterface;
use Sylius\Component\Resource\Factory\FactoryInterface;
use Sylius\ShopApiPlugin\Command\CompleteOrder;
use Sylius\ShopApiPlugin\Provider\CustomerProviderInterface;
use Sylius\ShopApiPlugin\Exception\WrongUserException;
use Sylius\ShopApiPlugin\Provider\LoggedInUserProviderInterface;
use Symfony\Component\Security\Core\Exception\TokenNotFoundException;
use Webmozart\Assert\Assert;

final class CompleteOrderHandler
{
/** @var OrderRepositoryInterface */
private $orderRepository;

/** @var CustomerProviderInterface */
private $customerProvider;
/** @var CustomerRepositoryInterface */
private $customerRepository;

/** @var StateMachineFactory */
private $stateMachineFactory;

/** @var FactoryInterface */
private $customerFactory;

/** @var LoggedInUserProviderInterface */
private $loggedInUserProvider;

public function __construct(
OrderRepositoryInterface $orderRepository,
CustomerProviderInterface $customerProvider,
CustomerRepositoryInterface $customerRepository,
FactoryInterface $customerFactory,
LoggedInUserProviderInterface $loggedInUserProvider,
StateMachineFactory $stateMachineFactory
) {
$this->orderRepository = $orderRepository;
$this->customerProvider = $customerProvider;
$this->customerRepository = $customerRepository;
$this->stateMachineFactory = $stateMachineFactory;
$this->customerFactory = $customerFactory;
$this->loggedInUserProvider = $loggedInUserProvider;
}

public function handle(CompleteOrder $completeOrder)
Expand All @@ -44,11 +59,40 @@ public function handle(CompleteOrder $completeOrder)

Assert::true($stateMachine->can(OrderCheckoutTransitions::TRANSITION_COMPLETE), sprintf('Order with %s token cannot be completed.', $completeOrder->orderToken()));

$customer = $this->customerProvider->provide($completeOrder->email());

$customer = $this->getCustomer($completeOrder->email());
$order->setNotes($completeOrder->notes());
$order->setCustomer($customer);

$stateMachine->apply(OrderCheckoutTransitions::TRANSITION_COMPLETE);
}

private function getCustomer(string $emailAddress): CustomerInterface
{
try {
$loggedInUser = $this->loggedInUserProvider->provide();

if ($emailAddress !== '') {
throw new \InvalidArgumentException('Can not have a logged in user and an email address');
}

/** @var CustomerInterface $customer */
$customer = $loggedInUser->getCustomer();

return $customer;
} catch (TokenNotFoundException $notLoggedIn) {
/** @var CustomerInterface|null $customer */
$customer = $this->customerRepository->findOneBy(['email' => $emailAddress]);

// If the customer does not exist then it's normal checkout
if ($customer === null) {
/** @var CustomerInterface $customer */
$customer = $this->customerFactory->createNew();
$customer->setEmail($emailAddress);

return $customer;
}

throw new WrongUserException('Email is already taken');
}
}
}
2 changes: 1 addition & 1 deletion src/Resources/config/services/actions/checkout.xml
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
>
<argument type="service" id="fos_rest.view_handler" />
<argument type="service" id="tactician.commandbus" />
<argument type="service" id="security.token_storage" />
<argument type="service" id="sylius.shop_api_plugin.provider.current_user_provider" />
</service>
</services>
</container>
4 changes: 3 additions & 1 deletion src/Resources/config/services/handler/cart.xml
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@
<service id="sylius.shop_api_plugin.handler.complete_order_handler"
class="Sylius\ShopApiPlugin\Handler\CompleteOrderHandler">
<argument type="service" id="sylius.repository.order"/>
<argument type="service" id="sylius.shop_api_plugin.provider.customer_provider"/>
<argument type="service" id="sylius.repository.customer" />
<argument type="service" id="sylius.factory.customer" />
<argument type="service" id="sylius.shop_api_plugin.provider.current_user_provider" />
<argument type="service" id="sm.factory"/>
<tag name="tactician.handler" command="Sylius\ShopApiPlugin\Command\CompleteOrder"/>
</service>
Expand Down
Loading