Checkout bug #351
Checkout bug #351
Conversation
| 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); |
There was a problem hiding this comment.
Do we need to fetch customer from the database? Won't checking just the customer's mail be enough?
There was a problem hiding this comment.
We need the customer object later on anyways to add it to the order.
| $order->setCustomer($customer)->shouldNotBeCalled(); | ||
| $stateMachine->apply('complete')->shouldNotBeCalled(); | ||
|
|
||
| $this->shouldThrow(WrongUserException::class) |
There was a problem hiding this comment.
👍 but I'm not sure about naming. WDYT about CheckingOutOnBehalfOfAnotherUserException?
There was a problem hiding this comment.
A little too descriptive but sure, I can change that. I would have a catchier name in mind something like IdentityTheftException
There was a problem hiding this comment.
This makes it possible to use it also in other places.
| return $user->getCustomer()->getEmail(); | ||
| try { | ||
| return $this->loggedInUserProvider->provide()->getEmail(); | ||
| } catch (TokenNotFoundException $tokenNotFoundException) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would be awesome. I don't like base business logic on exceptions
There was a problem hiding this comment.
I will add that to the interface and then to the UPDATE.md file.
src/Handler/CompleteOrderHandler.php
Outdated
| } | ||
|
|
||
| // If the customer does exist the user has to be logged in with this customer. Otherwise the user is not authorized to complete the checkout | ||
| $loggedInUser = $this->loggedInUserProvider->provide(); |
There was a problem hiding this comment.
That's not exactly true. Customer may exist without ShopUser on only during first guest checkout. We need to cover:
- User is logged in - valid
- User is not logged in and customer exists, but doesn't have an account - valid
- User is not logged in and customer exists, but has an account - exception, he needs to log in
- User is logged in but mail is provided - exception
There was a problem hiding this comment.
So the email is just thought for guest checkout and not needed when the user is already logged in?
There was a problem hiding this comment.
That would be the perfect solution
There was a problem hiding this comment.
Then I guess we should introduce another route for checkout. Otherwise this might lead to confusion with the documentation.
|
@lchrusciel Can you have a look at it again? |
|
Closing in favour of #365 |
This PR was merged into the 1.0-dev branch. Discussion ---------- This PR contains the improved implementation of #351. Current implementation will support following scenarios: 1. User is logged in - valid 2. User is not logged in and customer exists, but doesn't have an account - valid 3. User is logged in and customer exists and they are related - valid 4. User is not logged in and customer exists, but has an account - exception, he needs to log in 5. User is logged in but different mail is provided - exception Commits ------- ae0575e Fixed the bug and added tests to prevent it d4c81f6 Fixed phpspec 836982c Removed redundant comparision 069e714 Refactored to use the user provider 2d6deaa Renaming the exception 98f9981 Fixed tests ef6dc51 Fixed routes in PHPUnit Test 2fcec7d Changed the code to match the requirements d1e99a2 Fixed phpspec tests ae7433f Fixed tests for logged in customers 52e4e05 Adding check if a token exists 0408b78 Codestyle 2e64316 Improve logged in shop user provider a8e8762 Provide logged in safe customer provider implementation
Customer needs to be logged in in order to use the checkout.