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

Makes the order operation 'deliver' available. #114

Closed

Conversation

bogdan202
Copy link

Reason for this PR

The current PR makes the order operation 'deliver' available.
https://developer.shopping-feed.com/api/95a3d1e32e9c7-deliver-an-order

What does the PR do

It adds the operation 'deliver' to the list of allowed ones

@bogdan202 bogdan202 changed the title Make available the order operation 'deliver' Makes the order operation 'deliver' available. Dec 14, 2023
@@ -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

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.

*
* @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,
        ];
    }
}

@mdumoulin mdumoulin closed this Jul 16, 2024
@mdumoulin
Copy link
Contributor

Another PR has been opened : #119

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants