wp-calypso icon indicating copy to clipboard operation
wp-calypso copied to clipboard

Emails: Add informative message and actions when user can't add email to the selected domain

Open AllTerrainDeveloper opened this issue 2 years ago • 3 comments

Proposed Changes

As shown here 1200182182542585-as-1201225902975505/f we should be more informative to the user when the user is not able to add an email subscription to the domain, when the user is not the owner of the domain.

Testing Instructions

  1. Transfer a domain to your site, where your user is not the owner, and there are no email subscriptions attached to it
  2. Go Upgrades -> Emails
  3. Choose the transferred domain
  4. Assert that you see this screen:

image

Assert the following statements:

  1. The "log in" redirects you to the login page
  2. The reach-out link is enabled if the domain is registered with us and has privacy enabled.
  3. Each of the select buttons are disabled

Assert when clicking on reach out link that you see the following screen: image

Pre-merge Checklist

  • [ ] Have you written new tests for your changes?
  • [ ] Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • [ ] Have you checked for TypeScript, React or other console errors?
  • [ ] Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • [ ] Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?

Related to #66650

AllTerrainDeveloper avatar Sep 22 '22 13:09 AllTerrainDeveloper

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~740 bytes added 📈 [gzipped])

name     parsed_size           gzip_size
email        +3570 B  (+0.5%)    +1122 B  (+0.6%)
domains      +2495 B  (+0.2%)     +740 B  (+0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot avatar Sep 22 '22 13:09 matticbot

The requirements changed a little bit over the course of a discussion in the comments, but we concluded that the Select button in EmailProvidersStackedCard should not link to the log in page for unauthorized users, but rather just be disabled.

pbLl1t-1v2-p2#comment-1425

fredrikekelund avatar Sep 22 '22 13:09 fredrikekelund

@wongasy The tracks event seems to be working fine for me. I've addressed every other one concern :)

AllTerrainDeveloper avatar Sep 25 '22 19:09 AllTerrainDeveloper

I have a followup pull request (for a different task) incoming that will refactor the message into its own component <EmailNonDomainOwnerMessage domain={ domain } selectedSite={ selectedSite } usePromoCard={ true }>

AllTerrainDeveloper avatar Sep 26 '22 10:09 AllTerrainDeveloper

Making a note that #68156 is related to this PR.

fredrikekelund avatar Sep 26 '22 13:09 fredrikekelund

I have a followup pull request (for a different task) incoming that will refactor the message into its own component <EmailNonDomainOwnerMessage domain={ domain } selectedSite={ selectedSite } usePromoCard={ true }>

@AllTerrainDeveloper It would make sense to do the refactoring in a separate PR if we were refactoring code that exists in trunk. But given that we are introducing new code in this PR anyways, might as well do the refactoring here to avoid potential regression issues that might be introduced in the refactoring.

wongasy avatar Sep 26 '22 20:09 wongasy

I have a followup pull request (for a different task) incoming that will refactor the message into its own component <EmailNonDomainOwnerMessage domain={ domain } selectedSite={ selectedSite } usePromoCard={ true }>

@AllTerrainDeveloper It would make sense to do the refactoring in a separate PR if we were refactoring code that exists in trunk. But given that we are introducing new code in this PR anyways, might as well do the refactoring here to avoid potential regression issues that might be introduced in the refactoring.

Done :)

AllTerrainDeveloper avatar Sep 27 '22 08:09 AllTerrainDeveloper

@AllTerrainDeveloper I noticed an issue where I'm seeing the non-owner screen in the email upsell step for adding a domain:

Screen Shot 2022-09-29 at 2 02 38 PM

Good catch, fixed.

AllTerrainDeveloper avatar Sep 30 '22 09:09 AllTerrainDeveloper

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7557365

Thank you @AllTerrainDeveloper for including a screenshot in the description! This is really helpful for our translators.

a8ci18n avatar Sep 30 '22 15:09 a8ci18n

@AllTerrainDeveloper Other than the minor comment I left regarding tracking, I also found a weird issue, where when user gets redirected to the email comparison page, the Professional Email card is left collapsed. I think source=login-redirect might be triggering that logic for collapsing the PE card.

Screen Shot 2022-09-30 at 1 11 52 PM

wongasy avatar Sep 30 '22 17:09 wongasy

@wongasy that issue was a tough one! Due to the nature of the async properties being fetched, the initial state is triggered with the initial loading state (which may or not have finished fetching everything)

I've added a useEffect hook with a dependency to the flag that is taking time to load (and will also determine the visibility of the notice)

AllTerrainDeveloper avatar Oct 03 '22 07:10 AllTerrainDeveloper

@AllTerrainDeveloper I ran into an issue while reviewing the other PR on the Email Management page changes.

When we remove all mailboxes, the non-owner will see the Email Comparison screen instead of the Email Management screen with the warning messages. This becomes an issue because non-owner admins could end up setting up Email Forwarding, when we have an active email subscription in place. The ideal fix would be to ensure non-owner users land on the Email Management page with the appropriate warning. However, that may involve a move complex fix with routing. As an alternative, we can simply update the Email Comparison screen to hide the Email Forwarding for non-owners.

Screen Shot 2022-10-03 at 2 36 41 PM

wongasy avatar Oct 03 '22 18:10 wongasy

@AllTerrainDeveloper Confirming that the email forward link has been hidden for non-owners.

Screen Shot 2022-10-04 at 12 24 58 PM

wongasy avatar Oct 04 '22 16:10 wongasy

When we remove all mailboxes, the non-owner will see the Email Comparison screen instead of the Email Management screen with the warning messages

I wasn't able to reproduce this. Here's what I did:

  • On a site with a single domain with Professional Email.
  • With the owner account, delete all mailboxes in that subscription.
  • With the non-owner account, go to Upgrades > Emails in Calypso.
  • I am immediately forwarded to the titan/set-up-mailbox page.

Could you outline the steps to reproduce this @wongasy?

Also, with this discussion in mind pbLl1t-1z0-p2, I'm thinking that we maybe shouldn't hide the email forwarding option for non-owner users? Being able to set up email forwarding if there's an existing email subscription sounds like a bug, but right now it looks like non-owners will never see that Start with Email Forwarding link, right?

fredrikekelund avatar Oct 05 '22 07:10 fredrikekelund

Could you outline the steps to reproduce this?

@fredrikekelund Hm... looks like I'm also unable to reproduce this. Initially I thought this could be related to the sandbox store, but I've tried both the regular store and sandbox store, and I was not able to reproduce the issue, which is great!

I'm thinking that we maybe shouldn't hide the email forwarding option for non-owner users? Being able to set up email forwarding if there's an existing email subscription sounds like a bug, but right now it looks like non-owners will never see that Start with Email Forwarding link, right?

If we allow any non-owners to do the initial enabling of email forwarding, then there's a lower chance for the domain-owner to see the Email Comparison page. When they visit Upgrades > Email, they'll see the Email Forwarding management options with only a small button that takes them to the Email Comparison page. From the perspective of promoting a paid service, I think it makes sense to hide the email forwarding option for non-owners. Essentially, only the domain-owner should have the right to choose an email solution (paid or free). And once the email solution is chosen, then non-owner admins can manage the solution, in the case of email forward, they can set up additional forwarding (what we've fixed recently). What do you think?

wongasy avatar Oct 05 '22 14:10 wongasy

🤔 On the one hand, the behavior that you describe (only the owner can set up, but other admins can manage) is closest to what we have today. Because right now, if a user isn't allowed to add email, we don't even show the Add Email link for that domain. On the other hand, I think it could be helpful to let all admin users add email forwarding so long as there aren't any technical obstacles to that (in the same way that ownership of purchased subscriptions is a technical obstacle).

IMO, admin user privileges should typically mean that a user can perform almost all actions on the site. We could also contrast with what user privileges are required to edit DNS records, which is something I believe all site admins can do.

So I think I'd cast my vote for showing the link to all users, simply because it's helpful and the Set up Email Forwarding link is already quite discrete. The difference in conversion should be slim, and we're already improving it with the other PR's within this same effort.

fredrikekelund avatar Oct 05 '22 15:10 fredrikekelund

I do agree that the link for setting up Email Forwarding is so subtle that the potential conversion impact may not actually be a valid concern. In the interest of wrapping this up quickly, @AllTerrainDeveloper I've gone ahead and reverted the change for hiding the Email Forwarding Link.

wongasy avatar Oct 05 '22 16:10 wongasy

Translation for this Pull Request has now been finished.

a8ci18n avatar Oct 10 '22 07:10 a8ci18n