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

ACP-3507 APIs to enable projects embed payment elements to frontends based on Glue API #15

Conversation

stereomon
Copy link
Contributor

@stereomon stereomon commented Aug 6, 2024

Release Table

Module Release Type Constraint Updates
AppPayment minor

Module AppPayment

Change log

Improvements

  • Added new PaymentMethod handling. Payment methods are now persisted.
  • Added PreOrder Payment with creation, confirmation, and cancellation.

Copy link
Contributor

@matweew matweew left a comment

Choose a reason for hiding this comment

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

.

composer.json Outdated Show resolved Hide resolved
protected function addPaymentMethod(PaymentMethodTransfer $paymentMethodTransfer, AppConfigTransfer $appConfigTransfer): void
{
// Add the passed configuration to the default configuration.
$paymentMethodAppConfigurationTransfer = $this->getDefaultPaymentMethodAppConfiguration();
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks "too hidden" from the app developer.
so a developer even don't understand that default API URLs (/private) will be sent to SCOS, even no ability to define/overwrite them from the plaform plugin.

What if the app developer should use this method explicitly inside the plugin and have the ability to overwrite URLs?
e.g. via AppPaymentConfig->getDefaultPaymentMethodAppConfiguration().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's implement it when it gets requested. The point of this is that these packages provide these endpoints so a developer does not need to implement or even know them.

Documentation will help here. I added a more precise one for devs to the README.md

Copy link
Contributor

Choose a reason for hiding this comment

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

My main concern is as an app developer I cannot change URLs... Maybe it won't be used, but it makes me think that everything is hardcoded in the package and to change it the only way is overwrite methods :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are the developers and if we see the need for being able to change we can implement.

@matweew matweew changed the title Feature/acp 3507 investigate how the apis to enable projects embed payment elements to yves or in headless scenario should be built ACP-3507 APIs to enable projects embed payment elements to frontends based on Glue API Sep 23, 2024
…jects-embed-payment-elements-to-yves-or-in-headless-scenario-should-be-built' of https://github.com/spryker/app-payment into feature/acp-3507-investigate-how-the-apis-to-enable-projects-embed-payment-elements-to-yves-or-in-headless-scenario-should-be-built
Copy link

codecov bot commented Sep 26, 2024

Codecov Report

Attention: Patch coverage is 96.19048% with 16 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@c56ff8a). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ment/Business/Payment/Transfer/PaymentTransfer.php 10.00% 9 Missing ⚠️
...pPayment/Business/Payment/Method/PaymentMethod.php 97.69% 3 Missing ⚠️
...Zed/AppPayment/Business/Message/MessageBuilder.php 0.00% 2 Missing ⚠️
...ryker/Zed/AppPayment/Business/AppPaymentFacade.php 90.00% 1 Missing ⚠️
...ed/AppPayment/Persistence/AppPaymentRepository.php 96.42% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master      #15   +/-   ##
=========================================
  Coverage          ?   98.86%           
  Complexity        ?      403           
=========================================
  Files             ?       61           
  Lines             ?     1678           
  Branches          ?        0           
=========================================
  Hits              ?     1659           
  Misses            ?       19           
  Partials          ?        0           
Flag Coverage Δ
98.86% <96.19%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…jects-embed-payment-elements-to-yves-or-in-headless-scenario-should-be-built' of github.com:spryker/app-payment into feature/acp-3507-investigate-how-the-apis-to-enable-projects-embed-payment-elements-to-yves-or-in-headless-scenario-should-be-built
@stereomon stereomon merged commit fd7b7d9 into master Sep 30, 2024
2 checks passed
@stereomon stereomon deleted the feature/acp-3507-investigate-how-the-apis-to-enable-projects-embed-payment-elements-to-yves-or-in-headless-scenario-should-be-built branch September 30, 2024 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants