search

woocommerce / woocommerce-gateway-stripe

The official Stripe Payment Gateway for WooCommerce
https://wordpress.org/plugins/woocommerce-gateway-stripe/
237 stars 207 forks source link

[settings - UPE] Stripe settings persistence - payment methods #1811

Closed waclawjacek closed 3 years ago

waclawjacek commented 3 years ago

Part of #1717.

Add persistence of enabled payment methods to the backend to onboarding wizard and main settings page.

harriswong commented 3 years ago

Using https://github.com/woocommerce/woocommerce-gateway-stripe/pull/1797 as base PR.

harriswong commented 3 years ago

@waclawjacek I want to double check on this. Is this the second step in the onboarding wizard where users click and select the payment method checkboxes?

waclawjacek commented 3 years ago

That'd be the entire onboarding wizard but I am currently finishing up #1718 so I guess it'd be best if you could please wait for that. Sorry for not mentioning it! 😬

harriswong commented 3 years ago

Block by https://github.com/woocommerce/woocommerce-gateway-stripe/pull/1797 and https://github.com/woocommerce/woocommerce-gateway-stripe/pull/1796

c-shultz commented 3 years ago

I see #1797 is done and #1796 has a draft PR, so I'm marking this as unblocked. What do you think @harriswong @frosso ?

harriswong commented 3 years ago

When we were chatting on this 2 days ago (p1631607965091800/1631570172.089800-slack-C01RTUVR81K), the onboarding functionality task (#1796) may have high overlaps with this issue thus is still considered blocked. The thing I want to avoid is code conflicts. If there is other issue with the same weight of priority, we can work on those first until (#1796) is done. But otherwise no concern to unblock it.

frosso commented 3 years ago

Maybe the persistence of the checkboxes on the payment methods (and only the payment methods, nothing else) can be handled as a separate PR first, so that both this ticket and the onboarding persistence can work separately by leveraging the same mechanism? The functionality to persist that data seems similar on the controller/UI side, so in either ticket it needs to be addressed. I'm thinking maybe a separate PR would help reduce the conflicts on both 🤷

harriswong commented 3 years ago

I want to confirm if i am understanding #1718 and #1811 correctly.

For #1718, we save settings in local react/redux states (p1631294516063400/1631290211.056400-slack-C01RTUVR81K). This ticket cover all 3 steps in the onboarding, including "Add payment methods" button in step 2. None of these are actually saved in the DB.

For #1811 (this issue), we are to use #1718 so that in step 2, the payment methods are actually saved in the DB.

Maybe the persistence of the checkboxes on the payment methods (and only the payment methods, nothing else) can be handled as a separate PR first

image In step 2, we have these checkboxes. Are you suggesting to repurpose this issue to build out the hooks that save the data in the DB, but not the UI part? I think that will minimize overlaps since the data layer is separated from the local states/UI component parts.

frosso commented 3 years ago

Sorry for the confusion, I should have added some links to help clarify too! :D

There are 3 tickets related to this discussion: https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1811 , https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1812 , https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1718 .

All 3 need to manage the payment methods list. Maybe https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1811 can have a PR with some hooks that persists the data for the checked payment methods (and could demonstrate this functionality on either the onboarding wizard or the settings screen? Maybe demonstrating it on the onboarding wizard would be best, since that seems easier and it would completely address all the requirements in https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1811 ). That would help with the completion of https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1718 and the remainder of all the other settings in https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1812 .

In step 2, we have these checkboxes. Are you suggesting to repurpose this issue to build out the hooks that save the data in the DB, but not the UI part? I think that will minimize overlaps since the data layer is separated from the local states/UI component parts.

I think this aligns with my suggestion as well :)

It looks like all the PRs related to these changes already have a decent amount of conflicts, so in the meantime maybe just tackling the persistence of the enabled payment methods data though https://github.com/woocommerce/woocommerce-gateway-stripe/issues/1811 would help 👍