Plans: check if selectedSite exists before checking for domain credit #5089
Conversation
gwwar
added
[Feature] Plans & Upgrades
| @@ -335,7 +335,7 @@ export const List = React.createClass( { | |||
|
|
|||
| export default connect( ( state, ownProps ) => { | |||
| return { | |||
| hasDomainCredit: hasDomainCredit( state, ownProps.selectedSite.ID ) | |||
| hasDomainCredit: ownProps.selectedSite && hasDomainCredit( state, ownProps.selectedSite.ID ) | |||
There was a problem hiding this comment.
should this be done in current-site/notice.jsx as well?
There was a problem hiding this comment.
Yes a similar check, or enforce an object being passed through in propTypes:
propTypes: {
site: React.PropTypes.object.isRequired
},
There was a problem hiding this comment.
I'd prefer the latter of an explicit prop, but I added the former in 48a7bd2 for a quick fix
gwwar
force-pushed
the
fix/site-deletion
branch
from
87dcd09 to
48a7bd2
Compare
| @@ -92,7 +92,7 @@ const SiteNotice = React.createClass( { | |||
|
|
|||
| export default connect( ( state, ownProps ) => { | |||
| return { | |||
| hasDomainCredit: hasDomainCredit( state, ownProps.site.ID ) | |||
| hasDomainCredit: !! ownProps.site && hasDomainCredit( state, ownProps.site.ID ) | |||
There was a problem hiding this comment.
Isn't the convention to use Boolean() instead of !!?
FWIW, we usually just check only if it's falsy:
hasDomainCredit: ownProps.site && hasDomainCredit( state, ownProps.site.ID )
There was a problem hiding this comment.
It's a common js usage. I think its a bit more clear for !! to denote that we're testing for truthyness. The Boolean conversion can sometimes return unexpected things, like so:
Boolean("false"); // still returns true
|
Thanks for fixing it Kerry! I think one way to reproduce it is to sandbox the API (or to make API calls very slow :)). |
scruffian
removed
the
[Status] Needs Review
Potential fix for #5084. This adds bulletproofing before we check if a site has domain credits.
I wasn't able to reproduce the original issue, but we should handle this case anyway.
cc @fabianapsimoes @stephanethomas @rralian