Product reviews API #8772

paulstoica · 2017-10-04T19:58:54Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	#8753 Product Reviews API
License	MIT

This pull request is strong related to the #8753 ticket that I open.

I added API endpoints for Product Reviews: get, post, put, patch, show and in addition I added also other two endpoints for accepting/rejecting a product review.

In this pull request are all the changes. Also in this pull request is the documentation for this Product Reviews API.

pjedrzejewski · 2017-10-05T06:55:19Z

src/Sylius/Bundle/AdminApiBundle/Controller/UpdateProductReviewStatusController.php

+            $productCode
+        );
+
+        $productReview->setStatus(ReviewInterface::STATUS_ACCEPTED);


We should never do setStatus but use State Machine instead. :) http://docs.sylius.org/en/latest/book/architecture/state_machine.html

Alright, I will make the change.

GSadee

Great job! Please apply these minor changes 😃

GSadee · 2017-10-06T06:31:04Z

docs/api/product_reviews.rst

+| author           | Customer author for product review (This is customer that added the                            |
+|                  | product review; this will contain customer resource information)                               |      
+------------------+------------------------------------------------------------------------------------------------+
+| status           | Status of product review(New, Accepted, Rejected)                                              |


Missing space before (

GSadee · 2017-10-06T06:31:56Z

docs/api/product_reviews.rst

+---------------+----------------+----------------------------------------------------------+
+| comment       | request        | Product review comment                                   |
+---------------+----------------+----------------------------------------------------------+
+| rating        | request        | Product review rating(1..5)                              |


Missing space before (

GSadee · 2017-10-06T06:32:37Z

docs/api/product_reviews.rst

+        }
+    }
+
+


Unnecessary blank line

GSadee · 2017-10-06T06:33:42Z

docs/api/product_reviews.rst

+        }
+    }
+
+


Unnecessary blank line

GSadee · 2017-10-06T06:35:10Z

docs/api/product_reviews.rst

+---------------+----------------+----------------------------------------------------------+
+| comment       | request        | Product review comment                                   |
+---------------+----------------+----------------------------------------------------------+
+| rating        | request        | Product review rating(1..5)                              |


