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

Handle Blik Level 0 payments #14

Merged
merged 11 commits into from
Sep 11, 2024
Merged

Conversation

arti0090
Copy link
Contributor

@arti0090 arti0090 commented Sep 5, 2024

closes #6

@arti0090 arti0090 linked an issue Sep 5, 2024 that may be closed by this pull request
5 tasks
config/packages.php Outdated Show resolved Hide resolved
config/packages/events.php Outdated Show resolved Hide resolved
config/packages/events.php Outdated Show resolved Hide resolved
config/services/form.php Show resolved Hide resolved
src/Form/Type/CompleteTypeExtension.php Outdated Show resolved Hide resolved
src/Payum/Action/Api/CreateBlik0TransactionAction.php Outdated Show resolved Hide resolved
translations/messages.pl.yaml Outdated Show resolved Hide resolved
translations/messages.en.yaml Outdated Show resolved Hide resolved
src/Payum/Action/CaptureAction.php Outdated Show resolved Hide resolved
src/Payum/Request/Api/CreateBlik0Transaction.php Outdated Show resolved Hide resolved
@arti0090 arti0090 force-pushed the 6-implement-handling-blik-level-0 branch from ff36684 to 4c2868a Compare September 5, 2024 12:52
@arti0090 arti0090 force-pushed the 6-implement-handling-blik-level-0 branch from 6995cbd to 3cffa46 Compare September 10, 2024 14:05
@arti0090 arti0090 force-pushed the 6-implement-handling-blik-level-0 branch from 3cffa46 to ddf63ec Compare September 10, 2024 14:12
@arti0090 arti0090 changed the title [WIP] 6 implement handling blik level 0 6 implement handling blik level 0 Sep 10, 2024
@arti0090 arti0090 force-pushed the 6-implement-handling-blik-level-0 branch from 26475bf to 914043f Compare September 11, 2024 06:19
@arti0090 arti0090 force-pushed the 6-implement-handling-blik-level-0 branch from 914043f to 69bc7fd Compare September 11, 2024 06:27
@arti0090
Copy link
Contributor Author

arti0090 commented Sep 11, 2024

Missing points:

  • Validation for BLIK input field
  • Missing T&C Link on the input
  • Input currently shows as another payment "label" field rather then overriding existing one
  • no spinner that is waiting for payment status

@jakubtobiasz
Copy link
Member

I'll make a separate task to cover non happy path cases.

@jakubtobiasz jakubtobiasz changed the title 6 implement handling blik level 0 Handle Blik Level 0 payments Sep 11, 2024
@jakubtobiasz jakubtobiasz merged commit 100299b into main Sep 11, 2024
4 checks passed
@jakubtobiasz jakubtobiasz deleted the 6-implement-handling-blik-level-0 branch September 11, 2024 15:12
@jakubtobiasz jakubtobiasz mentioned this pull request Sep 11, 2024
1 task
'sylius.shop.checkout.complete.summary' => [
'blocks' => [
'blik' => [
'template' => '@CommerceWeaversSyliusTpayPlugin/blik.html.twig',
Copy link
Member

Choose a reason for hiding this comment

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

Templates like that should be rather placed in some subfolder. Shop prefix should be placed at least

@@ -4,6 +4,7 @@

namespace Symfony\Component\DependencyInjection\Loader\Configurator;

use CommerceWeavers\SyliusTpayPlugin\Payum\Action\Api\CreateBlik0TransactionAction;
Copy link
Member

Choose a reason for hiding this comment

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

Let's adjust naming to @coldic3 suggestion

Suggested change
use CommerceWeavers\SyliusTpayPlugin\Payum\Action\Api\CreateBlik0TransactionAction;
use CommerceWeavers\SyliusTpayPlugin\Payum\Action\Api\CreateBlikLevelZeroTransactionAction;

if @jakubtobiasz is fine with that

@@ -20,6 +21,7 @@
$services->set(CaptureAction::class)
->args([
service('commerce_weavers.tpay.payum.factory.create_transaction'),
service('commerce_weavers.tpay.payum.factory.create_blik0_transaction'),
Copy link
Member

Choose a reason for hiding this comment

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

or at least we can adjust service names

Suggested change
service('commerce_weavers.tpay.payum.factory.create_blik0_transaction'),
service('commerce_weavers.tpay.payum.factory.create_blik_level_zero_transaction'),

@@ -28,4 +29,8 @@
$services->set('commerce_weavers.tpay.payum.factory.create_transaction', CreateTransactionFactory::class)
->alias(CreateTransactionFactoryInterface::class, 'commerce_weavers.tpay.payum.factory.create_transaction')
;

$services->set('commerce_weavers.tpay.payum.factory.create_blik0_transaction', CreateBlik0TransactionFactory::class)
->alias(CreateTransactionFactoryInterface::class, 'commerce_weavers.tpay.payum.factory.create_blik0_transaction')
Copy link
Member

Choose a reason for hiding this comment

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

👍


trait OrderLastNewPaymentAwareTrait
{
public function getLastNewPayment(): ?PaymentInterface
Copy link
Member

Choose a reason for hiding this comment

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

Misleading naming. Payment on its own has new state and according to name I would expect such a payment.

Suggested change
public function getLastNewPayment(): ?PaymentInterface
public function getLastCartPayment(): ?PaymentInterface

class name should be changed as well

{
$builder->add('others', PaymentDetailsType::class, [
'label' => 'commerce_weavers_sylius_tpay.payment.blik.token',
// TODO some validation that works becuase this kind does not
Copy link
Member

Choose a reason for hiding this comment

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

No todos nor comments should be committed to the codebase. You are more than welcome to open an issue and add it to board

@@ -29,6 +30,14 @@ public function execute($request): void
/** @var PaymentInterface $model */
$model = $request->getModel();

if ($this->transactionIsBlik($model)) {
Copy link
Member

Choose a reason for hiding this comment

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

C'mon, it is widely accepted practice to put is and other verbs at the beginning of functions, please adjust code to informal standard

Suggested change
if ($this->transactionIsBlik($model)) {
if ($this->isBlikTransaction($model)) {

@jakubtobiasz jakubtobiasz mentioned this pull request Sep 13, 2024
3 tasks
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.

Implement handling Blik level 0
3 participants