Skip to content
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

Add the Google Site Verification process #139

Merged
merged 17 commits into from
Jan 27, 2021
Merged

Conversation

layoutd
Copy link
Contributor

@layoutd layoutd commented Jan 22, 2021

Changes proposed in this Pull Request:

This PR provides a REST endpoint to perform site verification using Google's Site Verification API.

Works towards #9 (and is a stepping stone towards #10).

  • A POST request to the WP REST endpoint site/verify performs the full verification process:
    • Google API getToken retrieves the <meta> tag to be added to the site ''
    • insert the <meta> tag into the site `
    • Google API insert which instructs Google to check for the <meta> tag
    • disable display of the <meta> tag once the site is verified.
  • A GET request to the same endpoint simply returns the current verification status.
  • Creates a [slug]_site_verification option in wp_option that stores the Google-generated meta_tag, and whether the site has been verified so far.
  • There's a SiteVerification service that displays the <meta> tag when it is found in the DB option, and the site isn't marked as verified (so it should only actually be displayed for the < 1 second between the Site Verification API getToken and insert calls).
  • The Site Verification API passes through the Google Proxy on the WooCommerce Connect Server (needs to be the latest version of the Woogle WCS with the google-sv proxy endpoint included - PR 1692)
  • There's a button to Perform Site Verification in the ConnectionTest page, which will only work on live sites as it uses the Site Verification API insert method to instruct Google to verify the site (so it must be web-accessible). Tested with ngrok.
  • Tracks events are recorded for successful or failed verification attempts. Failures include the step that failed (token retrieval or meta-tag verification).

Notes

  • Uses site_url() to specify the site to verify.
  • Doesn't contemplate unlinking a site.
  • If already verified, the WP endpoint returns a success message.
  • May need to brush up WP REST responses (add status codes, etc).

Detailed test instructions:

  1. Using the Perform Site Verification button on the ConnectionTest page.
  2. Using the site/verify endpoint in the REST API.

@layoutd layoutd self-assigned this Jan 22, 2021
@layoutd layoutd force-pushed the feature/site-verification branch from f855d6c to d5efef2 Compare January 22, 2021 12:38
@layoutd layoutd changed the title WIP Add the Google Site Verification process Add the Google Site Verification process Jan 22, 2021
@layoutd layoutd requested review from JPry and mikkamp January 22, 2021 14:52
Copy link
Member

@jconroy jconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice @layoutd !

Tested a live connection (via the middleware) using the ConnectTest Page and site/verification endpoint

Also confirmed in webmaster tools that the site/domain was linked to my account/s.

Also confirmed I could unlink and relink (manually) and have multiple people linked at the same time.

The one thing that caught me out was when swapping my GLA linked GA account. Because the option was already saved for the other account I couldn't do another verification so to speak which could catch people out if there is any account juggling going on.

composer.json Show resolved Hide resolved
Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far, just left a few small comments.

src/ConnectionTest.php Outdated Show resolved Hide resolved
@layoutd layoutd force-pushed the feature/site-verification branch 2 times, most recently from 910d954 to a3c1e18 Compare January 25, 2021 21:32
@layoutd layoutd requested review from mikkamp and jconroy January 25, 2021 21:33
Copy link
Member

@jconroy jconroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @layoutd

Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, it looks like it's pretty much ready to go. I just added a few small comments.

@layoutd layoutd force-pushed the feature/site-verification branch from 5c34065 to afd69c3 Compare January 26, 2021 15:37
@layoutd layoutd requested a review from mikkamp January 26, 2021 15:37
Copy link
Contributor

@mikkamp mikkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That all looks good to me. Although I'm not quite sure if the do_action names are a bit too fancy. Especially if we consider that a logging implementation needs to hook into each of them. In my mind I was thinking it would be best to stick to a handful of names mainly based on the type of exception it's passing so it can handle retrieving the details correctly. I personally prefer hardcoded strings (without get_slug) so I can search for them much easier.
Either way though, I'm fine to roll with whatever you think is best, so I'll approve this PR so we can move forward.

@layoutd
Copy link
Contributor Author

layoutd commented Jan 27, 2021

Thanks for your reviews and the thoughtful and insightful comments, @mikkamp. I'll merge now and make a note to revisit the issues you bring up regarding exceptions (naming and hard-coding the slug).

@layoutd layoutd merged commit 0a39278 into trunk Jan 27, 2021
@layoutd layoutd deleted the feature/site-verification branch January 27, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants