-
Notifications
You must be signed in to change notification settings - Fork 121
[POS as a tab i2] Support loading WC plugin and POS feature switch from system status endpoint #15906
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
[POS as a tab i2] Support loading WC plugin and POS feature switch from system status endpoint #15906
Conversation
…response, and refactor existing remote methods to use this.
…tch value from one system status API request.
|
|
iamgabrielma
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.
LGTM 🚀
| do { | ||
| let mapper = SystemStatusMapper(siteID: siteID) | ||
| let systemStatus = try await loadSystemStatus(for: siteID, | ||
| fields: [Field.environment, Field.activePlugins, Field.inactivePlugins], |
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.
Currently unused, but I think we're missing the settings field here?
| fields: [Field.environment, Field.activePlugins, Field.inactivePlugins], | |
| fields: [Field.environment, Field.activePlugins, Field.inactivePlugins, Field.settings], |
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.
SystemStatusMapper/SystemStatus currently does not need the settings field for the use case of this remote method (as in the fields in the deleted line 21), thus I don't think we need to fetch it for performance reason.
|
|
||
| // Finds WooCommerce plugin from active plugins response. | ||
| guard let wcPlugin = systemStatus.activePlugins.first(where: { $0.plugin == Constants.wcPluginPath }) else { | ||
| return POSPluginAndFeatureInfo(wcPlugin: nil, featureValue: nil) |
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.
If no WooCommerce plugin path is found at this point of execution, should we throw here rather than returning a POSPluginAndFeatureInfo with nil values?
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.
Good question - I thought about it before, and decided to keep this service simple and just return data if available and throw errors when the app fails to retrieve the data. The use case determines how to handle when the plugin / feature value isn't available.
Ideally, all the plugin related checks can be in Yosemite. However, right now VersionHelpers exists in the app layer and it'd require some refactoring to move it to Yosemite. If I have some time before the i2 release next Friday, I will tackle this.
| // As `settings.enable_features` was introduced in WC version 9.9.0, this field is optional. | ||
| // Ref: https://github.com/woocommerce/woocommerce/pull/57168 |
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 💯
| @Test func loadWooCommercePluginAndPOSFeatureSwitch_returns_nil_plugin_and_nil_feature_when_woocommerce_plugin_not_found() async throws { | ||
| // Given | ||
| network.simulateResponse(requestUrlSuffix: "system_status", filename: "system-status-wc-plugin-missing") | ||
|
|
||
| // When | ||
| let result = try await sut.loadWooCommercePluginAndPOSFeatureSwitch(siteID: sampleSiteID) | ||
|
|
||
| // Then | ||
| #expect(result.wcPlugin == nil) | ||
| #expect(result.featureValue == nil) | ||
| } |
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.
Linked to the comment above regarding not finding the WooCommerce path at this point, is this a expected scenario? Or should we throw an error?
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.
Responded in #15906 (comment).

For WOOMOB-835
Just one review is required.
Why
This PR adds Yosemite level support for retrying plugin related checks in the POS eligibility check WOOMOB-799. The integration with the eligibility checker will be in a separate PR because this PR is already close to 500 diffs.
Description
This PR introduces
POSSystemStatusServicein Yosemite to efficiently load both WooCommerce plugin information and POS feature switch value from a single system status API request. This improves performance by reducing API calls when refreshing POS eligibility.The implementation includes:
POSSystemStatusServiceProtocolwithloadWooCommercePluginAndPOSFeatureSwitchmethod withPOSSystemStatusServiceimplementationloadSystemStatusmethod inSystemStatusRemotefor reusable system status requestsPOSSystemStatusServiceSteps to reproduce
The new
POSSystemStatusServiceisn't used in the app yet. Some confidence testing on the refactoredSystemStatusRemotemethods would be helpful:SystemStatusRemote.loadSystemInformationusageThis method is called throughout the app via
SystemStatusAction.synchronizeSystemInformation. Example testing steps:SystemStatusRemote.fetchSystemStatusReportusageThis method is called throughout the app via
SystemStatusAction.fetchSystemStatusReport.Testing information
I tested the steps above in iPad A16 iOS 18.4 simulator.
RELEASE-NOTES.txtif necessary.