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

Stats: Add stats list to redux tree #5743

Merged
merged 4 commits into from
Jun 6, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions client/state/action-types.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,10 @@ export const SITE_PLANS_TRIAL_CANCEL = 'SITE_PLANS_TRIAL_CANCEL';
export const SITE_PLANS_TRIAL_CANCEL_COMPLETED = 'SITE_PLANS_TRIAL_CANCEL_COMPLETED';
export const SITE_PLANS_TRIAL_CANCEL_FAILED = 'SITE_PLANS_TRIAL_CANCEL_FAILED';
export const SITE_RECEIVE = 'SITE_RECEIVE';
export const SITE_STATS_RECEIVE = 'SITE_STATS_RECEIVE';
export const SITE_STATS_REQUEST = 'SITE_STATS_REQUEST';
export const SITE_STATS_REQUEST_FAILURE = 'SITE_STATS_REQUEST_FAILURE';
export const SITE_STATS_REQUEST_SUCCESS = 'SITE_STATS_REQUEST_SUCCESS';
export const SITE_VOUCHERS_ASSIGN_RECEIVE = 'SITE_VOUCHERS_ASSIGN_RECEIVE';
export const SITE_VOUCHERS_ASSIGN_REQUEST = 'SITE_VOUCHERS_ASSIGN_REQUEST';
export const SITE_VOUCHERS_ASSIGN_REQUEST_SUCCESS = 'SITE_VOUCHERS_ASSIGN_REQUEST_SUCCESS';
Expand Down
73 changes: 73 additions & 0 deletions client/state/stats/lists/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* External dependencies
*/
import omit from 'lodash/omit';

/**
* Internal dependencies
*/
import wpcom from 'lib/wp';
import {
SITE_STATS_RECEIVE,
SITE_STATS_REQUEST,
SITE_STATS_REQUEST_FAILURE,
SITE_STATS_REQUEST_SUCCESS
} from 'state/action-types';

/**
* Returns an action object to be used in signalling that stats for a given type of stats and query
* have been received.
*
* @param {Number} siteId Site ID
* @param {String} statType Stat Key
* @param {Object} query Stats query
* @param {Array} data Stat Data
* @return {Object} Action object
*/
export function receiveSiteStats( siteId, statType, query, data ) {
return {
type: SITE_STATS_RECEIVE,
statType,
siteId,
query,
data
};
}

/**
* Returns an action thunk which, when invoked, triggers a network request to
* retrieve site stats.
*
* @param {Number} siteId Site ID
* @param {String} statType Type of stats
* @param {Object} query Stats Query
* @return {Function} Action thunk
*/
export function requestSiteStats( siteId, statType, query ) {
return ( dispatch ) => {
dispatch( {
type: SITE_STATS_REQUEST,
statType,
siteId,
query
} );

Copy link
Contributor Author

@timmyc timmyc Jun 2, 2016

Choose a reason for hiding this comment

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

Currently in the stats-list we have support for undocumented endpoints here, and instead of copying that, I'd rather migrate those endpoints to wpcom.js as we update all of stats to use redux.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently in the stats-list we have support for undocumented endpoints here, and instead of copying that, I'd rather migrate those endpoints to wpcom.js as we update all of stats to use redux.

Would this be a blocker for merging this pull request? Or move forward with it despite being unusable with the assumption that it'll be added to wpcom.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a blocker at the moment. One of the undocumented endpoints is not even in use in Calypso. statsInsights is - but there is much work to do between here and there updating stats controller and components to get things working with Redux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return wpcom.site( siteId )[ statType ]( query ).then( data => {
dispatch( receiveSiteStats( siteId, statType, query, omit( data, '_headers' ) ) );
dispatch( {
type: SITE_STATS_REQUEST_SUCCESS,
statType,
siteId,
query
} );
} ).catch( error => {
dispatch( {
type: SITE_STATS_REQUEST_FAILURE,
statType,
siteId,
query,
error
} );
} );
};
}
86 changes: 86 additions & 0 deletions client/state/stats/lists/reducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* External dependencies
*/
import { combineReducers } from 'redux';
import merge from 'lodash/merge';

/**
* Internal dependencies
*/
import { isValidStateWithSchema } from 'state/utils';
import { getSerializedStatsQuery } from './utils';
import { itemSchema } from './schema';
import {
DESERIALIZE,
SERIALIZE,
SITE_STATS_RECEIVE,
SITE_STATS_REQUEST,
SITE_STATS_REQUEST_FAILURE,
SITE_STATS_REQUEST_SUCCESS,
} from 'state/action-types';

/**
* Returns the updated requests state after an action has been dispatched. The
* state maps site ID, post ID and stat keys to whether a request is in progress.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function requesting( state = {}, action ) {
switch ( action.type ) {
case SITE_STATS_REQUEST:
case SITE_STATS_REQUEST_SUCCESS:
case SITE_STATS_REQUEST_FAILURE:
const queryKey = getSerializedStatsQuery( action.query );
return merge( {}, state, {
[ action.siteId ]: {
[ action.statType ]: {
[ queryKey ]: SITE_STATS_REQUEST === action.type
}
}
} );

case SERIALIZE:
case DESERIALIZE:
return {};
}

return state;
}

/**
* Returns the updated items state after an action has been dispatched. The
* state maps site ID, statType and and serialized query key to the stat payload.
*
* @param {Object} state Current state
* @param {Object} action Action payload
* @return {Object} Updated state
*/
export function items( state = {}, action ) {
switch ( action.type ) {
case SITE_STATS_RECEIVE:
const queryKey = getSerializedStatsQuery( action.query );
return merge( {}, state, {
[ action.siteId ]: {
[ action.statType ]: {
[ queryKey ]: action.data
Copy link
Contributor

Choose a reason for hiding this comment

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

The behavior of merge here is a bit unnerving, since in my mind, action.data should entirely replace whatever data had existed on the key. However, if fields were missing, part of the original state would remain. It's probably a non-concern since we know the shape of the data we expect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand the concern, but in the context of stats, I think we always want the latest data from the API no matter what. Additionally if we stick with the selector/parser approach, some sanitization still happens at that layer.

}
}
} );

case DESERIALIZE:
if ( isValidStateWithSchema( state, itemSchema ) ) {
return state;
}

return {};
}

return state;
}

export default combineReducers( {
requesting,
items
} );
20 changes: 20 additions & 0 deletions client/state/stats/lists/schema.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
export const itemSchema = {
type: 'object',
patternProperties: {
'^\\d+$': {
type: 'object',
patternProperties: {
'^[A-Za-z]+$': {
type: 'object',
patternProperties: {
'^\\{[^\\}]*\\}$': {
type: 'object'
}
}
}
},
additionalProperties: false
}
},
additionalProperties: false
};
92 changes: 92 additions & 0 deletions client/state/stats/lists/selectors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/**
* External dependencies
*/
import get from 'lodash/get';

/**
* Internal dependencies
*/
import createSelector from 'lib/create-selector';
import i18n from 'lib/mixins/i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to use the moment from i18n, or could we import moment directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At one point in the distant past, we did do just that - but for some reason it was changed to utilize the mixin. I believe it was just a standards thing, but there may have been another reason for it that I just don't remember :)

import {
getSerializedStatsQuery
} from './utils';

/**
* Returns true if currently requesting stats for the statType and query combo, or false
* otherwise.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {String} statType Type of stat
* @param {Object} query Stats query object
* @return {Boolean} Whether stats are being requested
*/
export function isRequestingSiteStatsForQuery( state, siteId, statType, query ) {
const serializedQuery = getSerializedStatsQuery( query );
return !! get( state.stats.lists.requesting, [ siteId, statType, serializedQuery ] );
}

/**
* Returns object of stats data for the statType and query combo, or null if no stats have been
* received.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {String} statType Type of stat
* @param {Object} query Stats query object
* @return {?Object} Data for the query
*/
export function getSiteStatsForQuery( state, siteId, statType, query ) {
const serializedQuery = getSerializedStatsQuery( query );
return get( state.stats.lists.items, [ siteId, statType, serializedQuery ], null );
}

/**
* Returns a parsed object of statsStreak data for a given query, or default "empty" object
* if no statsStreak data has been received for that site.
*
* @param {Object} state Global state tree
* @param {Number} siteId Site ID
* @param {Object} query Stats query object
* @return {Object} Parsed Data for the query
*/
export const getParsedStreakDataForQuery = createSelector(
( state, siteId, query ) => {
const response = { days: {}, best: { day: 0, percent: 0 }, max: 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this would be a good opportunity to split the data we're constructing here into more granular selectors that can be composed together or used in more specific circumstances, e.g. getPostsStatsPerWeekday, getBestStatsWeekday, getPostsStatsWeekdayPercent, getStatsPostsWeekdayMax. Might also help readability of the code in splitting this into well-described functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this approach, and I think currently the only bit of data actually used on the component would be getPostsStatsStreak and getStatsPostsStreakMax.

I did notice you don't have ForQuery in any of those - too verbose?

Copy link
Contributor

Choose a reason for hiding this comment

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

I did notice you don't have ForQuery in any of those - too verbose?

Didn't leave the suffix out intentionally. I don't think I'd mind either way, depending on whether it helps clarify its meaning.

const days = [ 0, 0, 0, 0, 0, 0, 0 ];
let total = 0;
const data = getSiteStatsForQuery( state, siteId, 'statsStreak', query ) || [];

Object.keys( data ).forEach( ( timestamp ) => {
const postDay = i18n.moment.unix( timestamp );
const datestamp = postDay.format( 'YYYY-MM-DD' );
if ( 'undefined' === typeof( response.days[ datestamp ] ) ) {
response.days[ datestamp ] = 0;
}

response.days[ datestamp ] += data[ timestamp ];
days[ postDay.day() ] += data[ timestamp ];
total += data[ timestamp ];
} );

Object.keys( response.days ).forEach( ( datestamp ) => {
if ( response.days[ datestamp ] > response.max ) {
response.max = response.days[ datestamp ];
}
} );

const maxDay = Math.max.apply( null, days );
response.best.day = days.indexOf( maxDay );
if ( total ) {
response.best.percent = Math.round( 100 * ( maxDay / total ) );
}

return response;
},
( state, siteId, query ) => getSiteStatsForQuery( state, siteId, 'statsStreak', query ),
( state, siteId, taxonomy, query ) => {
const serializedQuery = getSerializedStatsQuery( query );
return [ siteId, 'statsStreak', serializedQuery ].join();
}
);
Loading