Multiple UPE - Duplicate notices when saving SEPA methods

[mdmoore]

Describe the bug

When adding SEPA payment methods via My Account > Payment Methods > Add Payment Method success notices are duplicated several times.

To Reproduce

1. On the poc/upe-instances-multiplied branch, enable SEPA Direct Debit
2. Navigate to My Account > Payment Methods > Add Payment Method
3. Add a SEPA payment method

Acceptance Criteria

• Adding a SEPA payment method results in only a single success notice
• Changes made should be merged into poc/upe-instances-multiplied, not develop.
• Callback functions are moved to the WC_Payments_UPE_Checkout class and are added to the hook only once.

Technical refinement

Please see the discussion below starting from this comment, in order to get an idea of how to achieve the goal of this task.

[mdmoore]

Hey team! Please add your planning poker estimate with Zenhub @FangedParakeet @timur27

[FangedParakeet]

I might have a few theories here...

@mdmoore, does this occur when adding the same, standard SEPA testing account (AT611904300234573201) repeatedly or does this still occur if you try any of the other payment methods listed here?

[mdmoore]

@FangedParakeet Your theories are all welcome!

does this occur when adding the same, standard SEPA testing account (AT611904300234573201) repeatedly or does this still occur if you try any of the other payment methods listed here?

The same is occurring with other test accounts as well. I've checked with AT321904300235473204 and AT861904300235473202. I'm getting 10 notices each time, regardless of the number of payment methods I have.

[timur27]

Did a quick investigation - this hook's callback is being fired N times, where N = size of $upe_payment_gateway_map which depends of the number of payment methods.

Since we now have the WC_Payments_UPE_Checkout class, moving add_action( 'wp', [ $this->gateway, 'maybe_process_upe_redirect' ] ); from the upe-gateway class to the constructor of the checkout class resolves the issue.

Note: With @FangedParakeet we discussed moving callbacks initialization from the gateway to checkout in this PR I guess and problems that appeared when I tried to accomplish this. I think it has something to do with the static callback defined in a different class. I tried to test the static callback function back then since my use case was to clear the cache and it didn't work (obviously, I changed __CLASS__ to UPE_Payment_Gateway::class and tried some other tricks). Looks like non-static methods can be moved easily, but they need to be tested.

I think it would be great to move all the hooks and test them all within this task. Since the fix for this particular issue is a one-liner and we know the place in code where it should be applied, I'd give this issue a 1. But, if we want to include moving all the hooks to the checkout class and testing them within this issue, I'd give this issue a 2 then.

Having all this in mind, @mdmoore @FangedParakeet please let me know, which scope you prefer to keep in this issue (move all the hooks, or just the one related to the issue) and please let me know if you would estimate differently or not after this comment.

[FangedParakeet]

Did a quick investigation - this hook's callback is being fired N times, where N = size of $upe_payment_gateway_map which depends of the number of payment methods.

Ooh, nice catch! Though, actually now that I look over that code a little closer, won't all of those hooks be defined N times, since in the has_action check, $this will be different for each registered payment gateway? That seems unintended to me, if true.

I think it would be great to move all the hooks and test them all within this task.

This seems like a better long-term solution for now. @timur27, do you have any idea why this was unsuccessful when you attempted it in that merge PR?

In terms of weighting the points here, I think the lowest I would go to is a 2. It seems like there are still a few uncertainties and things that could go wrong with moving the hooks to the checkout class, since apparently this exploded the last time we tried it. That being said, I think we've done a bit more of a detective work here already, so perhaps this isn't a 3 anymore. Thoughts?

[timur27]

Though, actually now that I look over that code a little closer, won't all of those hooks be defined N times, since in the has_action check, $this will be different for each registered payment gateway?

Yes. Callback functions in PHP are uniquely hooked to the filter per class instance, which makes sense. I found that out today because of your question - many thanks for asking, as I learned something new today!

do you have any idea why this was unsuccessful when you attempted it in that merge PR?

I worked with the class' static method only, and I think now that it didn't work because instead of

add_action( 'woocommerce_order_payment_status_changed', 'UPE_Payment_Gateway::remove_upe_payment_intent_from_session', 10, 0 );

OR

add_action( 'woocommerce_order_payment_status_changed', ['UPE_Payment_Gateway', 'remove_upe_payment_intent_from_session'], 10, 0 );

I passed the callback function like this:

add_action( 'woocommerce_order_payment_status_changed', [ UPE_Payment_Gateway::class, 'remove_upe_payment_intent_from_session' ], 10, 0 );

The first two options are the right ways to pass the static class method, according to the documentation. I would expect things to work after trying the correct approach.

Also, I am optimistic about moving the non-static methods group to the WC_Payments_UPE_Checkout class - the only thing needed there would be to find the use cases that will allow us to verify that the function exists in a single instance. In my personal opinion, this step requires the most effort.

I am okay with the 2 SP estimation here.

[mdmoore]

Thanks for all of the research @timur27! I'm fine with widening the scope of this issue to include moving all of the hooks. Given your research and experience with the issue so far, 2 sounds good to me as well. Please feel free to make any edits to the description of the issue.

[FangedParakeet]

Also, I am optimistic about moving the non-static methods group to the WC_Payments_UPE_Checkout class - the only thing needed there would be to find the use cases that will allow us to verify that the function exists in a single instance.

This sounds like a cleaner option. You could always just create a private method inside WC_Payments_UPE_Checkout that passes on the call to the static functions in UPE_Payment_Gateway, though this might be a little redundant. Alternatlively, you could probably even move the static functions over to the Checkout class as well, if you really wanted to. Though, that would be a larger refactor, taking into account all the places those functions are used, and wouldn't leave too much left in the main gateway class--not that that's necessarily a bad thing.

And cool, 2 points sounds fine 2 me 2. 👍