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

Blocks: Introduce a ProductPlanOverlapNotices block #37513

Merged
merged 7 commits into from
Nov 13, 2019

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Nov 12, 2019

Changes proposed in this Pull Request

  • Blocks: Introduce a ProductPlanOverlapNotices block

Preview

Testing instructions

@tyxla tyxla added Jetpack [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Pri] Normal [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components [Type] Feature progress New Component Adds a new common UI component labels Nov 12, 2019
@tyxla tyxla requested a review from a team November 12, 2019 14:12
@tyxla tyxla self-assigned this Nov 12, 2019
@matticbot
Copy link
Contributor

@matticbot
Copy link
Contributor

matticbot commented Nov 12, 2019

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

App Entrypoints (~36 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-main                 +46 B  (+0.0%)      +19 B  (+0.0%)
entry-login                +46 B  (+0.0%)       +9 B  (+0.0%)
entry-gutenboarding        +46 B  (+0.0%)       +8 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Async-loaded Components (~565 bytes added 📈 [gzipped])

name                      parsed_size           gzip_size
async-load-design-blocks      +2973 B  (+0.1%)     +565 B  (+0.1%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

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.

@enejb
Copy link
Member

enejb commented Nov 12, 2019

This was a bit hard to test, but overall I really like the implementation.

There is something that are not quite accurate.
Currently I was able to see the following.
Screen Shot 2019-11-12 at 3 40 22 PM

The current Personal plan doesn't have Realtime backups but just daily backups.
I don't have a suggestion but the copy could also use some ❤️ . Does it need to be so verbose?

Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

I've reviewed the code and left a couple of minor comments. I'm going to test it locally next.

client/blocks/product-plan-overlap-notices/README.md Outdated Show resolved Hide resolved
@tyxla
Copy link
Member Author

tyxla commented Nov 12, 2019

The current Personal plan doesn't have Realtime backups but just daily backups.

Nice catch @enejb! Should be fixed with 444acf3 - could you give it another spin?

@tyxla
Copy link
Member Author

tyxla commented Nov 12, 2019

Thanks for the great catches @delawski and @enejb - addressed what you folks reported. Would appreciate another 👀

Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

I've tested locally and while I'm seeing the notification when I'm owning the Real-Time option, there is no message displayed if I have the Daily Backup in conjunction with Jetpack Personal or Premium plan.

@tyxla could you double check if those combinations work for you or is it some issue on my end only?

@delawski delawski added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 12, 2019
@tyxla
Copy link
Member Author

tyxla commented Nov 13, 2019

@delawski thanks for testing - the problem here is not related with my code; it's coming from the WP.com store - it doesn't allow having a Personal or Premium plan together with the Daily backup product. If you try purchasing one of them while having the other, the existing plan or product will get automatically canceled in favor of the new plan or product. Take a look at your purchases page or your SA and you will notice that you only have a single product or plan.

AFAIK, this is something that @Automattic/chronos are aware of (cc @seear and @rcoll), but I'm not sure if it's actively being worked on.

Since the logic in the code is generic, it should work for other cases like this, as long as the store allows it and the purchases endpoint returns both a plan and a product purchase.

If you really want to test it, you can force the getSitePurchases selector to return an additional dummy purchase for a product while you're on a plan.

Let me know how that goes, but I'd really love to push this forward if it works for cases that the store currently supports.

@tyxla tyxla 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 Nov 13, 2019
@tyxla tyxla requested review from delawski and enejb November 13, 2019 07:39
@tyxla tyxla dismissed delawski’s stale review November 13, 2019 07:43

Issue is not related with this PR and with Calypso in general.

Copy link
Contributor

@delawski delawski left a comment

Choose a reason for hiding this comment

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

@tyxla Thank you for explaining where the issue I noticed come from! In this case I think this is good to go! Let's 🚢

@delawski delawski 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 Nov 13, 2019
@enejb
Copy link
Member

enejb commented Nov 13, 2019

While tested this and I found that when a site has the daily backup and the professional plan we don't show the notice at all in this case.

I am not sure if this should be addressed in a different PR or this one?
Since that type of nice is a bit different then the current overlap notice.

@tyxla
Copy link
Member Author

tyxla commented Nov 13, 2019

I think it's just a different type of overlap, but we can address it in a subsequent PR. I'll ship.

Thank you guys 💪

@tyxla tyxla merged commit fd19feb into master Nov 13, 2019
@tyxla tyxla deleted the add/product-plan-overlap-notices-block branch November 13, 2019 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. Jetpack New Component Adds a new common UI component [Pri] Normal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants