Free Trials: Fix display of stale data in PlanOverview
#1987
Conversation
Tug
added
[Feature] Plans & Upgrades
Adding the `REMOVE_SITE_PLANS` action type which simply removes a site from the site.plans store. The `updateSitePlans` actions dispatch a `REMOVE_SITE_PLANS` action add calls `fetchSitePlans`. For this commit we call the `updateSitePlans` action when the thank you page is mounted, so going back to the plan page won't display stale data.
Tug
force-pushed
the
fix/1465-stale-data-plan-overview
branch
from
ed75319 to
3805e23
Compare
Tug
force-pushed
the
fix/1465-stale-data-plan-overview
branch
from
fc0652a to
4e8ee99
Compare
Tug
added
[Status] Needs Review
| * @param {Object} siteId ID of the concerned site | ||
| * @return {Object} Action object | ||
| */ | ||
| export function updateSitePlans( siteId ) { |
There was a problem hiding this comment.
What do you think about naming this something like refreshSitePlans? updateSitePlans sounds to me like we're trying to change them on the server.
| @@ -632,4 +643,15 @@ PurchaseDetail = React.createClass( { | |||
| } | |||
| } ); | |||
|
|
|||
| module.exports = CheckoutThankYou; | |||
| module.exports = connect( | |||
| function mapStateToProps( state, props ) { | |||
There was a problem hiding this comment.
We have a linting rule against using defining variables that are never used. We should just skip argument this with an anonymous function () => {} or undefined. The later would be a better solution if it works (and I think it does, based on the docs).
….handleServerAction' because this action is triggered by the client
| @@ -63,6 +66,14 @@ var CheckoutThankYou = React.createClass( { | |||
| }, | |||
|
|
|||
| componentDidMount: function() { | |||
| var selectedSite = this.props.lastTransaction.selectedSite; | |||
| this.props.refreshSitePlans( selectedSite.ID ); | |||
There was a problem hiding this comment.
Do we need to refresh these for all purchases, or just when the user checks out with a trial/plan?
There was a problem hiding this comment.
You are right, we should refresh only when the user purchased a plan. I tested with themes though and it never reaches the thank you page.
I haven't tested domain or other upgrades so I guess it's cleaner to check for the type of transaction explicitly.
| siteId | ||
| } ); | ||
|
|
||
| return fetchSitePlans( siteId ); |
There was a problem hiding this comment.
In order for this function to fetch site plans, we need to pass the result of this call to dispatch like
dispatch( fetchSitePlans( siteId ) );This is because refreshSitePlans and fetchSitePlans are 'thunk action creators' (docs) and return functions that take dispatch.
Tug
force-pushed
the
fix/1465-stale-data-plan-overview
branch
from
229b141 to
ec48c4c
Compare
|
@drewblaisdell Thank you for reviewing, I have fixed the problems you reported. |
| var lastTransaction = this.props.lastTransaction, | ||
| selectedSite; | ||
|
|
||
| if ( lastTransaction ) { |
There was a problem hiding this comment.
Do we need to guard against a missing lastTransaction here? There are a couple other places in this file where we assume that it is present.
There was a problem hiding this comment.
True, I added it because I encountered an undefined lastTransaction error here (when reloading or going back to this page), but I think it will still throw an error because of those couple of other places.
|
I had one question about something small. Otherwise, this LGTM. |
|
Good stuff 👍 |
ghost
added
[Status] Ready to Merge
and removed
[Status] Needs Review
…rview Free Trials: Fix display of stale data in `PlanOverview`
Fixes #1465
where stale data is displayed when navigating to the plan page and the plan information is not yet available.
Before:
After:
Review guide:
git fetch origin && git checkout fix/1465-stale-data-plan-overviewmake runMy Sitesand thenPlans.cc: @spncrb @drewblaisdell