-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow paying with using on-site credit card #22
Allow paying with using on-site credit card #22
Conversation
e8ade80
to
9b21855
Compare
d35d5ff
to
6272f93
Compare
if ('failed' === $response['result']) { | ||
$details['tpay']['status'] = PaymentInterface::STATE_FAILED; | ||
} | ||
|
||
if ('success' === $response['result'] && 'pending' === $response['status']) { | ||
$details['tpay']['transaction_payment_url'] = $response['transactionPaymentUrl']; | ||
|
||
$model->setDetails($details); | ||
|
||
throw new HttpRedirect($details['tpay']['transaction_payment_url']); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be checked: make sure it handles all possible cases
Improvement: we might introduce ->isFailed()
or isRequiring3DS()
to make it cleaner;
|
||
$builder->addEventListener( | ||
FormEvents::PRE_SUBMIT, | ||
[$this->removeUnnecessaryPaymentDetailsFieldsListener, '__invoke'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[$this->removeUnnecessaryPaymentDetailsFieldsListener, '__invoke'], | |
[$this->removeUnnecessaryPaymentDetailsFieldsListener], |
Will it also work like this?
$form = $this->prophesize(FormInterface::class); | ||
$form->remove('card')->shouldBeCalled()->willReturn($form); | ||
$form->remove('blik_token')->shouldNotBeCalled(); | ||
|
||
$event = new FormEvent($form->reveal(), ['blik_token' => '123456']); | ||
|
||
$this->createTestSubject()->__invoke($event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's totally fine now, but I'd like to share an idea to follow AAA pattern. All "should" methods we could move at the end. WDYT?
$form = $this->prophesize(FormInterface::class); | |
$form->remove('card')->shouldBeCalled()->willReturn($form); | |
$form->remove('blik_token')->shouldNotBeCalled(); | |
$event = new FormEvent($form->reveal(), ['blik_token' => '123456']); | |
$this->createTestSubject()->__invoke($event); | |
$form = $this->prophesize(FormInterface::class); | |
$form->remove('card')->willReturn($form); | |
$event = new FormEvent($form->reveal(), ['blik_token' => '123456']); | |
$this->createTestSubject()->__invoke($event); | |
$form->remove('card)->shouldBeCalled(); | |
$form->remove('blik_token')->shouldNotBeCalled(); |
'mapped' => false, | ||
'label' => 'commerce_weavers_sylius_tpay.shop.order_summary.card.expiration_date.month.label', | ||
'choices' => [ | ||
'commerce_weavers_sylius_tpay.shop.order_summary.card.expiration_date.month.january' => '01', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$choices = [];
foreach (['january, 'february', '...', 'december'] as $key => $month) {
$choices[sprintf('commerce_weavers_sylius_tpay.shop.order_summary.card.expiration_date.month.%s', $month)] = sprintf("%02d", $key + 1)
}
WDYT?
|
||
use Symfony\Component\Form\FormEvent; | ||
|
||
final class RemoveUnnecessaryPaymentDetailsFieldsListener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we rather base our decision on the state of the payment itself? Only when given payment method will be selected, then we are adding given fields
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this solutions is enough. We base the decision about passed data on a type defined in the gateway (see my screenshot two comments below). Here we only check whether a some data is passed (template is no loaded = field is not shown = we're getting a null
); so we just remove it to avoid 500
.
|
||
$details = $model->getDetails(); | ||
|
||
return !isset($details['tpay']['card']) && !isset($details['tpay']['blik_token']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will we decide in UI which option will be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time, I believe this type should be a value on payment details, so it can be easily accessed e.g. in the Payum action, so instead this if
statement you marked, we'll do something like this:
return $details['tpay']['type'] === 'redirect';
Thanks to this change, once a new method is added, it's not required to modify the CreateRedirectBasedTransactionAction
class.
…ayPaymentDetailsType
c1f43c1
to
8b26e3a
Compare
closes #21