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
[Api] Visitor and Customer add ProductReview #12472
Conversation
Tomanhez
commented
Mar 23, 2021
Q | A |
---|---|
Branch? | master |
Bug fix? | no |
New feature? | yes |
BC breaks? | no |
Deprecations? | no |
License | MIT |
e3c9fd5
to
422af47
Compare
cb09ff4
to
2e9a709
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And when you making a big refactor (not directly related with scop of task), I suggest making it in separate PR, because the majority of this code is a refactor not a feature :)
src/Sylius/Bundle/ApiBundle/CommandHandler/ChangeShopUserPasswordHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/PickupCartHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/AddProductReviewHandlerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/AddProductReviewHandlerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/services/command_handlers.xml
Outdated
Show resolved
Hide resolved
ee6b994
to
89ebd17
Compare
public function iWantToReviewProduct(ProductInterface $product): void | ||
{ | ||
$this->client->buildCreateRequest(); | ||
$this->client->addRequestData('productCode', $product->getCode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my previous review, I wrote that we need to keep a product code on command instead of iri, but we still should pass to the endpoint an iri as the resource identifier for API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solution for it: #12487
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/AddProductReviewHandlerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/CommandHandler/AddProductReviewHandlerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/Resources/config/api_resources/ProductReview.xml
Show resolved
Hide resolved
932b0a3
to
62b66d3
Compare
$this->productRepository = $productRepository; | ||
} | ||
|
||
public function __invoke(AddProductReview $addProductReview): ReviewInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about this return type. I just checked few handlers and all of them returns void. So is it really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Command AddProductReview
create new ReviewInterface
. Its useful to return ReviewInterface
as a Response
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is needed if we want to return 201 and created object. But not sure if this is case here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add void, as a response I get object of AddProductReview
and 201
code. Its needed? Do user should know structure of this object? This is the intention creating ReviewInterface
object and it should return this object as a response. IMO
And for example: If I create new 'Review' object, and if I need to get it, I need call to next response as a get on item just to know my new created object, Its needed?
src/Sylius/Bundle/ApiBundle/DataTransformer/AddProductReviewInputDataTransformer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/DataTransformer/AddProductReviewInputDataTransformer.php
Outdated
Show resolved
Hide resolved
)); | ||
} | ||
|
||
function it_throws_an_exception_if_email_has_not_been_found( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are missing a nulled mail path coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it not the same case? Is only one way. You must have email, If not its throw exception.
src/Sylius/Bundle/ApiBundle/spec/DataTransformer/AddProductReviewInputDataTransformerSpec.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/DataTransformer/AddProductReviewInputDataTransformer.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/spec/DataTransformer/AddProductReviewInputDataTransformerSpec.php
Outdated
Show resolved
Hide resolved
ad20b82
to
46891a2
Compare
46891a2
to
48979be
Compare
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
src/Sylius/Bundle/ApiBundle/CommandHandler/AddProductReviewHandler.php
Outdated
Show resolved
Hide resolved
...ndle/ApiBundle/spec/DataTransformer/LoggedInShopUserEmailAwareCommandDataTransformerSpec.php
Outdated
Show resolved
Hide resolved
48979be
to
d95297d
Compare
Thanks, Tomasz! 🥇 |