mollie / mollie-api-php

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

$payment->getRefund(...) missing #235

Closed sandervanhooft closed 6 years ago

sandervanhooft commented 6 years ago

Specifications

Describe the issue

Working on the API docs, discovered there's no $client->refunds->get('refund_id_here') method. Makes it hard to retrieve or cancel a single refund.

Not worked on this package yet, let me know if you'd like me to create a PR.

willemstuursma commented 6 years ago

If I am not mistaken, the refund has to be "get" from the Payment. It cannot be retrieved from v2/refunds/re_xxxxx. See the self URL in the _links property.

I guess for canceling a refund, you would usually have the paymentId on hand right?

Do you have a use case?

sandervanhooft commented 6 years ago

Agreed. Retrieving the refund via the payment is fine. I was a bit confused because $client->refunds exists (RefundEndpoint).

There's however no way (afaik) to retrieve a single refund (by refund-id). Right now you need to manually filter the results from $payment->refunds().

Perhaps add a $payment->getRefund(...) helper?

sandervanhooft commented 6 years ago

(There's a similar getMandate('some_id') helper method on the Customer resource.)

Smitsel commented 6 years ago

Its allright if you make a PR for it :)

sandervanhooft commented 6 years ago

Ok, to be sure, that would require the following to be implemented:

RefundEndpoint:
tests/Mollie/API/Endpoints/RefundEndpointTest : testGetForWorks
src/Endpoints/RefundEndpoint : getFor(Payment $payment, $refundId, array $parameters = [])

PaymentResource:
tests/Mollie/API/Resources/Payment : testGetRefundWorks
src/Resources/Payment : getRefund($refundId, array $parameters = [])
sandervanhooft commented 6 years ago

Will work on this later this week.

Also noted that there's a minor docbloc issue in the Payment resource, referencing $filters, which should be $data.

Will take care of this as well.

sandervanhooft commented 6 years ago

Hi @Smitsel ,

Working on it...

Shouldn't RefundEndpoint have payments_refunds as the resource path?

class RefundEndpoint extends EndpointAbstract
{
    protected $resourcePath = "refunds"; // should be "payments_refunds"?
class MandateEndpoint extends EndpointAbstract
{
    protected $resourcePath = "customers_mandates"; // seems right

Edit: I'll make it work as is, but perhaps it's good to open a separate issue for this.

sandervanhooft commented 6 years ago

While implementing $client->refunds->getFor($payment, $refundId, $params) I now get these test results:

1) Tests\Mollie\Api\Endpoints\RefundEndpointTest::testGetRefundOnPaymentResource
URI path must be identical
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/v2/payments/tr_44aKxzEbr8/refunds/re_PsAvxvLsnm'
+'/v2/refunds/re_PsAvxvLsnm'

RefundEndpoint method

    /**
     * @param Payment $payment
     * @param string $refundId
     * @param array $parameters
     *
     * @return Refund
     */
    public function getFor(Payment $payment, $refundId, array $parameters = [])
    {
        $this->parentId = $payment->id;

        return parent::rest_read($refundId, $parameters);
    }
sandervanhooft commented 6 years ago

I'll prepare a PR and leave the nested resource path commented out for now.

Smitsel commented 6 years ago

Pull request has been merged! Thanks @sandervanhooft