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

Custom payment method registration (different state object) #3089

Closed
Cheeerd opened this issue Jun 20, 2019 · 8 comments
Closed

Custom payment method registration (different state object) #3089

Cheeerd opened this issue Jun 20, 2019 · 8 comments
Labels
Milestone

Comments

@Cheeerd
Copy link
Contributor

Cheeerd commented Jun 20, 2019

Current behavior

As described in docs/guide/integrations/payment-gateway.md and implemented in cash on delivery module, to register custom payment method we should dispatch payment/addMethod in afterRegistration hook.
Hovewer it appears that state object used on this step is different from one used after in checkout. So added by this way payment methods will be missed.

I think, it was the case why cash on delivery was static defined in node_modules/@vue-storefront/core/modules/checkout/store/payment/index.ts (#3054 )
Also that's why it was enough to me just remove static definition but keep module enabled when i wrote this: #2983

Can you handle fixing this bug by yourself?

I have to follow cashondelivery's static definition example to make custom method appear. But i'm not pretty familiar with architecture to fix the initial issue myself.

@Cheeerd Cheeerd added the bug Bug reports label Jun 20, 2019
@Cheeerd Cheeerd changed the title Custom payment method registration (different context object) Custom payment method registration (different state object) Jun 20, 2019
@Cheeerd
Copy link
Contributor Author

Cheeerd commented Jun 20, 2019

Also this bug can be ignored, if disable payment-backend-methods. Then, there are will no conflict with handling checkout-before-placeOrder event, and payment method code still will be taken from magento api. (Did not test this yet)
And why i faced the problem — because is_server_method property should be overwritten by preregistered methods to prevent payment-backend-methods module from handling before-place-order event, but it does not happen due to missing predefined methods in array. But, again, if i add them to static definition, it starts working.

@pkarw
Copy link
Collaborator

pkarw commented Jun 20, 2019

Hi @Cheeerd thanks for the feedback! I belive it might be related to how “payment-backend-methods” module do insert the server methods it gets from Magento backend. It calls “replaceMethods” so it automatically de-registers all the methods that has been registered from code. It maybe should replace just the methods set by “is_server_method” set to true?

I’m writhing this from my iPhone and can’t check how it exactly works :/

Anyway - that’s fact of the server and code based methods share the same code there will be the conflict and potentially a race condition.

@pkarw
Copy link
Collaborator

pkarw commented Jun 20, 2019

Another idea is that “replaceState” is being used after the “afterRegistration” hook from module was called. In that case the state would be replaced with the one that comes with INITIAL_STATE

This can be checked in the client-entry.ts

@Cheeerd
Copy link
Contributor Author

Cheeerd commented Jun 21, 2019

Another idea is that “replaceState” is being used after the “afterRegistration” hook from module was called. In that case the state would be replaced with the one that comes with INITIAL_STATE

This can be checked in the client-entry.ts

Yes, it is called after "afterRegistration" and replaces state with window.INITIAL_STATE. And i found that if i move payment/addMethod out of !isServer condition, custom methods (and cashondelivery) appear in INITIAL_STATE and start replacing payment-backend-methods.

But there are also another issue with cashondelivery (but not with my custom method): if card is empty (user first time opens the site), the "servercartAfterCreated" action still loads methods from backend, and cashondelivery from backend overrides cashondelivery from module because of "!isVirtualCart || method.code !== 'cashondelivery'" condition. So cashondelivery's info block won't be shown.

And also i wonder, is there are some way without changing core code or disabling payment-backend-methods to keep method's info (title) from magento, but use custom handler when placing order? And do not show custom method if it not available in magento and didn't come with backend methods.

@pkarw
Copy link
Collaborator

pkarw commented Jun 21, 2019

Yeah, the isServer check might do the difference - that's correct. Currently, the modules API has no other hook to register the methods so this might be the best approach (to remove this check). I don't see any downsides of this change.

This is related to #2760 and #3024 - please feel free to share some proposal on payment api changes in the #3024 as well

I think that we might have an config option like config.orders.payment_methods_priority setting if the client side registered payments are higher in the priorities or lower than returned from server side(?) Please also do check this config option to map server method to client method: https://github.com/DivanteLtd/vue-storefront/blob/70543b3cfa379971220f6514348fe0386a108878/docs/guide/basics/configuration.md#L458 it should be probably cashondelivery: cashondelivery mapping in your case - but it will notice VS to use the esxisting / registered method instead of using the payment-backend-methods instead.

Please feel free to test these options and select one of them implementing a PR that will cover the problem you're facing. I belive it will be beneficial for all current and future users as well :)

@pkarw pkarw added this to the 1.10.0 milestone Jun 29, 2019
@pkarw
Copy link
Collaborator

pkarw commented Jun 29, 2019

Related to #3152 - in a way that #3152 is adding another hook after state replace

@pkarw
Copy link
Collaborator

pkarw commented Jul 5, 2019

@Cheeerd have you maybe tried the solution with the payment_methods_mapping?

@Cheeerd
Copy link
Contributor Author

Cheeerd commented Jul 11, 2019

@pkarw Sorry for late response. I tested it with cashondelivery method. It is used only in getPaymentMethod() which is called just in prepareOrder(), after the cashondelivery module emitted "checkout-do-placeOrder" event. So i dont quite understand how its relevant. Because if cashondelivery was missed earlier (did not override backend method due to empty cart check) it wouldnt neither output info block nor emit "checkout-do-placeOrder" event (payment-backend-methods module would do it instead).
image

pkarw added a commit that referenced this issue Jul 16, 2019
This is workaroundn for `cashonndelivery` being removed by `backend-payment-methods` module
pkarw added a commit that referenced this issue Jul 16, 2019
@pkarw pkarw closed this as completed Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants