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

Plans: Remove siteSpecificPlansDetailsList and siteSpecificPlansDetailsMixin in favor of new actions/reducers #1649

Merged
merged 10 commits into from
Dec 23, 2015
Merged

Plans: Remove siteSpecificPlansDetailsList and siteSpecificPlansDetailsMixin in favor of new actions/reducers #1649

merged 10 commits into from
Dec 23, 2015

Conversation

drewblaisdell
Copy link
Contributor

This PR updates the components in client/my-sites/plans/ to use site plans data in the global state.

Previously, the /plans components used siteSpecificPlansDetailsList, which:

  • Was a module containing a singleton class that fetched/stored data related to site plans.
  • Used site.domain instead of site.slug to namespace plans, which causes namespace conflicts for sites that share a domain property.
  • Was used with a mixin, siteSpecificPlansDetailsMixin.

This PR:

  • Removes siteSpecificPlansDetailsList and siteSpecificPlansDetailsMixin.
  • Updates all instances of use of the former to use the new global store.

Testing
The plans components are used in both /plans and on the plans step in signup, so it is important that we assert that the behavior of both places remains unchanged.

Signup

  • Visit http://calypso.localhost:3000/start
    • Assert that you can complete signup with the free plan, and are not redirected to checkout afterward.
    • Assert that you can complete signup with a paid plan, and are redirected to checkout afterward.
    • Assert that you can view the 'Compare Plans' page on the plans step by clicking one of the 'Learn more' links.

Plans

  • Visit http://calypso.localhost:3000/plans/:site where :site is the slug of a site with the free plan.
    • Assert that you are taken to checkout with a free trial by clicking 'Start Free Trial'.
    • Assert that you are taken to checkout with a full plan by clicking 'Upgrade Now'.
  • Visit http://calypso.localhost:3000/plans/:site where :site is the slug of a site with a free trial.
    • Assert that you see the PlanOverview component indicating the amount of time remaining for the trial.
    • Assert that you are taken to checkout with the full plan by clicking 'Purchase Now'.
  • Visit http://calypso.localhost:3000/plans/:site where :site is the slug of a site with a premium/business plan.
    • Assert that the behavior of the buttons below each plan (to manage the plan or upgrade to business if the site has premium) is unchanged.

Reviews

  • Code
  • Product

@drewblaisdell drewblaisdell added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] In Progress [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. labels Dec 15, 2015
@drewblaisdell drewblaisdell self-assigned this Dec 15, 2015
@drewblaisdell drewblaisdell added this to the No-cc Free Trials: v1 milestone Dec 15, 2015
@drewblaisdell drewblaisdell mentioned this pull request Dec 16, 2015
Merged
@drewblaisdell drewblaisdell force-pushed the add/site-plans-reducer branch 6 times, most recently from 6895799 to b38c5b0 Compare December 16, 2015 18:33
@drewblaisdell drewblaisdell 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 Dec 16, 2015
@scruffian
Copy link
Member

Forgive me if I have misunderstood, but isn't one of the strengths of Redux that we don't have to pass the state down through the props any more? I guess that's an option we have, but we can still do it the old way too?

scruffian
scruffian reviewed Dec 16, 2015
View reviewed changes
client/components/plans/plan-list/index.jsx
plansList,
currentPlan;

if ( plans.length === 0 || ( ! this.props.isInSignup && ! this.props.sitePlans.hasLoadedFromServer ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition looks long and ugly. Is it worth extracting it?

@scruffian
Copy link
Member

This looks great to me. It would probably be good for @stephanethomas and @Tug to take a look too...

@drewblaisdell drewblaisdell force-pushed the add/site-plans-reducer branch 3 times, most recently from 3b43eb9 to 0037d0e Compare December 17, 2015 00:46
@Tug
Copy link
Contributor

Tug commented Dec 17, 2015

I'll have a look but don't wait for me to merge this ;)

@ghost
Copy link

ghost commented Dec 18, 2015

👍 LGTM everything works as expected

@ghost ghost mentioned this pull request Dec 19, 2015
Merged
2 tasks
stephanethomas
stephanethomas reviewed Dec 20, 2015
View reviewed changes
client/components/plans/plan-list/index.jsx
return (
<Plan placeholder={ true }
isInSignup={ this.props.isInSignup }
key={ 'plan-' + n } />
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge deal but we could have used a template string here.

@stephanethomas
Copy link
Contributor

I've just found another issue on the Plans step during signup where no price would be displayed:

screenshot

@stephanethomas stephanethomas 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 Dec 20, 2015
@drewblaisdell drewblaisdell force-pushed the add/site-plans-reducer branch 2 times, most recently from 22a3f8a to 6ef6be0 Compare December 21, 2015 18:18
@drewblaisdell drewblaisdell mentioned this pull request Dec 21, 2015
Closed
@drewblaisdell
Copy link
Contributor Author

Thanks for catching that, @stephanethomas. I updated and added to the PR to address your feedback.

I'm not sure if this relates to this pull request but I was presented with both paid plans on the Purchases page after upgrading from Premium to Business (using the store sandbox)

This is unrelated to this PR, and is because we don't never remove purchases from PurchasesStore. I opened an issue for it: #1888

@drewblaisdell drewblaisdell 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 Dec 21, 2015
@drewblaisdell
Copy link
Contributor Author

Note: if this is merged after #1780, the changes to the siteSpecificPlansDetailsList assembler from that PR will need to be copied into the sitePlans assembler in this PR.

@drewblaisdell
Plans: Merge changes to site-plans assembler from #1780
3791d64
@drewblaisdell
Plans: subscribedMoment → subscribedDayMoment
07edc2d 
@Tug pointed out that we should indicate the disparity between
`subscribedDate` and `subscribedMoment` here, because the latter
is set to the start of the day.
Tug
Tug reviewed Dec 22, 2015
View reviewed changes
client/my-sites/plans/main.jsx
@@ -99,3 +113,18 @@ module.exports = React.createClass( {
);
}
} );

module.exports = connect(
( state, props ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use named functions here? Just to make it easier for us who are starting with redux.

@Tug
Copy link
Contributor

Tug commented Dec 23, 2015

Code looks good 💥

@Tug Tug 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 23, 2015
Tug added a commit that referenced this pull request Dec 23, 2015
@Tug
Merge pull request #1649 from Automattic/add/site-plans-reducer
daa7a3b 
Plans: Remove `siteSpecificPlansDetailsList` and `siteSpecificPlansDetailsMixin` in favor of new actions/reducers
@Tug Tug merged commit daa7a3b into master Dec 23, 2015
@lancewillett lancewillett deleted the add/site-plans-reducer branch December 30, 2015 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@drewblaisdell drewblaisdell

Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Feature] Signup & Account Creation All screens and flows for making a new WordPress.com account. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@drewblaisdell @scruffian @Tug @stephanethomas