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.

Sign up for GitHub

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

Jump to bottom

Fixes wrong selected domain name #29824

Merged
merged 7 commits into from
Jan 2, 2019
Merged

Fixes wrong selected domain name #29824

merged 7 commits into from
Jan 2, 2019

Conversation

belcherj
Copy link
Contributor

@belcherj belcherj commented Dec 31, 2018 •
edited
Loading

Changes proposed in this Pull Request

  • Fixes: p2MSmN-6Vk-p2

Testing instructions

  • Have site with custom domain and no G Suite
  • Have *.wordpress.com as primary domain
  • Goto domains and click on custom domain
  • Click "email"
  • Click to add G Suite
  • Add user
  • Goto checkout and observe that domain is correct in the left column

@belcherj belcherj added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 31, 2018
@belcherj belcherj self-assigned this Dec 31, 2018
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=fix/wrong-selected-domain

stephanethomas
stephanethomas approved these changes Dec 31, 2018
View reviewed changes
Copy link
Contributor

@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 only had two minor suggestions, otherwise this works fine 👍.

client/my-sites/domains/domain-management/add-google-apps/add-email-addresses-card.jsx Outdated
if ( this.props.selectedDomainName ) {
if (
this.props.selectedDomainName &&
getGoogleAppsSupportedDomains( [ this.props.selectedDomainName ] ).length
Copy link
Contributor

Choose a reason for hiding this comment

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

We could use the hasGoogleAppsSupportedDomain() function instead.

Copy link
Contributor Author

@belcherj belcherj Jan 1, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think that helps us. Domains needs to be loaded for that. Additionally we just want to check if the domain is valid for G Suite. I think we can make this better in a refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure to understand what you mean. I was just suggesting the following change:

if (
    this.props.selectedDomainName &&
    hasGoogleAppsSupportedDomain( [ this.props.selectedDomainName ] )
) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhhhh, that works perfect.

client/my-sites/domains/domain-management/email/add-google-apps-card.jsx Outdated
@@ -47,7 +47,7 @@ class AddGoogleAppsCard extends React.Component {

goToAddGoogleApps = () => {
page(
domainManagementAddGoogleApps( this.props.selectedSite.slug, this.props.selectedSite.domain )
domainManagementAddGoogleApps( this.props.selectedSite.slug, this.props.selectedDomainName )
Copy link
Contributor

Choose a reason for hiding this comment

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

We're using selectedSite.domain in two other places in this AddGoogleAppsCard component. Shouldn't we update them as well?

@stephanethomas stephanethomas added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 31, 2018
@stephanethomas
Copy link
Contributor

I'll address my comments, and merge it.

@stephanethomas
Copy link
Contributor

I'm afraid we'll have to iterate a bit more on that one, I've just noticed that the domain name displayed on the Add G Suite page is not the right one:

screenshot

I bet we trigger a Tracks event for the Learn more about integrating G Suite with your site. link with the wrong domain name as well.

@belcherj belcherj added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jan 1, 2019
@stephanethomas
Copy link
Contributor

I've just noticed that the domain name displayed on the Add G Suite page is not the right one:

This is still happening when the primary address of the site is set to the free .wordpress.com address. Sorry, I should have mentioned that.

@belcherj
Copy link
Contributor Author

belcherj commented Jan 1, 2019

Ahh, so if there is no selected site what should we display?

@stephanethomas stephanethomas added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 1, 2019
@stephanethomas
G Suite: Offer G Suite only for eligible domains
4162f0d 
This fixes the case where the primary address of the site is set to the free .wordpress.com address (not to any custom domain).
@stephanethomas
Copy link
Contributor

I've added a commit to retrieve and display the first domain that is eligible to G Suite.

@stephanethomas stephanethomas added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 1, 2019
@belcherj
Copy link
Contributor Author

belcherj commented Jan 2, 2019

screen shot 2019-01-02 at 10 01 47 am

Unfortunately this will pull in the latest site and not use the selected site if it exists.

@belcherj belcherj added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2019
@belcherj belcherj added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Jan 2, 2019
@robertf4
Copy link
Contributor

robertf4 commented Jan 2, 2019 •
edited
Loading

LGTM

@robertf4 robertf4 added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2019
@belcherj belcherj merged commit 52f7dbb into master Jan 2, 2019
@belcherj belcherj deleted the fix/wrong-selected-domain branch January 2, 2019 20:06
blowery added a commit that referenced this pull request Jan 3, 2019
@blowery
Merge remote-tracking branch 'origin/master' into feature/color-refre…
ebe595c 
…sh-2019

* origin/master:
  Disable select when only one domain exiists (#29873)
  Tiled galleries: Disable captions (#29776)
  Prevent scrolling up when opening a dialog (#29832)
  Gutenberg: Move Shortlinks to production (#29883)
  Gutenlypso: add convert to blocks dialog (#29790)
  Gutenberg: Move Related Posts to production blocks (#29650)
  Analytics: fix ad-tracking issue on signup for non-gdpr countries (#29741)
  Update mobile phone validation module (used for 2fa) (#29740)
  Gutenlypso: make sure titles load on second edit (#29877)
  Site Picker: Change wording of /page and /block-editor to match /post (#29859)
  Gutenberg: update copy link in page list to be editor aware
  Gutenberg: use core approach of initialEdits over overridePost
  Gutenberg: when duplicating a post, override post content
  Gutenberg: update duplicate url when Gutenlypso is enabled
  Refactor: Replace use of key-mirror with inline code (#29857)
  Fixes wrong selected domain name (#29824)
  Turn off prettier for SASS, use stylelint instead (#29697)
  Gutenberg: Add copy button to Shortlinks (#29556)
  add a section name to the body class (#29563)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephanethomas stephanethomas stephanethomas approved these changes

Assignees

@belcherj belcherj

Labels
G Suite [Type] Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@belcherj @matticbot @stephanethomas @robertf4