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

Improve Google Verification Service integration #10143

Merged
merged 22 commits into from
Sep 25, 2018
Merged

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Sep 14, 2018

Fixes #10141
Requires server patch D18402-code

Changes proposed in this Pull Request:

  • Do not verify the site on page load, even if the user already has a google site verification token, wait for the user to click on the "Auto verify with Google" button
  • Display an error notice on request error
  • Revamp of the google site verification UI, props to @jeffgolenski for his design proposal!

Testing instructions:

Proposed changelog entry for your changes:

  • Improve Google Verification Service integration

@Tug Tug self-assigned this Sep 14, 2018
@Tug Tug requested a review from a team as a code owner September 14, 2018 12:24
@jetpackbot
Copy link

That's a great PR description, thank you so much for your effort!

Generated by 🚫 dangerJS

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Verification tools Admin Page React-powered dashboard under the Jetpack menu [Pri] BLOCKER labels Sep 14, 2018
@jeherve jeherve added this to the 6.6 milestone Sep 14, 2018
@jeherve
Copy link
Member

jeherve commented Sep 18, 2018

@Tug Is this ready to test? If so, could you add testing steps and change the labels so I can get started with testing?

Thank you!

Copy link

@stephanethomas stephanethomas left a comment

Choose a reason for hiding this comment

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

I'll second Jeremy, it's difficult to test those changes without more guidelines.

I noticed that once I successfully verified a site, the Traffic page shows a Click to Verify button. However when I click it it switches to Connect to Google, and then return to Click to Verify without any more visual feedback. I'm not sure what behavior we expect, but I find it confusing.

if ( token !== this.props.getSettingCurrentValue( 'google' ) ) {
return this.props.updateOptions( { google: token } );
}
} );
}

Choose a reason for hiding this comment

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

We should be using componentDidMount() to run side effects like this one. componentWillMount() has been deprecated.

@stephanethomas stephanethomas added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 18, 2018
@Tug
Copy link
Contributor Author

Tug commented Sep 18, 2018

@jeherve sorry I'm still working on it, it's not ready to test yet but should be soon.

@Tug Tug force-pushed the update/site-verif branch from 8174cb7 to 4574b80 Compare September 18, 2018 18:12
@Tug Tug force-pushed the update/site-verif branch from e7a098a to f1fffb7 Compare September 19, 2018 14:02
@Tug Tug added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Sep 20, 2018
@Tug
Copy link
Contributor Author

Tug commented Sep 24, 2018

I think we're ready for a final review.

We also added two user notices that display when a user tries to edit the HTML Tag.
Please review the copy of the 2 messages.

  • 1 is for the owner of the connection, a link will be display to unverify the site property

Editing this HTML Tag code won’t unverify your site with your Google account.
Action button: Unverify with Google

image

  • 2 is for other admin to let them know that editing the HTML tag won't unverify the site with google.

Editing this HTML Tag code won’t unverify your site with the Google account it is connected to.
No action button

image

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 24, 2018
@jeherve
Copy link
Member

jeherve commented Sep 24, 2018

1 is for the owner of the connection, a link will be display to unverify the site property

Oddly enough I got the notification without the action button, although I am the owner of the Jetpack connection on the site. Why do you think that is?

@Tug
Copy link
Contributor Author

Tug commented Sep 24, 2018

Oddly enough I got the notification without the action button, although I am the owner of the Jetpack connection on the site. Why do you think that is?

You might want to update the server patch maybe?

@jeherve
Copy link
Member

jeherve commented Sep 24, 2018

That was it, you were right. It works well now for me as the main admin. My site is verified with Google.

If, however, I log in using a different admin account, that is not yet connected to WordPress.com on the site, the UI shows me that the site has not been verified yet:

screenshot 2018-09-24 at 15 34 42

That seems wrong since the site is in fact verified:

screenshot 2018-09-24 at 15 35 09

@Tug
Copy link
Contributor Author

Tug commented Sep 24, 2018

@jeherve Good remark! In this case I think we'll revert to showing the default input field. There is no way of knowing if the site is verified since this information comes from wpcom.
Updating now!

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Everything seems to work well for me now on a brand new site. We should be good to go! 🚢

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Sep 24, 2018
@stephanethomas
Copy link

This looks good so far to me. I'm just wondering what's the exact use case for keeping the Edit button once a site has been verified?

screenshot

If it's just to unverify a site manually by editing the meta tag and going to Webmaster Central, then we could update that button to actually perform that process via the API. That way we could remove this warning:

image

Copy link
Contributor

@cbauerman cbauerman 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! Everything works as described. I agree with Stéphane, we can address the other issues with a follow up. LGTM 👍

@cbauerman
Copy link
Contributor

cbauerman commented Sep 24, 2018

When this PR is merged, please add the following instructions to to-test.md

### Admin Page

Added ability to verify the site with the Google Search Console with "one-click" and logging in to your Google account.

To Test:

1. Open a Jurassic Ninja Site set to the master branch via [this link.](https://jurassic.ninja/create?jetpack-beta&branch=master&shortlived&wp-debug-log)
2. Set Up Jetpack and connect to a Wordpress.com account.
3. Choose a Free Plan
4. Navigate to https://[my-site].jurassic.ninja/wp-admin/admin.php?page=jetpack#/traffic
5. Scroll to the "Site verification" pane.
6. Click "auto-verify with Google"
7. Choose a Google Account and log into it
8. Confirm that the Google Site Verification field has a green check marks and the text "Your site is verified with Google"
9. You should see a link to the Google Search Console, click on it and verify that you can access your site there.
10. Click the "Edit" button and edit the text field, clear it, and click save.
11. Navigate to https://www.google.com/webmasters/verification/home and confirm you can see your Jurassic.ninja site.
12. Click on  "Verification Details" to the right of your site.
13. Click "Unverify" near the bottom right and "Unverify" in the dialog.
14. Confirm that the site is removed from the list of properties and that no error appears
15. Return to https://[my-site].jurassic.ninja/wp-admin/admin.php?page=jetpack#/traffic or refresh and confirm that the Google Site Verification field has revert to its original two button layout. 

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Working swimmingly. Let's do it.

@kraftbj kraftbj merged commit 84cc2e9 into master Sep 25, 2018
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 25, 2018
@jeherve jeherve deleted the update/site-verif branch September 25, 2018 06:06
@Tug
Copy link
Contributor Author

Tug commented Sep 25, 2018

If it's just to unverify a site manually by editing the meta tag and going to Webmaster Central, then we could update that button to actually perform that process via the API. That way we could remove this warning:

It's not only for unverifying, it could be used to manually enter other tags to verify other google accounts.

@stephanethomas
Copy link

It's not only for unverifying, it could be used to manually enter other tags to verify other google accounts.

We only support one meta tag per site. My understanding is that users may change the meta tag to verify other accounts, but since Google periodically checks the meta tag of each account this will inevitably lead to having some of them becoming unverified.

jeherve pushed a commit that referenced this pull request Sep 25, 2018
**DO NOT MERGE**
 Includes changes from #10143 and should be rebased and merged after

Adds a disclaimer to the Google Site Verification that warns users that connecting to Google Search Console will cause emails to be sent to them.

#### Changes proposed in this Pull Request:

**Before**
<img width="730" alt="screen shot 2018-09-24 at 7 05 23 pm" src="https://user-images.githubusercontent.com/2810519/45988682-fbc6bd80-c02c-11e8-9fa0-3789a2215f29.png">

**After**
<img width="723" alt="screen shot 2018-09-24 at 7 03 13 pm" src="https://user-images.githubusercontent.com/2810519/45988688-05502580-c02d-11e8-929c-d93ac66d7997.png">


#### Testing instructions:

<!--
Add as many details as possible to help others reproduce the issue and test the fix.
"Before / After" screenshots can also be very helpful when the change is visual.

Would you like this feature to be tested by Beta testers as well?
Please add instructions to to-test.md in a new commit as part of your PR.
-->

* Load up a Jurassic.Ninja Live Branch [here](https://jurassic.ninja/create?jetpack-beta&branch=update/google-site-verification-ui&shortlived&wp-debug-log)
* Connect to WordPress.com
* Navigate to https://[my-site].jurassic.ninja/wp-admin/admin.php?page=jetpack#/traffic
* Scroll to the "Site verification" pane
* Connect your site to Google via the "Auto-verify with Google" button
* Verify that the screen now is nearly identical with the "After" image above
jeherve added a commit that referenced this pull request Sep 25, 2018
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Admin Page React-powered dashboard under the Jetpack menu [Feature] Verification tools [Pri] BLOCKER [Status] Design Review Complete [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants