More precise error messages #301
Conversation
|
Please rerun the build. |
| 400: | ||
| description: "Validation failed" | ||
| schema: | ||
| $ref: "#/definitions/GeneralError" | ||
| 401: | ||
| description: "No user is logged in" | ||
| 500: |
There was a problem hiding this comment.
As you may have noticed I have added authorization symbols to the documentation. And the address book is something that only logged in users can see. If you are not logged in and try to call a route where you need to be logged in I wanted to return a 401 Unauthorized.
There was a problem hiding this comment.
I suppose it is not the scope of this PR anyway?
There was a problem hiding this comment.
The change in the documentation? That is already implemented. But the only parts that are behind a login wall are the address book and some of the customer endpoints.
There was a problem hiding this comment.
Sorry, I was just confused, that we are handling error with 500 code. But this is totally not part of this PR. I'm just missing some part why, but this is rather general question about current codebase :)
There was a problem hiding this comment.
True, the problem with the API was that assertions throw exceptions which are not very helpful to a user.
| */ | ||
| private $tokenStorage; | ||
| private $currentUserProvider; |
There was a problem hiding this comment.
We should adjust the name of field as well
| * @param CommandBus $bus | ||
| * @param TokenStorageInterface $tokenStorage | ||
| * @param CurrentUserProviderInterface $currentUserProvider | ||
| * @param ViewHandlerInterface $viewHandler |
There was a problem hiding this comment.
No doc comment variable alignment?
There was a problem hiding this comment.
We have resigned from this practice, as it is problematic once you want to add a new argument to docblock. People tend to forget about alignment and even if not, then diffs are less readable. Can you revert these changes?
There was a problem hiding this comment.
Only manually, but I will.
| $this->client->request('GET', '/shop-api/address-book', [], [], ['ACCEPT' => 'application/json']); | ||
| $response = $this->client->getResponse(); | ||
|
|
||
| $this->assertResponse($response, 'address_book/show_address_book_unauthorized_response', Response::HTTP_UNAUTHORIZED); |
There was a problem hiding this comment.
Can't we just assert response code?
| */ | ||
| private $currentUserProvider; | ||
|
|
||
| /** | ||
| * AddressExistsValidator constructor. | ||
| * | ||
| * @param AddressRepositoryInterface $addressRepository | ||
| * @param CurrentUserProviderInterface $currentUserProvider | ||
| * @param AddressRepositoryInterface $addressRepository |
| @@ -18,19 +18,19 @@ | |||
| */ | |||
| private $addressRepository; | |||
| /** | |||
| * @var CurrentUserProviderInterface | |||
| * @var LoggedInUserProviderInterface | |||
| */ | |||
| private $currentUserProvider; | |||
There was a problem hiding this comment.
We should rename this parameter in the whole codebase
| @@ -9,12 +9,20 @@ | |||
| final class RemoveAddressRequest | |||
| { | |||
| /** | |||
| * @var mixed | |||
| * @var int|string | |||
| { | ||
| /** | ||
| * @var TokenStorage |
| @@ -77,9 +77,11 @@ public function __invoke(Request $request): Response | |||
| $updateCustomerCommand = $updateCustomerRequest->getCommand(); | |||
| $this->bus->handle($updateCustomerCommand); | |||
|
|
|||
| return $this->viewHandler->handle(View::create( | |||
| return $this->viewHandler->handle( | |||
| View::create( | |||
There was a problem hiding this comment.
Should be reverted or arguments of create method should be indented
| $this->addressBookViewFactory->create($updatedAddress, $user->getCustomer()), | ||
| Response::HTTP_OK | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Instead of else we should use early return approach. Using return $this->viewHandler->handle(...); with two different arguments should be easier to understand what is happening here
| return $this->viewHandler->handle(View::create(null, Response::HTTP_UNAUTHORIZED)); | ||
| } | ||
|
|
||
| if (($customer = $user->getCustomer()) !== null) { |
| TokenStorageInterface $tokenStorage, | ||
| TokenInterface $token, | ||
| ShopUserInterface $shopUser | ||
| ) |
|
Thanks @mamazu! 🐋 |
As I have already mentioned (#296) I would suggest to use more precise error codes in the address book api to indicate for example that the user is not logged in. I have also added a PHPSpec test for the
CurrentlyLoggedInUserProvider