-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update Onbarding steps for PayPal #1491
Conversation
Size Change: +128 B (+0.06%) Total Size: 201 kB
ℹ️ View Unchanged
|
Hi @ilyasfoo , @moon0326 , I have created this PR based on #1426 but I have updated the testing instructions a bit as I was not able to reproduce them exactly. I have removed the Could you please review the PR and let me know any comments? Please note that I am not very familiar with this codebase so be thorough in your review 😉 Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @epeicher 👍
I think we should make a shared class for remove_woo_payments_from_payments_suggestions_feed
and remove_payments_note
functions when we get more payment tasks later on.
Other than that, LGTM and tested well 🚀
Thank you, @moon0326 for reviewing.
Agree! We can work on this on a follow-up task. |
*/ | ||
public function __construct() { | ||
// Only for free trials. | ||
if ( ! wc_calypso_bridge_is_ecommerce_trial_plan() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should avoid using the wc_calypso_bridge_is_ecommerce_trial_plan
condition. In the future the plan provisioned might not be a trial. Also, the behaviour should only be dictated by the $onboarding_profile['partner']
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a key point to consider. We included that condition to avoid diverging from the original implementation, but I soon realized that it would need an update if the creating flow changes at any point, which is indeed very likely. And I totally agree with you that it should only depend on the onboarding_profile
option.
What do you think about considering this when updating the process to be more dynamic, context here: p1721665079790669-slack-C02JPCHEKDY? Or do you think that we should instead remove this condition before shipping this iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do it in a clean up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Task created to avoid forgetting this:
Changes proposed in this Pull Request:
Closes https://github.com/Automattic/dotcom-forge/issues/8277
This PR adds
Get paid with PayPal
task and removes Woo Payments task when the optionwoocommerce_onboarding_profile.partner
ispaypal
How to test the changes in this Pull Request:
Get paid with PayPal task
woocommerce_onboarding_profile
option using the following commandwp option patch insert woocommerce_onboarding_profile partner paypal
(orupdate
if thepartner
property exists).<your-domain>
with the domain of your host. Or you can use the following instructions to synchronize thewc-calypso-bridge
plugin to the atomic site: Pe5pgL-2eX-p2WooCommerce -> Home
Get paid with PayPal
task exists.Other information:
FOR PR REVIEWER ONLY: