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.

Sign up for GitHub

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

Makes the order operation 'deliver' available. #114

Closed
wants to merge 3 commits into from
Closed

Makes the order operation 'deliver' available. #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
57dc3c2
deliver order operation
bogdan202 Dec 14, 2023
1fb8565
OrderOperation::deliver() + test + doc
bogdan202 Dec 15, 2023
9c1a4dc
test
bogdan202 Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/manual/resources/order.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,24 @@ $operation
$orderApi->execute($operation);
```


### Deliver

The deliver operation accepts 3 parameters:

1. [mandatory] `$id` : Order id (eg: '123456qwerty')
2. [optional] `$reference` : Order reference (eg: 'reference1')
3. [optional] `$channelName` : The channel where the order is from (eg: 'amazon')

Example :

```php
$operation = new \ShoppingFeed\Sdk\Api\Order\OrderOperation();
$operation->deliver('123456qwerty');
$orderApi->execute($operation);
```


### Refund

The refund operation accepts 4 parameters :
Expand Down
27 changes: 27 additions & 0 deletions src/Api/Order/OrderOperation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class OrderOperation extends Operation\AbstractBulkOperation
public const TYPE_ACKNOWLEDGE = 'acknowledge';
public const TYPE_UNACKNOWLEDGE = 'unacknowledge';
public const TYPE_UPLOAD_DOCUMENTS = 'upload-documents';
public const TYPE_DELIVER = 'deliver';

/**
* @var array
Expand All @@ -34,6 +35,7 @@ class OrderOperation extends Operation\AbstractBulkOperation
self::TYPE_ACKNOWLEDGE,
self::TYPE_UNACKNOWLEDGE,
self::TYPE_UPLOAD_DOCUMENTS,
self::TYPE_DELIVER,
Copy link

@KevinFrydman KevinFrydman Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi, you need to add also a new method in this class :

public function deliver($reference, $channelName)
    {
        $this->addOperation(
            $reference,
            $channelName,
            self::TYPE_DELIVER
        );

        return $this;
    }

And make some tests in the OrderOperationTest class

Copy link
Author

Choose a reason for hiding this comment

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

I see, but I would like to point out that the parameters 'reference' and 'channelName' aren't required by API anymore. Instead, the parameter 'id' should be sent. I think all methods in this class should be adapted to the new API.
Do you agree?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @bogdan202, thanks for the PR. In addition to Kevin's feedback, can you add a section in documentation here please : https://github.com/shoppingflux/php-sdk/blob/master/docs/manual/resources/order.md#refund

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, but I would like to point out that the parameters 'reference' and 'channelName' aren't required by API anymore. Instead, the parameter 'id' should be sent. I think all methods in this class should be adapted to the new API. Do you agree?

Yes, you are right. This version was meant to be compatible to be integrated by old modules. It would be a good step towards the migration if the SDK could manage order's ID in addition to channel's name & order's reference.

];

/**
Expand Down Expand Up @@ -361,4 +363,29 @@ public function refund($reference, $channelName, $shipping = true, $products = [

return $this;
}

/**
* Notify marketplace of order delivery
*
* @param string $id Order id
* @param string $reference Order reference
* @param string $channelName Channel to notify
*
* @return OrderOperation
*
* @throws Exception\InvalidArgumentException
*/
public function deliver($id, $reference = '', $channelName = ''): self
Copy link
Contributor

Choose a reason for hiding this comment

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

However, I am not favorable to proposed implementation because it breaks the object consistency & it only manage IDs for deliver operation. We are a bit annoyed here, because the number of parameter has to change from OrderOperation::<operation>($reference, $channelName, $operationParams...) to OrderOperation::<operation>($id, $operationParams...).

My recommandation would probably to create a new OrderOperation object with fixed signatures and deprecate the actual one. SDK user would be able to use the new object if he want. I can understand that is much more work than your need here.

For now, I suggest you to keep the object consitency with channel name & reference, and if you need to use IDs, to manage it on your own. I will check on my side if we can schedule to update SDK soon to manage order's ID. However, I can't give you any date yet.

Copy link
Author

Choose a reason for hiding this comment

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

@mdumoulin, the following implementation of the method deliver is ok for you?

public function deliver($reference, $channelName, $id): self
{
        $this->addOperation(
            $reference,
            $channelName,
            self::TYPE_DELIVER,
            [
                'id' => $id,
            ]
        );

        return $this;
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because it should be either $reference and $channelName; or $id. The only suitable solution to implement it in actual object is :

public function deliver($reference, $channelName): self
{
    $this->addOperation(
        $reference, 
        $channelName, 
        self::TYPE_DELIVER
    );

    return $this;
}

A possible solution to manage ID would be to create another OrderOperation object. In short it could be something like that :

<?php

namespace ShoppingFeed\Sdk\Api\Order;

use ShoppingFeed\Sdk\Operation\AbstractBulkOperation;

class Operation extends AbstractBulkOperation
{
    public function deliver(OrderIdentifier $id): self
    {
        $this->addOperation(
            $id,
            self::TYPE_DELIVER
        );

        return $this;
    }
}

interface OrderIdentifier
{
    public function toArray(): array;
}

class Id implements OrderIdentifier
{
    private $id;

    public function __construct(int $id)
    {
        $this->id = $id;
    }

    public function toArray(): array
    {
        return [
            'id' => $this->id,
        ];
    }
}

class Reference implements OrderIdentifier
{
    private $channelName;

    private $reference;

    public function __construct(string $channelName, string $reference)
    {
        $this->channelName = $channelName;
        $this->reference = $reference;
    }

    public function toArray(): array
    {
        return [
            'channelName' => $this->channelName,
            'reference'   => $this->reference,
        ];
    }
}

{
$this->addOperation(
$reference,
$channelName,
self::TYPE_DELIVER,
[
'id' => $id,
]
);

return $this;
}
}
42 changes: 42 additions & 0 deletions tests/unit/Api/Order/OrderOperationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,4 +361,46 @@ public function testRefundOperation()
)
);
}

/**
* @throws \Exception
*/
public function testDeliverOperation()
{
$data = [
'123456qwerty',
'ref1',
'amazon',
];

$op = new Api\Order\OrderOperation();
$instance = $this
->getMockBuilder(Api\Order\OrderOperation::class)
->setMethods(['addOperation'])
->getMock();

$instance
->expects($this->once())
->method('addOperation')
->with(
'ref1',
'amazon',
Api\Order\OrderOperation::TYPE_DELIVER,
$this->callback(
function ($param) {
return $param['id'] === '123456qwerty';
}
)
);

$this->assertInstanceOf(
Api\Order\OrderOperation::class,
$instance->deliver(...$data)
);
$op->deliver(...$data);
$this->assertEquals(
1,
$op->count(Api\Order\OrderOperation::TYPE_DELIVER)
);
}
}