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

[API] Cart and Cart item CRUD #7297

Merged
merged 11 commits into from Feb 9, 2017
Merged

Conversation

lchrusciel
Copy link
Member

@lchrusciel lchrusciel commented Jan 20, 2017

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Related tickets
License MIT

TODO:

  • Update cart docs
  • Update cart item docs
  • Extract forms to separate service file

EDIT:
Based on #7315

@lchrusciel lchrusciel changed the title [API] Cart and Cart item CRUD [WIP] [API] Cart and Cart item CRUD Jan 20, 2017
@pjedrzejewski pjedrzejewski added the API APIs related issues and PRs. label Jan 20, 2017
use Webmozart\Assert\Assert;

/**
* @author Paweł Jędrzejewski <pawel@sylius.org>
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there should be your name

@lchrusciel lchrusciel force-pushed the cart-api branch 5 times, most recently from 9a485b6 to 32eea32 Compare January 24, 2017 08:26
factory:
method: createForCart
arguments:
- "expr:service('sylius.repository.order').findOneBy({id: $cartId, state: 'cart'})"
Copy link
Member

Choose a reason for hiding this comment

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

We should use the proper method here.

criteria:
order: $orderId
state: 'cart'
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -40,4 +35,5 @@ sylius_api_order_item_delete:
serialization_version: $version
criteria:
Copy link
Member

Choose a reason for hiding this comment

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

And here.

@@ -56,6 +56,22 @@ public function findLatest($count)
/**
* {@inheritdoc}
*/
public function createCartsPaginator(array $criteria = [], array $sorting = [])
Copy link
Member

Choose a reason for hiding this comment

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

Should be replaced with createCartListQueryBuilder method and used with grids - it will take care of filtering and sorting.

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be slightly harder, as we don't have a cart resource... So it would create order routing

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand what do you mean - By "grids" I understand grid configuration, not routing generation.

@lchrusciel lchrusciel force-pushed the cart-api branch 4 times, most recently from 249da04 to beb8c75 Compare January 25, 2017 12:41
],
"adjustments_total":0,
"total":0,
"state":"cart",
Copy link
Contributor

Choose a reason for hiding this comment

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

We display only carts, so this field is redundant.

template: "@SyliusUi/Grid/Field/state.html.twig"
vars:
labels: "@SyliusAdmin/Order/Label/PaymentState"
shippingState:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shipping and payment states are irrelevant for cart.

resource: |
alias: sylius.order
section: api
only: [show, delete]
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO we should not allow for deleting orders, you can only cancel already created ones.


<service id="sylius.listener.api.add_to_cart" class="Sylius\Bundle\ApiBundle\EventListener\AddToCartListener">
<argument type="service" id="sylius.order_processing.order_processor.composite" />
<tag name="kernel.event_listener" event="sylius.order_item.post_create" method="cartItemResolver" />
Copy link
Contributor

Choose a reason for hiding this comment

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

event pre_create
method recalculateOrder?

Shouldn't we recalculate order on update also?

@@ -96,7 +97,7 @@ private function addResourcesSection(ArrayNodeDefinition $node)
->scalarNode('model')->defaultValue(OrderItem::class)->cannotBeEmpty()->end()
->scalarNode('interface')->defaultValue(OrderItemInterface::class)->cannotBeEmpty()->end()
->scalarNode('controller')->defaultValue(OrderItemController::class)->cannotBeEmpty()->end()
->scalarNode('repository')->cannotBeEmpty()->end()
->scalarNode('repository')->defaultValue(OrderItemRepository::class)->cannotBeEmpty()->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not be configured here.

