#13 - order status update #35

ddattee · 2018-05-14T14:49:52Z

Adding accept, refuse, ship, and cancel order operations call and resolve #13 and resolve #16 .

…more important

…urce

ronan-gloo · 2018-05-22T09:01:20Z

src/Api/Order/OrderResource.php

+    {
+        $dateValue = $this->getProperty('updatedAt');
+        if (false === $dateValue) {
+            return false;


@ddattee null|\DateTimeImmutable, on évite de retourner deux types différents.

ça nous permet également de migrer vers php7 avec le support des types de retour

ronan-gloo · 2018-05-22T09:03:17Z

src/Order/OrderOperation.php

+    public function addOperation($reference, $channelName, $type, $data = [])
+    {
+        if (! in_array($type, $this->allowedOperationTypes)) {
+            throw new \Exception(sprintf(


Pas d'exception du scope global, sinon le client ne pourra pas cibler précisément les exceptions du SDK à plus haut niveau dans son code.

ronan-gloo · 2018-05-22T09:06:11Z

tests/unit/Operation/AbstractBulkOperationTest.php

+                }
+            },
+            'group1'
+        );


Une règle des tests unitaire est de les garder le plus simple possible, et donc éviter les block.
Essaie aussi d'expliquer tes intentions (dans le nom du test case, et avec des commentaires lors des assertions), car à la lecture, ce n'est pas tout de suite évident de savoir quel comportement est vérifié ici

ronan-gloo · 2018-05-22T09:07:56Z

src/Api/Order/OrderDomain.php

+    {
+        $this->orderOperations = $orderOperation;
+        if (! $this->orderOperations) {
+            $this->orderOperations = new OrderOperation();


Comme discuté, maintenir l'état de orderOperations dans cette classe expose à des effets de bord sur d'éventuels calls consécutifs

…tiple type return

…ric one

ronan-gloo · 2018-05-28T08:13:41Z

src/Api/Order/OrderDomain.php

+     *
+     * @return OrderOperation
+     */
+    public function newOperations()


Merci de supprimer ce shortcut (et de mettre la documentation à jour pour être conforme avec ce que l'on a fait sur inventory:

<?php $op = new OrderOperation(); $api->execute($op);

ronan-gloo · 2018-05-28T08:14:37Z

docs/order.md

+    ->newOperations()
+    ->ship('ref1', 'amazon')
+    ->ship('ref2', 'amazon', 'ups', '123456789abcdefg', 'http://tracking.url/')
+    ->execute($orderApi->getLink());


Quelles est (sont) la (les) valeurs de retour pour chacune des opérations ?

ronan-gloo · 2018-05-28T08:23:51Z

src/Operation/AbstractBulkOperation.php

     */
-    protected function eachBatch(callable $callback)
+    protected function eachBatch(callable $callback, $groupedBy = null)


Ce n'est pas un "regroupement", mais un filtre (ex du GROUP BY sql)

La il s'agit bien d'un regroupement : on regroupe les requetes par operations car les batch ne peuvent être fait que sur une opération à la fois car on a 1 url par operation.
X batchs pour les cancels, Y batchs pour les accepts...

vu ce cette classe, c'est bien un filtre, puisque seulement la clé $groupedBy est exécutée

ronan-gloo · 2018-05-28T08:24:43Z

src/Order/OrderOperation.php

+            $this->getPoolSize()
+        );
+
+        return new Api\Order\OrderCollection($resources);


On ne retourne pas une collection de order, mais des "tickets" puisque ces ops sont async

ronan-gloo · 2018-05-28T08:32:31Z

@ddattee Petite question: Quant est-il de acknowlegde / unacknowlegde ?
Ce sont principalement ces opérations que les plugins ont besoin

ronan-gloo · 2018-05-28T08:37:36Z

src/Order/OrderOperation.php

+    {
+        // Create requests per batch
+        $requests = [];
+        foreach ($this->allowedOperationTypes as $type) {


Ton regroupement est fait ici ;)

ronan-gloo · 2018-05-28T08:37:58Z

src/Order/UnexpectedTypeException.php

+
+class UnexpectedTypeException extends \Exception
+{
+


ronan-gloo · 2018-05-28T08:39:46Z

src/Api/Order/OrderResource.php

+    public function getUpdateddAt()
+    {
+        $dateValue = $this->getProperty('updatedAt');
+        if (false === $dateValue) {


getProperty ne retourne jamais FALSE, donc se référer à la doc API pour les valeurs non existantes :

php-sdk/src/Hal/HalResource.php

Lines 75 to 82 in 50150d4

public function getProperty($name, $default = null)

{

if (isset($this->properties[$name])) {

return $this->properties[$name];

}

return $default;

}

… update

…ration API

ddattee · 2018-05-28T10:51:29Z

@ronan-gloo un/acknowledge operation were on another branch I added them to this PR

ronan-gloo · 2018-05-28T13:29:16Z

docs/order.md

+```php
+<?php
+/** @var \ShoppingFeed\Sdk\Api\Order\OrderDomain $orderApi */
+$operations = new \ShoppingFeed\Sdk\Order\OrderOperation();


UpdateOperation 😄

ronan-gloo · 2018-05-30T13:38:04Z

docs/order.md

+    ->refuse('ref4', 'amazon')
+    ->ship('ref5', 'amazon')
+    ->cancel('ref3', 'amazon')
+    ->execute($orderApi->getLink());


cf commentaire sur inventory #36 (comment)

ronan-gloo · 2018-05-31T10:46:06Z

@ddattee Operations should be located at AP namespace level, it makes the sdk organisation more predictable and consistent.

ddattee · 2018-05-31T14:00:14Z

@ronan-gloo done and I updated InventoryUpdate too.

ronan-gloo · 2018-06-01T07:27:00Z

src/Api/Order/OrderOperation.php

+            },
+            null,
+            [],
+            $this->getPoolSize()


Pour la prochaine implémentation d'opérations sur une resource, faudra réfléchir à factoriser le tout pour éviter la duplication de code ;)

Oui et faciliter le test des closure :)

ronan-gloo

Cf commentaires.

Faut assurer checker le coverage sur les classes API, on n'est pas à 100% là ;)

ronan-gloo · 2018-06-04T08:34:40Z

docs/order.md

+/** @var \ShoppingFeed\Sdk\Api\Order\OrderDomain $orderApi */
+$updateOperation = new \ShoppingFeed\Sdk\Api\Order\OrderOperation();
+/** @var \ShoppingFeed\Sdk\Api\Order\OrderTicketCollection $ticketCollection */
+$ticketCollection = $updateOperation


c'est la méthode execute qui retourne $ticketCollection non ?

ronan-gloo · 2018-06-04T08:37:41Z

src/Api/Order/OrderOperation.php

+     * @param Request         $request
+     * @param                 $ticketReferences
+     */
+    public function associateTicketWithReference(


Cette méthode ne doit pas faire partir de l'API publique de la classe, faudrait la passer en private

ronan-gloo · 2018-06-04T08:39:28Z

src/Api/Order/OrderTicketCollection.php

+     *
+     * @return OrderTicketCollection
+     */
+    public function setTicketReferences($ticketReferences)


Les références devraient être passées à la construction de l'objet, car là on ouvre l'accès aux modifications depuis l'extérieur.

Faudrait aussi caster $ticketReferences en array

ronan-gloo · 2018-06-04T08:41:16Z

src/Api/Order/OrderTicketCollection.php

+     *
+     * @throws Order\Exception\TicketNotFoundException
+     */
+    public function getShippedTicket($reference)


Pour toutes les méthodes:

Le paramètre $reference devrait être optionnel : dans ce cas, on retourne tous les tickets liés aux opérations "shipped"

côté sémantique, le "ticket" est redondant, c'est une collection de tickets ;)

@ronan-gloo donc on retourne 2 types de donnée différent en fonction de ce qu'on trouve (1 resource ou un array de resource) ou alors on retourne un array dans tt les cas mais on ne peut plus faire l'enchainement : $ticketCollection->getAccepted('ref3')->getId()

Ok pour la sémantique.

En effet.

Je propose que l'on remette tout ça à plus tard, le temps de trancher pour une API consistante.
On est un peu hors scope de toute manière pour le moment, donc on peu se contenter de la collection d'instances de ticket, sans accepteurs particuliers, et on blinde la suite dans une prochaine iteration.

ça te vas comme plan ?

J'ai passé le tout en full array du coup pour que ce soit consistant.
Mais je suis d'accord que la réflexion viendra avec l'API ticket surtout.

ronan-gloo · 2018-06-04T08:43:07Z

src/Operation/AbstractBulkOperation.php

@@ -62,13 +62,35 @@ public function getPoolSize()
        return $this->poolSize;
    }

+    public function countOperation($groupedBy = null)


J'aurais plutôt implémenté \Countable ici. Faut aussi penser au docblock

Si je passe par \Countable je ne peux pas passer de filter (incompatible avec la signature de l'interface) je vais juste la renommer en count().
D'autant que l'interface est déjà implémenter au niveau du parent.

Ah oui mal lu, désolé

Descends plutôt ça dans TicketCollection pour le moment, on a rien dans la classe abstraite qui décris une structuration des opérations (ex sur inventory). Quitte à le remonter plus tard si c'est pertinent.

@ronan-gloo le count() d'operation dans TicketCollection ? pas sur de comprendre la

ronan-gloo · 2018-06-04T08:44:28Z

src/Order/Exception/TicketNotFoundException.php

+     *
+     * @return TicketNotFoundException
+     */
+    public static function withOperation($operation)


Côté convention, with() sont plutôt méthodes qui clone les instances existantes (PSR http-message par exemple)
Se serait plutôt fromOperation() pour un constructeur nommé

…ce in constructor, setting $reference as optionnal, better count method, renaming exception method, updating ut

ddattee added 2 commits May 7, 2018 17:44

shoppingflux#12 - Order - adding main API classes

e56159e

shoppingflux#13 - Order - adding status operation api calls

602536b

ddattee added the enhancement New feature or request label May 14, 2018

ddattee added this to the v0.2 milestone May 14, 2018

ddattee self-assigned this May 14, 2018

ddattee added 9 commits May 14, 2018 17:26

shoppingflux#13 - Order - generalizing grouping of bulk operations

c9b60a1

shoppingflux#13 - Order - adding unit test

64a8102

shoppingflux#13 - Composer - checking phpunit before phpcs for it is …

d2a0603

…more important

shoppingflux#13 - updating bulk operation unit test

c6a462c

shoppingflux#13 - Doc - updating doc after renaming method

e0c6ecb

shoppingflux#13 - Store - better naming of sub-api access

fc2d94e

shoppingflux#13 - Order - adding propererties retreival to order reso…

2c53411

…urce

shoppingflux#13 - UT - adding ut for order resource and updating store's

2795294

shoppingflux#13 - Order - updating ut to pass php 7+ tests

b291143

ronan-gloo requested changes May 22, 2018

View reviewed changes

ddattee added 6 commits May 22, 2018 11:08

shoppingflux#13 - Order - extracting order operation from domain

74b7bca

shoppingflux#13 - Order - updating ut

e47fb90

shoppingflux#13 - Order - returning null when false date to avoid mul…

0a6fd49

…tiple type return

shoppingflux#13 - Order - throw sdk local exception instead of a gene…

f636e18

…ric one

shoppingflux#13 - Order - updating ut

78de538

shoppingflux#13 - cleaning phpunit file

dbe45b0

ronan-gloo requested changes May 28, 2018

View reviewed changes

ronan-gloo reviewed May 28, 2018

View reviewed changes

shoppingflux#13 - removing newOperations method

515a865

ronan-gloo reviewed May 28, 2018

View reviewed changes

src/Order/UnexpectedTypeException.php Outdated

class UnexpectedTypeException extends \Exception

{

Copy link

Contributor

ronan-gloo May 28, 2018

Choose a reason for hiding this comment

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

space

shoppingflux#13 - code style

80b0dd5

ronan-gloo reviewed May 28, 2018

View reviewed changes

shoppingflux#16 - adding operation acknowledge and unacknowledge

10277da

ddattee added 6 commits May 28, 2018 11:44

shoppingflux#16 - updating to match last modification on order status…

f1c99d1

… update

shoppingflux#16 - updating doc for un/acknowledge

3de5561

shoppingflux#13 - renaming operation filtering on send

268bef1

shoppingflux#13 - adding ticket resource to materialize return of ope…

f25bdb9

…ration API

shoppingflux#13 - removing unnecessary check

30a21e5

shoppingflux#13 - adding ticket description on order operation doc

bc995fe

ddattee added 4 commits May 28, 2018 13:13

shoppingflux#13 - doc orth

584a970

shoppingflux#13 - fixing test for php7

30eb861

Inventory - roll back on out of scope modification

f72e6f7

shoppingflux#13 - ut renaming var

f971642

ronan-gloo reviewed May 28, 2018

View reviewed changes

ddattee added 3 commits May 28, 2018 16:26

shoppingflux#38 - adding ticket resource and collection basics

7e215b8

shoppingflux#13 - var name

4d2c607

shoppingflux#13 - adding ticket collection specific to order workflow

cbf206c

ronan-gloo requested changes May 30, 2018

View reviewed changes

ddattee added 2 commits May 31, 2018 11:25

shoppingflux#13 - moving execute back to domain object

12f5710

shoppingflux#13 - fixing ut

23e3324

shoppingflux#13 - moving OrderOperation into API

c917770

ddattee force-pushed the order-status-update branch from 3ac8339 to c917770 Compare May 31, 2018 13:52

ronan-gloo reviewed Jun 1, 2018

View reviewed changes

shoppingflux#13 - cleanup exception declaration

0e3dcd0

ronan-gloo requested changes Jun 4, 2018

View reviewed changes

ddattee added 2 commits June 4, 2018 13:16

shoppingflux#18 - fix doc, changing access type, moving ticketReferen…

e56453f

…ce in constructor, setting $reference as optionnal, better count method, renaming exception method, updating ut

shoppingflux#18 - fixing ut

f7a67f9

ronan-gloo approved these changes Jun 6, 2018

View reviewed changes

ddattee merged commit 7655890 into shoppingflux:develop Jun 6, 2018

	public function getProperty($name, $default = null)
	{
	if (isset($this->properties[$name])) {
	return $this->properties[$name];
	}

	return $default;
	}

#13 - order status update #35

#13 - order status update #35

Conversation

ddattee commented May 14, 2018 • edited Loading

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

ddattee May 28, 2018 • edited Loading

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

ronan-gloo commented May 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronan-gloo May 28, 2018 • edited Loading

Choose a reason for hiding this comment

ddattee commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronan-gloo commented May 31, 2018

ddattee commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronan-gloo 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

ddattee Jun 4, 2018 • edited Loading

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

ddattee Jun 4, 2018 • edited Loading

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

ddattee commented May 14, 2018 •

edited

Loading

ddattee May 28, 2018 •

edited

Loading

ronan-gloo commented May 28, 2018 •

edited

Loading

ronan-gloo May 28, 2018 •

edited

Loading

ddattee Jun 4, 2018 •

edited

Loading

ddattee Jun 4, 2018 •

edited

Loading