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

De-couple PaymentMethod from PaymentMethodHandler #671

Closed
michaelbromley opened this issue Jan 26, 2021 · 3 comments
Closed

De-couple PaymentMethod from PaymentMethodHandler #671

michaelbromley opened this issue Jan 26, 2021 · 3 comments

Comments

@michaelbromley
Copy link
Member

michaelbromley commented Jan 26, 2021

The PaymentMethod entity has a "code" field which is supposed to refer to one of the configured PaymentMethodHandlers. However, in the GraphQL API updatePaymentMethod mutation, as well as in the Admin UI, this "code" can be edited.

There seems to be no good reason to allow setting this code. Rather, it would make more sense to de-couple the handler and the method, so that you can create a PaymentMethod and then select from one of the configured handlers.

Additionally, the PaymentMethod should have a "name" and "description" field.

Also add some e2e tests for the PaymentMethodResolver.

@michaelbromley
Copy link
Member Author

While working on this, also handle these related issues: #469, #662

michaelbromley added a commit that referenced this issue Jan 29, 2021
Relates to #671

BREAKING CHANGE: The PaymentMethod entity and type has changed. Previously, a PaymentMethod was
coupled to the configured PaymentMethodHandlers 1-to-1. Now the PaymentMethodHandler is just
a configurable _property_ of the PaymentMethod, much in the same way that a ShippingCalculator
relates to a ShippingMethod. Any existing PaymentMethod entities will need to be migrated to the
new structure.
@stefanvanherwijnen
Copy link
Contributor

stefanvanherwijnen commented Feb 26, 2021

Could you also add the name and description on the returned eligibleShippingMethods?

results.push({
id: method.id,
code: method.code,
isEligible,
eligibilityMessage,
});

Also, if I understand correctly, code is deprecated and could be removed?

Edit: addPaymentToOrder also requires the code as input, I think this should also be either the name or id?

@michaelbromley
Copy link
Member Author

Could you also add the name and description on the returned eligibleShippingMethods?

@stefanvanherwijnen could you please open a new issue for this so I don't forget about it?

Also, if I understand correctly, code is deprecated and could be removed?

The code property is still needed in the addPaymentToOrder() as you pointed out. True, it might be preferable to use the id instead now, as it is more explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants