-
Notifications
You must be signed in to change notification settings - Fork 21
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
API Pull - Grant access to Google WPCOM app in onboarding flow #2337
base: feature/google-api-project-phase-2
Are you sure you want to change the base?
API Pull - Grant access to Google WPCOM app in onboarding flow #2337
Conversation
…roduct sync button
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project-phase-2 #2337 +/- ##
====================================================================
Coverage ? 62.8%
Complexity ? 4481
====================================================================
Files ? 765
Lines ? 22616
Branches ? 543
====================================================================
Hits ? 14195
Misses ? 7964
Partials ? 457
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…-app-in-setting-page' into add/grant-access-to-google-wpcom-app-in-onboarding-flow
If no `step` query param provided, use the step from MC setup.
…setup mc" This reverts commit 712f38c.
Note that this step shares the same key with the step `accounts` defined in `js/src/setup-mc/setup-stepper/stepNameKeyMap.js`, so when this step hasn't finished, the UI will stay on the setup accounts page.
…ogle-wpcom-app-in-onboarding-flow
…ogle-wpcom-app-in-onboarding-flow
Because we wouldn't want the merchant to disable it during onboarding. Also remove hideNotificationService since we will always show it in both onboarding and settings page.
…ogle-wpcom-app-in-onboarding-flow
Just a note here: If this is for next version I guess we should create a separate branch than |
❓ Doesn't matter if the grant access gets an error or is approved. You can always continue the setup. I assume this is the expected behaviour |
Wonder if we might add some events/tracks regarding the final setup. (ie. If the auth is granted) |
What about adding some tests for the UI as well? |
@@ -40,12 +40,12 @@ import { getSettingsUrl } from '.~/utils/urls'; | |||
* @param {Object} props React props. | |||
* @param {{ id: number }} props.googleMCAccount A data payload object containing the user's Google Merchant Center account ID. | |||
* @param {boolean} [props.hideAccountSwitch=false] Indicate whether hide the account switch block at the card footer. | |||
* @param {boolean} [props.hideNotificationService=true] Indicate whether hide the enable Notification service block at the card footer. | |||
* @param {boolean} [props.hideDisableProductFetch=true] Indicate whether hide the disable product fetch block at the card footer. |
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 guess this should be [props.hideDisableProductFetch=false]
import useApiFetchCallback from '.~/hooks/useApiFetchCallback'; | ||
import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices'; | ||
|
||
const useRestAPIAuthURLRedirect = () => { |
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.
Maybe we can add some JSDOCS for this Hook?
|
||
if ( $this->is_mc_contact_information_setup() && $this->checked_pre_launch_checklist() ) { | ||
$step = 'paid_ads'; | ||
if ( ! $notifications_enabled || $this->is_rest_api_granted() ) { |
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.
instead of $this->is_rest_api_granted()
we might use NotificationsService::is_ready()
that does the same so we can avoid adding more code doing the same.
…ss-to-google-wpcom-app-in-onboarding-flow
Changes proposed in this Pull Request:
Project: pcTzPl-1SM-p2
Figma: JjJwsuOziVFv6eP1HNFhU8-fi-18069-355
Part of #2146 and similar to #2311. This PR adds the granting access to Google WPCOM app flow in the onboarding flow.
What has been changed:
grant_rest_api_access
. Like the stepaccounts
, its index is also1
as we want the UI stays on the set up accounts page when the stepgrant_rest_api_access
is still incomplete.woocommerce_gla_notifications_enabled
to true)hideNotificationService
in the componentConnectedGoogleMCAccountCard
as we would always show it on both settings and onboarding pages.hideDisableProductFetch
in the componentConnectedGoogleMCAccountCard
as we are not showingDisable product data fetch
in the onboarding flow when the granting access is approved.Screenshots:
Detailed test instructions:
Approved
woocommerce_gla_notifications_enabled
totrue
gla_wpcom_rest_api_status
is not set/wp-admin/admin.php?page=wc-admin&path=/google/setup-mc
google_wpcom_app_status=approved
. E.g./wp-admin/admin.php?page=wc-admin&path=/google/setup-mc&google_wpcom_app_status=approved
Google has been granted access to fetch your product data.
Disable product data fetch
like that in the settings page. API Pull - Grant access to google WPCOM app in settings page #2311Disapproved or Error
DELETE /wc/gla/mc/connection
, and delete the optiongla_wpcom_rest_api_status
google_wpcom_app_status=error
orgoogle_wpcom_app_status=disapproved
. E.g./wp-admin/admin.php?page=wc-admin&path=/google/setup-mc&google_wpcom_app_status=disapproved
There was an issue granting access to Google for fetching your products.
Grant access
will redirect to WPCOM app granting access pageNotifications Service is not Enabled
woocommerce_gla_notifications_enabled
tofalse
gla_wpcom_rest_api_status
gla_wpcom_rest_api_status
toapproved
,disapproved
, orerror
Additional details
Verifying who can set the REST API status by setting a query parameter
google_wpcom_app_status
will be implemented in another PR.Changelog entry