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

Domain Management: add unlimited email forwarding #3156

Merged
merged 1 commit into from
Feb 18, 2016

Conversation

jeremeylduvall
Copy link
Contributor

The Business bundle allows for unlimited email forwards. Currently though, Business users are still limited to 5 email forwards in Calypso. This PR does a few different things:

  1. Create index.js in client/lib/domains/email-forwarding/.
  2. Add a function emailForwardingPlanLimit that accepts product_id as a param and returns the allowed amount of email forwards.
  3. In client/my-sites/upgrades/domain-management/email-forwarding/email-forwarding-add-new.jsx, this current email forwarding list length is compared against the return value from emailForwardingPlanLimit so the limit is set to 5 for Premium users and 100 for Business users (Easy to bump up, but 100 seemed pretty high).
  4. The limit at the bottom of the email forwarding page ("You are using _ out of _ email forwards.") uses the return value from emailForwardingPlanLimit to set the correct number for the "out of" value. In order for this to work, I added selectedSite as a prop so I could reference it later on.

To test:

  1. Setup a site with a Business bundle
  2. Start adding email forwards. You should see a limit of 100.
  3. Remove the Business bundle. The limit will drop back to 5. If you're at or over 5, the Add New button is disabled.

Here's how this currently looks on a site with a Business bundle active and 6 email forwards setup:

screen shot 2016-02-07 at 9 27 03 pm

With 6 email forwards setup, I removed the Business bundle and the limit is set back to 5:

screen shot 2016-02-07 at 9 32 01 pm

Fix for #2387. h/t to @bikedorkjon for writing most of the code in the original bug report. @gziolo is the kind of setup you were thinking of?

@jeremeylduvall jeremeylduvall added the [Feature Group] Emails & Domains Features related to email integrations and domain management. label Feb 8, 2016
@jeremeylduvall jeremeylduvall self-assigned this Feb 8, 2016
@jeremeylduvall jeremeylduvall added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 8, 2016
@jeremeylduvall jeremeylduvall force-pushed the fix/2387-unlimited-email-forwards branch from e6e01fb to b2eaa9c Compare February 8, 2016 22:18
@@ -0,0 +1,12 @@
function emailForwardingPlanLimit( product_id ) {
let planLimit;
if ( product_id === 1008 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we try to avoid including a product ID or slug at a random place in the code like this, in favor of functions that do the number/string comparison for us. This makes it easier for us to change the product ID or slug, as we only need to modify the function that does the actual comparison.

Currently, we use isBusiness for this, which lives in lib/products-values. If you change this function so that it accepts a product (which, in this case, is the plan), you could use that function instead of this comparison (i.e. isBusiness( plan )).

@jeremeylduvall jeremeylduvall force-pushed the fix/2387-unlimited-email-forwards branch from b2eaa9c to 055a569 Compare February 10, 2016 21:03
@jeremeylduvall
Copy link
Contributor Author

Appreciate the feedback @drewblaisdell! Updated the function emailForwardingPlanLimit to accept plan instead of product_id. Used isBusiness() in the logic now instead of comparing the product_id. Still works as originally anticipated.

import { isBusiness } from 'lib/products-values';

function emailForwardingPlanLimit( plan ) {
let planLimit;
Copy link
Member

Choose a reason for hiding this comment

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

You can simplify that to:

return ( isBusiness( plan ) ? 100 : 5 );

@gziolo
Copy link
Member

gziolo commented Feb 15, 2016

Code looks good 👍 This is exactly what we need :D
I haven't tested it.
It just needs sign off from @bikedorkjon and can be 🚢

- This PR sets the email forwarding limit to 100 for Business users. Premium users still set at 5.
@jeremeylduvall jeremeylduvall force-pushed the fix/2387-unlimited-email-forwards branch from 055a569 to 789f3e3 Compare February 16, 2016 19:41
@jeremeylduvall
Copy link
Contributor Author

Thanks @gziolo! Changed the emailForwardingPlanLimit function to just be return ( isBusiness( plan ) ? 100 : 5 );. Much simpler. Still works as originally anticipated. Appreciate it!

@bikedorkjon
Copy link

I was able to add 7 email forwards. Tested and LGTM! Nice work, @jeremeylduvall :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants