-
Notifications
You must be signed in to change notification settings - Fork 120
[Mobile Payments] Use active (not networkActivated) to determine plugin activation state #5341
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
Conversation
… active (not networkActivated)
| /// | ||
| let activePlugins = systemStatus.activePlugins.map { | ||
| $0.overrideNetworkActivated(isNetworkActivated: true) | ||
| $0.copy(active: true) |
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.
Props @ctarda
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can trigger an installable build for these changes by visiting CircleCI here. |
jaclync
left a comment
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.
Looks good! ![]()
Just wanted to clarify 2 things from testing:
- Sign into the app as a shop manager for that store
- Navigate to settings (gear), In-Person Payments
- Ensure that you are asked to install the plugin
- Install, but do not activate the WooCommerce Payments plugin
As a shop manager, I wasn't able to manage plugins. The step to install WCPay plugin has to be performed by another admin account right?
- Tap on Refresh After Activating
- Ensure that you can now get to the Buy/Manage/Read view for card readers
I saw this screen instead, probably because I haven't finished setting up WCPay with Stripe and all that?
|
|
||
| /// Is the plugin active, i.e. false | true | ||
| /// | ||
| public let active: Bool |
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.
nit: just wondering, "active" and "activated" are interchangeable in the WP plugins context right? I'm just more used to "activated" as in wp-admin. After seeing networkActivated above, I was expecting this to be activated but it's very minor 🙂
TIL'ed "network active plugins" 😄
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.
IKR?! I went back and forth on active vs activated. I ultimately picked active because that is the term used in the JSON in the system status response (inactive_plugins, active_plugins)
woocommerce-ios/Networking/Networking/Model/SystemStatus.swift
Lines 32 to 33 in 15f88d6
| case activePlugins = "active_plugins" | |
| case inactivePlugins = "inactive_plugins" |
|
Thanks for the review @jaclync !
That's correct.
Yep! That's also correct! |
Closes #5269
@joshheald @jaclync - if either of you could take a look, that would be great - just one of y'all are needed of course, but both of y'all are welcome
This PR undoes the "hack" we introduced in 7.8 ( #5259) to use networkActivated for storing the active state (we did this to avoid a core data change during the 7.8 code freeze) and leverages the core data work done in #5279
To test:
Uninstall the WooCommerce Payments plugin if installed on your store
Sign into the app as a shop manager for that store
Navigate to settings (gear), In-Person Payments
Ensure that you are asked to install the plugin
Install, but do not activate the WooCommerce Payments plugin
Tap on Refresh After Installing
Ensure that you are asked to activate the plugin
Activate the plugin
Tap on Refresh After Activating
Ensure that you can now get to the Buy/Manage/Read view for card readers
Also ensure unit tests pass. Unit tests for SystemStatusMapper and CardPresentPaymentsOnboardingUseCase were updated.
Update release notes:
RELEASE-NOTES.txtif necessary.