"adjustments_total": 0,
"variant": {
"id": @integer@,
"on_hold": 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

on_hold alone is not useful, we should add also on_hand field or skip both.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a part of variant serialization 👍 Will be fixed in #7348

"created_at": "@string@.isDateTime()",
"updated_at": "@string@.isDateTime()",
"enabled": true,
"tax_calculation_strategy": "order_items_based",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need all the above information about the channel for embedded responses?

/**
* @test
*/
public function it_allows_to_get_order()
Copy link
Contributor

Choose a reason for hiding this comment

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

get_cart

Copy link
Contributor

Choose a reason for hiding this comment

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

And everywhere else

/**
* @test
*/
public function it_does_not_show_orders_in_stare_other_than_cart()
Copy link
Contributor

Choose a reason for hiding this comment

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

stare 😆

/**
* @test
*/
public function it_does_not_allow_to_add_item_to_cart_without_providing_all_needed_fields()
Copy link
Contributor

Choose a reason for hiding this comment

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

all_needed -> required?

+-------------------+----------------------------------------------------------------------+
| adjustments | List of adjustments related to cart |
+-------------------+----------------------------------------------------------------------+
| adjustments_total | Sum of all items adjustments |
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be Sum of all order adjustments? Adjustments can be applied on Order as well as on OrderItem.

+-------------------+----------------------------------------------------------------------+
| Field | Description |
+===================+======================================================================+
| id | Id of shipping category |
Copy link
Member

Choose a reason for hiding this comment

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

shipping category? 😱

.. _Default channel serialization: http://docs.sylius.org/en/latest/api/channels.html#channels-api



Copy link
Member

Choose a reason for hiding this comment

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

Some extra blank lines.

$orderItem = $event->getData();

if (null === $orderItem) {
throw new UnexpectedTypeException($orderItem, OrderItemInterface::class);
Copy link
Member

Choose a reason for hiding this comment

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

Is it really unexpected type? 😄 In fact we're not getting any type here, it's just nothing (null) we get. UnexpectedTypeException should be thrown if type assertion is made, not not null assertion. I think we should use there just an Assert::notNull($orderItem).

*/
public function getBlockPrefix()
{
return 'sylius_api_cart_item';
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be somehow connected with class name? So OrderItemType -> sylius_api_order_item, or CartItemType -> sylius_api_cart_item? Just thinking out loud 😄

version: 1
exclusion:
groups: [Default, Detailed, DetailedCart]

Copy link
Member

Choose a reason for hiding this comment

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

And here.

@@ -0,0 +1,32 @@
<?php

namespace Sylius\Bundle\OrderBundle\Doctrine\ORM;
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need a license block?

->getOneOrNullResult()
;
}

Copy link
Member

Choose a reason for hiding this comment

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

And again not needed blank line.

@@ -0,0 +1,19 @@
<?php
namespace Sylius\Component\Order\Repository;
Copy link
Member

Choose a reason for hiding this comment

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

:(

@@ -0,0 +1,87 @@
Sylius\Component\Core\Model\Order:
order_001:
Copy link
Member

Choose a reason for hiding this comment

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

Why 001? Will we have hundreds of orders in this fixture? 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

It just the way how we define order fixtures with alice. I just wanted to be consistent. It doesn't change anything. 🐋

@lchrusciel lchrusciel force-pushed the cart-api branch 2 times, most recently from cea9f90 to 12dd4fe Compare January 27, 2017 10:36
@lchrusciel lchrusciel changed the title [WIP] [API] Cart and Cart item CRUD [API] Cart and Cart item CRUD Jan 27, 2017
@lchrusciel lchrusciel force-pushed the cart-api branch 5 times, most recently from 0816ef1 to e617762 Compare January 30, 2017 12:13
Copy link
Member

@CoderMaggie CoderMaggie left a comment

Choose a reason for hiding this comment

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

1681 lines of docs where 90% are tables and api responses is truly tough to review.

Index of all orders
-------------------
If you request an order, you will receive an object with following fields:

Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary blank line.

@@ -3,165 +3,89 @@ Orders API

Sylius orders API endpoint is `/api/v1/orders`.

Index of all orders
-------------------
If you request an order, you will receive an object with following fields:
Copy link
Member

Choose a reason for hiding this comment

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

the following fields

+-----------------------+---------------------------------------------+
| Field | Description |
+=======================+=============================================+
| id | Id of cart |
Copy link
Member

Choose a reason for hiding this comment

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

of cart? not an order? Seems you are describing an order here :D

Copy link
Member

Choose a reason for hiding this comment

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

and a cart btw.

+-----------------------+---------------------------------------------+
| checkout_completed_at | Date when the checkout has been completed |
+-----------------------+---------------------------------------------+
| number | Number of an order |
Copy link
Member

Choose a reason for hiding this comment

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

Number? serial number of the order maybe?

+-----------------------+---------------------------------------------+
| shipping_address | Detailed address serialization |
+-----------------------+---------------------------------------------+
| billing_address | Detailed address serialization |
Copy link
Member

Choose a reason for hiding this comment

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

This one and the above one have identical descriptions, but shouldn't.

Detailed billing address serialization

Copy link
Member Author

Choose a reason for hiding this comment

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

They should, because it is the same enitity


.. note::

If it is still confusing to you, you should read more about `Carts (Orders)`_ and `Adjustments`_ first.
Copy link
Member

Choose a reason for hiding this comment

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

learn more?


.. code-block:: text

POST /api/v1/carts/
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we give a full command? with $ curl part? (in .. code-block:: bash)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I see. The example is below. So here do just a description.

To create a new cart you will need to call the ``/api/v1/carts/`` endpoint with ``POST`` method.


Example
.......

Copy link
Member

Choose a reason for hiding this comment

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

A description of what it does is needed.

To create a new cart for the ``shop@example.com`` user in the ``US_WEB`` channel in the ``en_US`` locale use the below method. 

This method replicates the environment of shop usage, when a user is logged in some channel with some locale chosen.

I think such descriptions are required somewhere. The API calls are not self-understandable to everyone probably.


.. tip::

In this response you can see that promotion and shipping have been taken into account to calculate an appropriate price
Copy link
Member

Choose a reason for hiding this comment

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

the appropriate price


.. code-block:: text

STATUS: 200 Ok
Copy link
Member

Choose a reason for hiding this comment

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

is it 200 Ok or 200 OK coz you have it both in these docs?

@lchrusciel
Copy link
Member Author

I know @TheMadeleine, but I have try to be as clear with our docs as possible :( Any advice, what should be removed would be appreciated :)

@lchrusciel lchrusciel force-pushed the cart-api branch 2 times, most recently from 85633aa to f772750 Compare January 31, 2017 07:45
number: desc
fields:
channel:
type: twig
Copy link
Member

Choose a reason for hiding this comment

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

In API?

Copy link
Member Author

Choose a reason for hiding this comment

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

🐋

variant: "@hard_available_mug"
order: "@fulfilled_cart"

Sylius\Component\Core\Model\OrderItemUnit:
Copy link
Member

Choose a reason for hiding this comment

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

I think creating entire order with units with fixtures can hurt us in the long run, but let's keep it for now, hopefully we can switch to Behat for API too and create these in the background at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is just needed to change cart item quantity. Checkout will be checked with request processing


A currency code will be added automatically based on a channel settings. :doc:`Read more about channels </book/configuration/channels>`

If you try to create a resource without name or code, you will receive a 400 error.
Copy link
Contributor

@tuka217 tuka217 Feb 2, 2017

Choose a reason for hiding this comment

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

@TheMadeleine made a suggestion that it should be a warning.
And 400 Bad Request.

Example
^^^^^^^

To see delete the cart with id 21 use the method below.
Copy link
Contributor

Choose a reason for hiding this comment

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

To delete

@@ -2,6 +2,7 @@
* :doc:`/api/authorization`
* :doc:`/api/channels`
* :doc:`/api/orders`
* :doc:`/api/carts`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be before order

type: string
sortable: customer.email
path: customer.email
options:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be removed

@@ -37,7 +37,7 @@
}
}
},
"currency_code": "USD",
"currency_code": "USD", "locale_code": "en_US",
Copy link
Member

