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

Added NotifyAction #28

Merged
merged 4 commits into from
Oct 10, 2016
Merged

Added NotifyAction #28

merged 4 commits into from
Oct 10, 2016

Conversation

koemeet
Copy link
Contributor

@koemeet koemeet commented Oct 6, 2016

This PR is a PoC for issue Payum/Payum#590.

I am trying to accomplish that the payment details gets synced when a Notify request comes in. Right now I simply replace the request details and also set some response specific data on it for debugging purposes.

@makasim Could you maybe check if this implementation is correct?

@makasim
Copy link
Member

makasim commented Oct 6, 2016

@steffenbrem Looks good to me. Can you add some tests?

@koemeet
Copy link
Contributor Author

koemeet commented Oct 6, 2016

@makasim Sure will do, wanted to have a confirmation first 👍

Copy link
Member

@makasim makasim left a comment

Choose a reason for hiding this comment

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

/**
* @author Steffen Brem <[email protected]>
*/
class NotifyAction extends BaseApiAwareAction implements GatewayAwareInterface
Copy link
Member

@makasim makasim Oct 6, 2016

Choose a reason for hiding this comment

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

It is better not to use that BaseApiAwareAction class. Do something like this:

<?php
class NotifyAction  implements GatewayAwareInterface, ApiAwareInterface, ActionInterface
{
   use ApiAwareTrait;
   public function __construct()
   {
      $this->apiClass = Foo::class;
   }
}

@koemeet
Copy link
Contributor Author

koemeet commented Oct 10, 2016

@makasim Some gateways only use the fetchTransaction (such as Mollie) so I use both now. The notify action will be used when either a fetchTransaction or a acceptNotification method is present on the omnipay api.

@koemeet
Copy link
Contributor Author

koemeet commented Oct 10, 2016

I think everything is ready now. I am currently using it in production and notify requests now properly updates the payments for me (I am using Sylius).

$response = $this->omnipayGateway->fetchTransaction($details->toUnsafeArray())->send();
if (method_exists($this->api, 'fetchTransaction')) {
$response = $this->api->fetchTransaction($details->toUnsafeArray())->send();
} else if (method_exists($this->api, 'acceptNotification')) {
Copy link
Member

Choose a reason for hiding this comment

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

should not we try call acceptNotification first?

@makasim makasim merged commit 85f22db into Payum:master Oct 10, 2016
@makasim
Copy link
Member

makasim commented Oct 10, 2016

@koemeet koemeet deleted the feature/notify-action branch October 10, 2016 21:32
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.

2 participants