-
Notifications
You must be signed in to change notification settings - Fork 114
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
Google Ads: Add eligibility check for campaign creation #13192
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
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.
This looks good to me and is ready to ship. 🚢
I left a couple of non-blocking questions.
self.featureFlagService = featureFlagService | ||
} | ||
|
||
func isSiteEligible(siteID: Int64) async -> 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.
❓ Should we mark this as MainActor
? Also, I think marking the protocol method as MainActor
doesn't really check the MainActor
conformance here.
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.
@selanthiraiyan Actually marking the protocol method with @MainActor
ensures that all conformance method are on the main actor. I learned this when attempting to do the migration for Swift 6, so I figured it's better to do it correctly now than to fix it later in the migration process.
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.
Actually marking the protocol method with @mainactor ensures that all conformance method are on the main actor.
That is great. TIL. Thanks.
|
||
let isPluginSatisfied: Bool = await { | ||
if let savedPlugin = await fetchPluginFromStorage(siteID: siteID) { | ||
return checkIfGoogleAdsIsSupported(plugin: savedPlugin) |
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.
❓ Should we fetch the plugin from remote in case the plugin version has been updated recently and it isn't synched to storage yet?
This is an edge case considering that we fetch plugin info upon app launch.
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.
@selanthiraiyan do you mean we should always fetch from remote rather than relying on storage?
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.
do you mean we should always fetch from remote rather than relying on storage?
We can fetch from remote only when the local plugin version doesn't match the required pluginMinimumVersion
. This covers the case in which the user has updated the plugin version recently but we haven't fetched the new version details. This is an edge case and please feel free to ignore it.
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 it is safer to always fetch plugins from remote to avoid edge cases. Updated in 5f0f3ea. Thanks Sharma for the suggestion!
Closes: #13190
Description
This PR adds an eligibility checker to decide whether to display the entry point to Google ads campaign creation on the app. The logic includes:
googleAdsCampaignCreationOnWebView
to ensure that it's enabled.Testing steps
The eligibility check has not been used anywhere, so just unit tests passing is sufficient.
Screenshots
N/A
RELEASE-NOTES.txt
if necessary.