-
Notifications
You must be signed in to change notification settings - Fork 2k
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 warning for mapped domain with wrong NS config #3329
Conversation
1dfe3bd
to
13fef8a
Compare
const wrongMappedDomains = this.getDomains().filter( domain => | ||
domain.type === domainTypes.MAPPED && ! domain.hasZone ); | ||
|
||
let learnMoreLink = ( <a |
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.
It looks like we don't need this learnMoreLink
variable. It's only used once below.
The issue of configuring name servers for domains mapped to a WordPress.com site is a bit complicated, I'm not sure this notice addresses that. There are several use cases that I think could use special attention:
Otherwise, the format of the notice could be a bit problematic. We can only show a very short message here, and we don't have a complete flow to address the issue, we're just pointing to a support doc from what I can tell. We could address this by creating a support doc dedicated to this notice, so that it would fill in for the missing flow. That could help people get the right idea even if they don't immediately understand what it means that their "name servers should be configured". This notice might work with that:
This doesn't really address the issues mentioned above. But it could at least make for a better notice. @umurkontaci Let me know what you think and I can prepare a dedicated support page for this if necessary. |
I agree that it's not an error. Ideally, this should be integrated in something like a post-purchase flow.
I think this warrants an error notice. Although I don't think there's a way for us to know whether it worked before. We can assume this category if the domain is more than X days old and still not set up.
I didn't know that at all. It's impossible to differentiate this from the previous category. The only solution I can find for that is to allow people to migrate to site redirect and cancel their mapping sub; but this is way overkill for a notice.
I think this is the quick win here. It's relatively easy to differentiate between a new vs an old domain, so we can change the copy / flow for them in the future. |
Perhaps we can start by checking how many mapped domains currently have non-wpcom name servers set up. That would give us an idea of how many people will be affected by this when the notice first launches. We have done this analysis in the past, I'll try to look up some examples. |
Code looks good (althought depending on Ran's findings might need some reworking), but I just wanted to note that the variable |
@ranh Let me know if I can help with that. |
Where is this name server data coming from and how often is it refreshed? |
I'm not sure about how many resolvers we have, but DNS data is cached on every resolver depending on its TTL. NS records have usually very large TTLs but it definitely varies. This approach was being used in wp-admin and I don't recall hitting any query limits. Those limits are pretty high, e.g. Google DNS supposedly has 500QPS, which makes it 43m/day. @aidvu How about going a different way, instead of checking NS records, can we I think this would get rid of the issue if the users want to keep their records at somewhere else and willing to use a subdomain with a CNAME record. |
domain.type === domainTypes.MAPPED && ! domain.hasZone ); | ||
|
||
let learnMoreLink = ( <a | ||
href="https://support.wordpress.com/domains/change-name-servers" |
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.
Are we sure this is a proper link?
The whole notice should be displayed, when domain is bought elsewhere and Name Servers should be set in the admin panel of domain provider.
The attached link shows how to point domain bought on wp.com to outside name servers.
I believe this is the link that was supposed to be here: https://en.support.wordpress.com/map-existing-domain/#2-ask-your-domain-provider-to-update-your-dns-settings
Also, we show the same link in case of subdomain, in which case we should give this link (I believe): https://en.support.wordpress.com/map-subdomain/#instructions-for-mapping-subdomains
And information about CNAME....
Also I really find the Domain Helper output being pretty helpful and a bit more in context then a general support page.
'https://en.support.wordpress.com/domain-helper/?host=' + wrongMappedDomains[0].name
After testing this we now know that app. 10% of mapped domains are not using WordPress.com name servers. Of course, we don't know if these domains are set up that way intentionally or not. |
@ranh What is your take from the result? |
@umurkontaci I think 10% is a significant group of users that we don't exactly understand. Maybe we could start with a more conservative approach, for example a lower key notice that is less prominent. That said, note that we're also currently sending email reminders to owners of mapped domains that are not using out name servers, and those emails also don't address the site redirect use case. |
@ranh :
Well, how can it even work intentionally? Something has to be wrong here. If the users are keeping the mapping "just in case", we can provide "dismiss" option for that notice, but I cannot imagine technically how keeping that mapping for the domain that points elsewhere can work. |
@ranh, my point is, that regarding your comment above:
We should show the notice, because users very often forget to configure NS. There is an avalanche of tickets every day and HE remind people that that they need to update NS
We should show the notice because obviously something is wrong
I disagree. If they did that, they should delete the mapping. The mapping is essentially ineffective and there is no indication of that anywhere. My point is: in all of these cases something is misconfigured and we should inform users of that. |
Note that the domain mapping in this case is not ineffective. It is effectively equivalent to using the Site Redirect upgrade to redirect the site away from WordPress.com. It costs the same too. I guess the ideal scenario here would be that every instance of domain mapping that is used as a site redirect is converted to an actual site redirect upgrade. But that is a big project with very vague benefits. |
I am not sure about that... If my domain points to WP NS and I have a site redirect upgrade, I still can put custom settings in DNS editor:
During support rotation I had a user that had an email in the domain that was actually mapped and then he changed NS settings, so his email stopped working and even he didnt get any information from us, because well, his email stopped working :) I think that NS changing to point elsewhere is the most dangerous scenario of all :) |
@artpi Using mapped domains instead of Site Redirect looks like this: You have two options:
Basically a mapped domain allows you to a set a specific domain as a primary domain and since we redirect requests to other domains to the primary domain, this can be used instead of a Site Redirect upgrade. No DNS involved in this flow, it's just a HTTP redirect. |
13fef8a
to
2aef0ad
Compare
4a2edc2
to
6257396
Compare
@klimeryk Fixed the conflicts, everything seems OK. |
6257396
to
a8d7d99
Compare
Awesome work, @umurkontaci! 👍 |
This enables the warning also on the individual domain's view, not only on the main list.
a8d7d99
to
b174f2d
Compare
This fixes #3152.
Goes with D2003-code.
There's a bug where mapped domain notices don't work, fix in #6983 and D2348-code.
This adds a warning notice when you buy a mapped domain but don't update the nameserver records to point to our name servers.
There are three place where this appears:
@ranh or @breezyskies: I had to change GridIcon size from 16 to 18 (It wouldn't let me commit), fyi.
Screenshot:
Testing:
PS: Although we'd like the message to visible to make people configure their site, there are some legitimate (albeit not endorsed by us) ways to get a mapped domain working without changing NS records. So there's a slight chance that seeing this message everywhere might be annoying for some.
/cc: @klimeryk @aidvu for code
/cc: @ranh @breezyskies for copy and ideas on ^