mollie / mollie-api-php

Mollie API client for PHP
http://www.mollie.com
BSD 2-Clause "Simplified" License
552 stars 191 forks source link

[Beta]Order and Shipment integration #247

Closed sandervanhooft closed 6 years ago

sandervanhooft commented 6 years ago

Specifications

Describe the issue

I am preparing the Order and Shipment integrations in this fork/branch.

Any discussion on the development can take place here in this issue.

PR is here: #252.

Endpoints:

Resources:

Types:

Documentation:

sandervanhooft commented 6 years ago

The response for OrderEndpoint.get/create/list includes order lines (lines).

@ndijkstra Should this be an OrderLineCollection resource?

It would be a first for this client to include a nested resource(collection) in a response. It would probably require overriding the rest_create method.

ndijkstra commented 6 years ago

The order lines are in the order response. The order line endpoint is only for canceling an order line

sandervanhooft commented 6 years ago

Ok, so no OrderLineCollection is necessary on the Order resource?

I'll update the plan above to only include the cancel method on OrderLine.

ndijkstra commented 6 years ago

I think that is correct. @Smitsel Can you check if this is ok?

Smitsel commented 6 years ago

I think it is, take for example the IssuerCollection. These are an include on the methods api. But its rather useful to make them value objects.

This way we can iterate the OrderLineCollection and create cool helper functions like:$orderline->cancel().

So please do add them :)

Smitsel commented 6 years ago

I can also see value for people implementing this library and typehint them for example.

sandervanhooft commented 6 years ago

Thanks @Smitsel ! I overlooked the IssuerCollection on the methods api. I'll look into that one for inspiration.

sandervanhooft commented 6 years ago

By the way, why are all properties defined explicitly on the resource classes? The tests are passing without. Is this for autocompletion?

Smitsel commented 6 years ago

Yes it is.

sandervanhooft commented 6 years ago

Are there any other relations I should be aware of, like to a customer entity?

I.e. the word "customer" is used a lot in the order docs, but there seems to be no relationship defined.

sandervanhooft commented 6 years ago

I'm assuming there's going to be a Order cancel (delete) api endpoint, right?

I.e.

curl -X DELETE https://api.mollie.com/v2/orders/ord_kEn1PlbGa \
       -H "Authorization: Bearer test_dHar4XY7LxsDOtmnkVtjNVWXLSlXsM"
ndijkstra commented 6 years ago

Correct. Docs coming soon ๐Ÿ˜‡

sandervanhooft commented 6 years ago

OrderEndpoint ready (see backlog). Had to use some response fixtures and assertion helpers to keep the test file length in check. Reduced it from 1600+ lines to less than 600. Pfew ๐Ÿ˜“

willemstuursma commented 6 years ago

Had to use some response fixtures and assertion helpers to keep the test file length in check.

Yes, it is very important to remember that tests are software too, and they can massively benefit from abstractions just as the units under test.

holtkamp commented 6 years ago

@sandervanhooft just noted the incorrect casing of Stdclass, it should be stdClass

https://github.com/sandervanhooft/mollie-api-php/blob/02bc1988b91ef66cc28d6ada659f4fe10aa8c211/tests/Mollie/API/Endpoints/OrderEndpointTest.php#L335

sandervanhooft commented 6 years ago

Thanks @holtkamp! Interestingly though, the tests pass... Nevertheless, I'll convert StdClass into stdClass as suggested.

willemstuursma commented 6 years ago

Class names are case-insensitive in PHP @sandervanhooft, only the Composer autoloaders (PSR-0, PSR-4) are.

sandervanhooft commented 6 years ago

Working on the OrderResource statuses and status methods now.

Can someone with Order API domain knowledge check whether this test (link) is ok? If so I can continue with implementation.

(The docs are not ready for this yet, only stating "See Order status changes for details on the ordersโ€™ statuses.", but no link is provided / page is not available yet.)

sandervanhooft commented 6 years ago

Class names are case-insensitive in PHP @sandervanhooft, only the Composer autoloaders (PSR-0, PSR-4) are.

Good to know ๐Ÿ˜„

Smitsel commented 6 years ago

Hi Sander, the status fields are as described in the docs. So your status methods are correct.

What we ment with the "order status changes details" is the transitions between statuses. We will add a guide for this and update accordingly.

Take a look at the cancel order endpoint docs and you will understand why we want such a guide ;)

sandervanhooft commented 6 years ago

Ok @Smitsel ! So I'm going to assume there's no additional logic to implement than (pseudo):

public function isSomeStatus()
{
    return $this->status === ORDER_STATUS::SOME_STATUS;
}
sandervanhooft commented 6 years ago

Take a look at the cancel order endpoint docs and you will understand why we want such a guide ;)

Indeed I do ๐Ÿ˜†

Seems to me that there should be a isCancelable method on the OrderLine resource as well.

sandervanhooft commented 6 years ago

Perhaps this would require an additional isCancelable field in the Order API response? I noticed it on some other resources.

sandervanhooft commented 6 years ago

Ok, I'm mixing things up here. I think we need this isCancelable method both on the Order and the OrderLine.

I can probably implement the logic client side, but I think it's better to include a isCancelable field on both the Order and the OrderLine responses, to keep the logic where it belongs - server side.


Update: I've implemented Order.isCancelable() and OrderLine.isCancelable() with client side logic for now.

sandervanhooft commented 6 years ago

Like with the payment response, it has a isCancelable field.

sandervanhooft commented 6 years ago

@Smitsel Minor doc issue: the id is missing on the Order line details. (Link)

sandervanhooft commented 6 years ago

As you can see here: nearly done with the implementation!

I'd like to manually test this against the private beta api when it's available.

sandervanhooft commented 6 years ago

@ndijkstra @Smitsel @willemstuursma

Next steps:

sandervanhooft commented 6 years ago

I just read up on the Order docs. It seems the void status is dropped, and the expired status is new.

Is that correct?

willemstuursma commented 6 years ago

@sandervanhooft yes we are indeed re-considering some of the statuses

sandervanhooft commented 6 years ago

@willemstuursma Thanks. So it's not final yet? In that case I'll leave it alone for now.

sandervanhooft commented 6 years ago

Added the examples.

sandervanhooft commented 6 years ago

@ndijkstra @Smitsel @willemstuursma

Similar to the isCancelable field, I think there should be an isRefundable field on the Order response. What do you think?

willemstuursma commented 6 years ago

We'll create a field for the refundable amount probably.

sandervanhooft commented 6 years ago

@willemstuursma Then an isRefundable() helper would be nice

sandervanhooft commented 6 years ago

Order.lines can be accessed as OrderLine resources via the Order.lines() method.

Can we do the same with Refund.lines / Refund.lines()? Or is this is a slightly different resource than the OrderLine?

Update: Just talked with @ndijkstra. He's on this.

sandervanhooft commented 6 years ago

@Smitsel When you review this, be sure to keep an eye out for OAuth compatibility. I've not explicitly tested this.

willemstuursma commented 6 years ago

@sandervanhooft we want to merge this into a branch and create a pre-release, is that OK for you?

sandervanhooft commented 6 years ago

@willemstuursma Great idea!

I've just made some comments at the @Smitsel 's PR review #252 . And incorporated some of his remarks, minor new PR coming your way.

sandervanhooft commented 6 years ago

And received some feedback from @Smitsel already (thanks!). Will implement this, expect the PR later today.

Smitsel commented 6 years ago

I think we've got everything. If we still miss some stuff we can open a new issue :)