Consolidate merchant details before and after onboarding (5311) #3674
Conversation
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. 🔗 Test this pull request with WordPress Playground What's included:
Login credentials:
Plugin Details:
🤖 Auto-generated for commit d79983c • Last updated: 2025-10-09T12:19:11.942Z |
Dinamiko
requested review from
AlexP11223,
Narek13,
danieldudzic,
hmouhtar,
puntope,
stracker-phil and
strangerkir
There was a problem hiding this comment.
Should we integrate those checks into the MerchantDetails class for a more consistent API?
Generally, the utility would be better placed in the wc-gateway module, instead of the settings module
| $can_use_subscriptions = $container->has('wc-subscriptions.helper') && $container->get('wc-subscriptions.helper')->plugin_is_active(); | ||
| $should_skip_payment_methods = class_exists('\WC_Payments'); | ||
| $can_use_fastlane = $container->get('axo.eligible'); | ||
| $can_use_pay_later = $container->get('button.helper.messages-apply'); |
There was a problem hiding this comment.
Suggestion: Is it useful to repeat the full service code here? Looks like a lot of maintenance to keep this doc in-sync with the code + does not add a huge benefit to the documentation.
A permalink would be sufficient IMO (which is usually rendered as embed in GH)
File: modules/ppcp-settings/services.php
woocommerce-paypal-payments/modules/ppcp-settings/services.php
Lines 85 to 104 in 2355409
|
|
||
| #### Flag Value Sources | ||
|
|
||
| **`can_use_casual_selling`** - Personal account support |
There was a problem hiding this comment.
Suggestion: To me this looks a bit like code documentation that should be present in the .php file.
Also, the service name is already visible in the code snippet above and describing the implementation logic here seems risky to get outdated as well.
I would keep this flag overview more concise, documenting the "business perspective", e.g.
can_use_casual_selling- Business accounts are supported in all countries; this flag indicates if the store country also supports onboarding using a personal accountcan_use_vaulting- Whether the store's country supports vaulting - displays additional onboarding choicesshould_skip_payment_methods- Whether the "branded only" mode is active and the onboarding wizard is reduced- ...
| $applies = $container->get('module.helpers.applies'); | ||
| return $applies->for_country() && $applies->for_merchant(); | ||
| }, | ||
| ``` |
There was a problem hiding this comment.
Suggestion: The whole "Country-Based Eligibility Services" section seems unrelated to the document topic - which should be about the "Onboarding Flow" - as it documents generic architecture instead of explaining onboarding details.
It's a nice idea to document code architecture, but let's move this to a different document. This section is confusing for new developers, especially since the code looks valid but actually displays pseudo-code (there is no service named module.helpers.applies, but specific services like applepay.helpers.apm-applies, which is not mentioned here)
|
|
||
| #### Casual Selling Country List | ||
|
|
||
| The casual selling feature (personal PayPal accounts) supported countries are defined in: `modules/ppcp-settings/services.php` |
There was a problem hiding this comment.
Suggestion: Remove this, as code navigation within a single service file should be easy enough for every developer, and this description adds no value when reading the document in isolation. IMO this list/reference is also more related to a documentation about "Casual Seller vs Business Merchants"
| 'accept_card_payments' => bool // Whether to enable card payment methods | ||
| 'products' => array // Selected product types ['virtual', 'physical', 'subscriptions'] | ||
| 'gateways_synced' => bool // Payment gateway sync status | ||
| 'gateways_refreshed' => bool // Payment gateway refresh status |
There was a problem hiding this comment.
Suggestion: Could we add those code comments to the actual .php file, e.g. to the OnboardingProfile and drop a link here? Those comments are very helpful, but developers usually do not see them in this MD file
| - Final step provides connection buttons/forms | ||
| - Connection success updates onboarding completion status | ||
| - Failed connections allow retry without losing progress | ||
|
|
There was a problem hiding this comment.
Suggestion: Add a link to the authentication-flows.md document
| return $this->environment->is_sandbox(); | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Suggestion: Instead of documenting class code, IMO it would be beneficial to document the service name + the API methods (public methods) - but actually this section here is already 100% covered by the doc-block comment in the .php file and only duplicates content.
A link-list to all relevant classes would give the reader a better overview of what's involved in the post-onboarding process.
| 'merchantId' => '[email protected]', // Merchant's PayPal email address | ||
| 'accountStatus' => 'BUSINESS_ACCOUNT', // Account type indicator | ||
| 'ppcpToken' => 'VALIDATION_TOKEN' // Security token | ||
| ``` |
There was a problem hiding this comment.
Suggestion: Thinking about the OAuth Response, I would rather add those details to the existing "authentication-flows.md" document, as it's tightly related to the authentication, and not post-onboarding
Shouldn't the "Post Onboarding" mainly describe how the merchant is initialized in the plugin, and talking about the SettingsDataManager flow?
| return false; | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Suggestion: Merge this class with the existing MerchantDetails class, which lives in the ppcp-wc-gateway module and already provides related capability checks
| $this->dcc_product_status = $dcc_product_status; | ||
| } | ||
|
|
||
| public function can_save_paypal_methods(): bool { |
There was a problem hiding this comment.
Suggestion: I highly recommend to add doc-blocks to these methods and explain what they check. I.e. that this capability is checked using the PayPal API response
|
|
||
| This dual-eligibility system ensures that merchants can offer the most appropriate save payment options based on their PayPal account capabilities and regional availability. | ||
|
|
||
| ## Centralized Capability Management |
There was a problem hiding this comment.
Suggestion: Capability detection is a central business logic of the plugin and deserves its own MD file :)
Dinamiko
changed the title
| 'woocommerce-paypal-payments' | ||
| ), | ||
| 'enabled' => $this->merchant_capabilities['pay_later'] && ! $save_paypal_and_venmo, | ||
| 'enabled' => $this->merchant_capabilities['paylater'], |
There was a problem hiding this comment.
Was ! $save_paypal_and_venmo removed intentionally? Just want to make sure it’s not an accidental change.
There was a problem hiding this comment.
Was ! $save_paypal_and_venmo removed intentionally?
yes. The reason is that this check checks if the setting is active. That setting is correctly blocking The payment method in the payment method page. But that doesn't mean we need to disable pay_later feature.
But now I check again maybe we want to...
Lets discuss tomorrow. I added it back for now.
4 participants
This PR consolidates Merchant details before and after onboarding, including deprecating/removing duplicated logic.
It also adds two new documents explaining how merchant onboarding and post-onboarding works.
Post onboarding document also explains how vaulting (save payment methods) availability is currently implemented.