Missing space before (

GSadee · 2017-10-06T06:53:01Z

tests/Controller/ProductReviewApiTest.php

+
+    /**
+     * @param ProductInterface $product
+     * @param ProductReview $productReview


ProductReviewInterface

Hi,
I tried to find the "ProductReviewInterface" but it does not exists, only "ReviewInterface".
And because this class refers to the Product Review Api, I thought will be a good fit to use "ProductReview" model. Or I should create a interface for ProductReview model?

My bad, there should be ReviewInterface instead of ProductReview 😃

GSadee · 2017-10-06T06:53:41Z

tests/DataFixtures/ORM/resources/product_reviews.yml

+        lastName: King
+        email: example.king@example.com
+        emailCanonical: example.king@example.com
+Sylius\Component\Core\Model\Product:


Missing blank line before this line

GSadee · 2017-10-06T06:53:53Z

tests/DataFixtures/ORM/resources/product_reviews.yml

+        comment: "mug_review_bad_comment"
+        author: "@customer_example1"
+        status: "accepted"
+        reviewSubject: "@product1"


Missing blank line at the end of file

GSadee · 2017-10-06T06:56:01Z

tests/Responses/Expected/product_review/accept_response.json

@@ -0,0 +1,92 @@
+{
+  "id": @integer@,


indentation should be with 4 spaces in this file and other jsons

GSadee · 2017-10-06T06:56:27Z

tests/Responses/Expected/product_review/accept_response.json

+  },
+  "createdAt": @string@,
+  "updatedAt": @string@
+}


Missing blank line at the end of file here and in other jsons

pamil · 2017-10-19T09:37:42Z

src/Sylius/Component/Core/Repository/ProductReviewRepositoryInterface.php

+     *
+     * @return QueryBuilder
+     */
+    public function createQueryBuilderByProductCode(string $locale, string $productCode): QueryBuilder;


Adding methods to an interface is a BC break.

@pamil, in this case how can proceed next? Should we put the methods only into the ProductReviewRepository?

According to our proposed BC promise (#8901), it isn't a BC break 🎉

Zales0123 · 2017-10-25T06:33:11Z

src/Sylius/Bundle/AdminApiBundle/Resources/config/routing/product_review.yml

+        _sylius:
+            state_machine:
+                graph: sylius_product_review
+                transition: reject


Missing blank line at the end of file.

Zales0123 · 2017-10-25T06:35:44Z

src/Sylius/Component/Core/Repository/ProductReviewRepositoryInterface.php

+
+    /**
+     * @param mixed $id
+     * @param string  $productCode


Doubled space after string

Zales0123 · 2017-10-25T06:36:40Z

tests/Controller/ProductReviewApiTest.php

+    /**
+     * @test
+     */
+    public function it_allows_create_product_review()


it_allows_creating_new_product_review

Hello @Zales0123,

For these methods names I checked also tests/Controller/ProductVariantApiTest.php witch have added the same method name's logic: "it_allows_create_product_variant"; it means that for tests/Controller/ProductVariantApiTest.php is also incorect? if not, what is the difference between ProductReviewApiTest.php and ProductVariantApiTest.php, why for one is correct and for the other one, not?

Thank you.
Paul.

For the name of the methods the most important requirement is language correctness :) So even if somewhere else method names are named incorrectly (from the grammatical point of view) we should on writing new ones as perfect as possible.

Alright, then I will rewrite, these methods name, with the good ones, for tests/Controller/ProductReviewApiTest.php.

Zales0123 · 2017-10-25T06:38:29Z

tests/Controller/ProductReviewApiTest.php

+        {
+          "title": "J_REVIEW",
+          "rating": "3",
+          "comment": "J_REVIEW_COMMENT",


Don't be afraid of using some more natural exemplary data, these are of course not wrong, but less natural to read. Tests are good place to place easter eggs in code (look at our Behat scenarios and I'll see what I mean 😄 )

Zales0123 · 2017-10-25T06:40:04Z

tests/Controller/ProductReviewApiTest.php

+    /**
+     * @test
+     */
+    public function it_allows_delete_product_review()


it_allows_deleting_... here and below.

Zales0123 · 2017-10-25T06:40:48Z

tests/Controller/ProductReviewApiTest.php

+    /**
+     * @test
+     */
+    public function it_does_not_allows_accept_product_review_if_it_has_not_new_status()


it_does_not_allows_accepting_... and so on.

GSadee · 2017-10-27T06:58:14Z

docs/api/product_reviews.rst

+| status           | Status of product review (New, Accepted, Rejected)                                             |
+------------------+------------------------------------------------------------------------------------------------+
+| reviewSubject    | This is the review subject for the product review. For this case of the product review, this   |
+|                  | will contains a product resource                                                               |


this contains... or this will contain...

GSadee · 2017-10-27T07:00:37Z

docs/api/product_reviews.rst

+        "status": "new",
+        "reviewSubject": {
+            "name": "MUG-TH",
+            "id": 1,


id should be in the first position here and in other examples

GSadee · 2017-10-27T07:03:12Z

docs/api/product_reviews.rst

+Example
+^^^^^^^
+
+To create new product review for the product with ``code = MUG-TH`` use the below method.


a new product

GSadee · 2017-10-27T07:07:24Z

src/Sylius/Bundle/AdminApiBundle/Resources/config/grids/product_review.yml

+# This file is part of the Sylius package.
+# (c) Paweł Jędrzejewski
+#
+# @author Paul Stoica <paul.stoica18@gmail.com>


We are not using author docblocks anymore, because they are just redundant (data can be fetched from git history), please remove it from here and other files

GSadee · 2017-10-27T07:12:19Z

src/Sylius/Component/Core/Repository/ProductReviewRepositoryInterface.php

+     *
+     * @return QueryBuilder
+     */
+    public function createQueryBuilderByProductCode(string $locale, string $productCode): QueryBuilder;


According to our proposed BC promise (#8901), it isn't a BC break 🎉

GSadee · 2017-10-27T07:24:38Z

@paulstoica, please rebase your branch and provide other fixes :)

lchrusciel · 2017-11-21T09:04:59Z

tests/Responses/Expected/product_review/reject_response.json

+            }
+        }
+    },
+    "createdAt": @string@,


@string@.isDateTime()

Also, we used to surround @string@ matches with "

lchrusciel

For me it is good to go :)

lchrusciel · 2017-12-08T11:14:03Z

tests/Controller/ProductReviewApiTest.php

+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * @author Paul Stoica <paul.stoica18@gmail.com>


We are not adding author doc blocks anymore. Relevant PR #8882

lchrusciel · 2017-12-08T11:14:09Z

tests/Controller/ProductReviewApiTest.php

+ */
+final class ProductReviewApiTest extends JsonApiTestCase
+{
+


lchrusciel · 2017-12-08T11:14:44Z

tests/Controller/ProductReviewApiTest.php

+        $product = $productReviewsData['product1'];
+
+        $data =
+            <<<EOT


Wrong indent

lchrusciel · 2017-12-08T11:15:29Z

tests/Controller/ProductReviewApiTest.php

+    /**
+     * @test
+     */
+    public function it_allows_indexing_product_reviews()


Missing denied access test

@lchrusciel there is a test method: "it_does_not_allows_showing_product_review_list_when_access_is_denied", or what do you mean?

I just missed it

Zales0123 · 2018-01-04T07:18:56Z

@paulstoica will you be able to make this few final fixes from @lchrusciel? It would be great to have it merged. Also there are some fails on travis which seems to have no connection with proposed changes, maybe rebase with current 1.1 branch would work.

lchrusciel · 2018-01-05T08:27:45Z

docs/api/index.rst

    product_variants
+    product_reviews


One would be enough

lchrusciel · 2018-01-05T08:27:55Z

docs/api/map.rst.inc

 * :doc:`/api/product_variants`
+* :doc:`/api/product_reviews`


lchrusciel · 2018-01-05T08:32:28Z

tests/Controller/ProductReviewApiTest.php

+
+final class ProductReviewApiTest extends JsonApiTestCase
+{
+


pamil

Looks good overall, I'll add an entry in UPGRADE.md for these added repository methods, just to have everything covered.

pamil · 2018-01-05T11:49:11Z

Thanks Paul for your effort (and patience)! 🎉

paulstoica · 2018-01-05T12:07:54Z

@pamil, you're welcome.

But, how @lchrusciel said, I need also to remove from the documentation one "product_reviews" text because I saw that is a duplication there(I think because of an old conflict that I had). Can I work in this pull request, or how we will proceed, if it was closed?

pamil · 2018-01-05T12:13:10Z

@paulstoica I've already made it for you in #9084. Anyway, the flow in these cases is to open an another PR (based on the updated remote branch the previous PR have been merged into).

Product reviews API

pjedrzejewski added the New Feature label Oct 5, 2017

pjedrzejewski added this to the 1.1 milestone Oct 5, 2017

pjedrzejewski reviewed Oct 5, 2017

View reviewed changes

paulstoica force-pushed the product-reviews-api branch from 99d8265 to 41e6c2d Compare October 5, 2017 21:10

GSadee added the API APIs related issues and PRs. label Oct 6, 2017

GSadee reviewed Oct 6, 2017

View reviewed changes

paulstoica force-pushed the product-reviews-api branch from 01cca12 to 72ed226 Compare October 9, 2017 19:37

paulstoicareea added 9 commits October 9, 2017 23:09

[Product Reviews API] - add API for product reviews

0f6966a

[Documentation][API] - add product reviews api docs

c660fa9

[Product Reviews API]: accept/reject endpoints with state machine

fb337a6

[Unit Testing]: ProductReviewApi Controller

0e2ccc9

[Documentation]: add in toctree and map Product Reviews API

0151d20

[Documentation]: minor changes

9785c0e

[Product Reviews API] - minor changes

4aa26f9

[Documentation]: fix malformed tables Product reviews API

de75e27

[Product Reviews API] - minor changes for tests

ca4a90a

paulstoica force-pushed the product-reviews-api branch from 72ed226 to ca4a90a Compare October 9, 2017 20:33

pamil suggested changes Oct 19, 2017

View reviewed changes

Zales0123 reviewed Oct 25, 2017

View reviewed changes

GSadee reviewed Oct 27, 2017

View reviewed changes

pamil added Feature New feature proposals. and removed New Feature labels Oct 27, 2017

paulstoicareea added 7 commits November 17, 2017 21:24

[Product Reviews API] - add API for product reviews

3c1048f

[Documentation][API] - add product reviews api docs

bc07098

[Product Reviews API]: accept/reject endpoints with state machine

5e41ae6

[Unit Testing]: ProductReviewApi Controller

e998b59

[Documentation]: add in toctree and map Product Reviews API

dd37af3

[Documentation]: minor changes

0228602

[Product Reviews API] - minor changes

d571b9c

paulstoicareea added 6 commits November 17, 2017 21:24

[Documentation]: fix malformed tables Product reviews API

3933ed2

[Product Reviews API] - minor changes for tests

3cf247a

[Documentation]: texts improvments Product reviews API

859021c

Product review API syntax improvments

ca795cb

Product Review API tests: review creation real inputs

78d2c96

[Product Reviews API] - add API for product reviews

aca53a4

lchrusciel reviewed Nov 21, 2017

View reviewed changes

fix product reviews api test methods names, expected response

27086bd

lchrusciel approved these changes Dec 8, 2017

View reviewed changes

pamil changed the base branch from master to 1.1 January 4, 2018 10:30

product reviews api test remove author, fix indentation

e386f37

lchrusciel approved these changes Jan 5, 2018

View reviewed changes

pamil approved these changes Jan 5, 2018

View reviewed changes

pamil merged commit 3f8cd17 into Sylius:1.1 Jan 5, 2018

pamil mentioned this pull request Jan 5, 2018

Minor enhancements to product review API pull request #9084

Merged

pamil mentioned this pull request Jan 5, 2018

Product Reviews API #8753

Closed

Zales0123 mentioned this pull request Feb 8, 2018

Product reviews API semantic fix #9163

Merged

pamil added a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#8772 from paulstoica/product-reviews-api

4c7877e

Product reviews API

pamil added a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#8772 from paulstoica/product-reviews-api

c76dd4c

Product reviews API

Product reviews API #8772

Product reviews API #8772

Conversation

paulstoica commented Oct 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GSadee commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lchrusciel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zales0123 commented Jan 4, 2018 • edited by pamil Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil left a comment

Choose a reason for hiding this comment

pamil commented Jan 5, 2018

paulstoica commented Jan 5, 2018 • edited Loading

pamil commented Jan 5, 2018

Zales0123 commented Jan 4, 2018 •

edited by pamil

Loading

paulstoica commented Jan 5, 2018 •

edited

Loading