Choose a reason for hiding this comment

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

:(

@@ -39,6 +39,6 @@
},
"payments": [],
"shipments": [],
"currency_code": "USD",
"currency_code": "USD", "locale_code": "en_US",
Copy link
Member

Choose a reason for hiding this comment

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

:(

+-------------------+-------------------------------------------------------------------+
| adjustments | List of adjustments related to the cart |
+-------------------+-------------------------------------------------------------------+
| adjustments_total | Sum of all order adjustments values |
Copy link
Contributor

Choose a reason for hiding this comment

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

Since those are adjustments related to the cart, as stated above, shouldn't this order be changed to cart as well?

+-------------------+-------------------------------------------------------------------+
| total | Sum of items total and adjustments total |
+-------------------+-------------------------------------------------------------------+
| customer | :doc:`Customer detailed serialization </api/customers>` for order |
Copy link
Contributor

Choose a reason for hiding this comment

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

for cart

.. warning::

Remember, that it doesn't replicate the environment of shop usage. It is more like an admin part of cart creation, which will allow you to manage
cart from the admin perspective. ShopAPI is still an experimental concept.
Copy link
Contributor

Choose a reason for hiding this comment

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

Either the cart or carts

{
"customer": "shop@example.com",
"channel": "US_WEB",
"locale_code": "en_US"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the locale_code needed to create the cart, but not included in the response?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

+===============+================+======================================+
| Authorization | header | Token received during authentication |
+---------------+----------------+--------------------------------------+
| id | url attribute | Id of the requested resource |
Copy link
Contributor

Choose a reason for hiding this comment

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

resource => cart lets be specific

sylius_api_order_item:
resource: "@SyliusApiBundle/Resources/config/routing/order_item.yml"
prefix: /orders/{orderId}/items
sylius_api_cart_item:
Copy link
Contributor

Choose a reason for hiding this comment

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

Move it under the cart's route

*/
public function it_denies_carts_deletion_for_non_authenticated_user()
{
$this->client->request('DELETE', '/api/v1/carts/-1');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if we actually tried to delete an existing cart.

public function it_does_not_allow_to_delete_orders_in_state_different_than_cart()
{
$this->loadFixturesFromFile('authentication/api_administrator.yml');
$carts = $this->loadFixturesFromFile('resources/order.yml');
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're defining it as a non-cart object (as stated in the test's title), should the variables still be named cart?

"This form should not contain extra fields."
],
"children": {
"quantity": {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the array closed here...

],
"children":{
"quantity":{

Copy link
Contributor

Choose a reason for hiding this comment

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

... but here's wide open?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed


.. warning::

If you try to create a resource without name or code, you will receive a ``400 Bad Request`` error, that will contain validation errors.
Copy link
Contributor

Choose a reason for hiding this comment

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

without localeCode, channel or customer?

^^^^^^^

To add a new item of a variant with code ``MEDIUM_MUG_CUP``
to the cart with id 21(assuming, that we didn't remove it in the previous example) use the below method:
Copy link
Contributor

Choose a reason for hiding this comment

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

id = 21

Response
~~~~~~~~
Example Response
~~~~~~~~~~~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

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

In previous doc you use Exemplary Response, so which phrase is correct?

Copy link
Contributor

@tuka217 tuka217 Feb 7, 2017

Choose a reason for hiding this comment

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

And it is at the same level as Example or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exemplary Response 👍

Creating a Cart
---------------

To create a new cart you will need to call the ``/api/v1/carts/`` endpoint with ``POST`` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

the ``POST``

Collection of Carts
-------------------

To retrieve a paginated list of carts you will need to call the ``/api/v1/carts/`` endpoint with ``GET`` method.
Copy link
Contributor

Choose a reason for hiding this comment

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

the``GET``

@pjedrzejewski
Copy link
Member

Thanks Łukasz! Nice work!

@lchrusciel lchrusciel deleted the cart-api branch February 9, 2017 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API APIs related issues and PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants