From 73034a20f0e7b564206b679aed0b3f60271f4928 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 13 Feb 2018 17:24:57 -0700 Subject: [PATCH] added support for configuring limiting, action optims --- Dockerfile.onbuild | 3 + .../src/constants/stream.js | 29 ++-- .../src/tabs/stream/containers/Comment.js | 7 +- .../src/tabs/stream/containers/Stream.js | 3 +- docs/_docs/02-02-advanced-configuration.md | 27 +++- graph/loaders/actions.js | 130 ++++++++++++++---- graph/resolvers/comment.js | 8 +- graph/resolvers/user.js | 4 +- services/actions.js | 109 ++------------- test/server/services/actions.js | 63 --------- webpack.config.js | 3 + 11 files changed, 180 insertions(+), 206 deletions(-) diff --git a/Dockerfile.onbuild b/Dockerfile.onbuild index dab06f336..5739d6f95 100644 --- a/Dockerfile.onbuild +++ b/Dockerfile.onbuild @@ -1,6 +1,9 @@ FROM coralproject/talk:latest # Setup the build arguments +ONBUILD ARG TALK_ADDTL_COMMENTS_ON_LOAD_MORE=10 +ONBUILD ARG TALK_ASSET_COMMENTS_LOAD_DEPTH=10 +ONBUILD ARG TALK_REPLY_COMMENTS_LOAD_DEPTH=3 ONBUILD ARG TALK_THREADING_LEVEL=3 ONBUILD ARG TALK_DEFAULT_STREAM_TAB=all ONBUILD ARG TALK_DEFAULT_LANG=en diff --git a/client/coral-embed-stream/src/constants/stream.js b/client/coral-embed-stream/src/constants/stream.js index 09d069851..64d4e64d3 100644 --- a/client/coral-embed-stream/src/constants/stream.js +++ b/client/coral-embed-stream/src/constants/stream.js @@ -1,14 +1,27 @@ +import defaultTo from 'lodash/defaultTo'; + const prefix = 'TALK_EMBED_STREAM'; -export const SET_ACTIVE_REPLY_BOX = 'SET_ACTIVE_REPLY_BOX'; -export const ADDTL_COMMENTS_ON_LOAD_MORE = 10; -export const VIEW_ALL_COMMENTS = 'VIEW_ALL_COMMENTS'; -export const VIEW_COMMENT = 'VIEW_COMMENT'; +export const ADDTL_COMMENTS_ON_LOAD_MORE = parseInt( + defaultTo(process.env.TALK_ADDTL_COMMENTS_ON_LOAD_MORE, '10') +); +export const ASSET_COMMENTS_LOAD_DEPTH = parseInt( + defaultTo(process.env.TALK_ASSET_COMMENTS_LOAD_DEPTH, '10') +); +export const REPLY_COMMENTS_LOAD_DEPTH = parseInt( + defaultTo(process.env.TALK_REPLY_COMMENTS_LOAD_DEPTH, '3') +); +export const THREADING_LEVEL = parseInt( + defaultTo(process.env.TALK_THREADING_LEVEL, '3') +); + +export const ADD_COMMENT_BOX_TAG = `${prefix}_COMMENT_BOX_ADD_TAG`; export const ADD_COMMENT_CLASSNAME = 'ADD_COMMENT_CLASSNAME'; +export const CLEAR_COMMENT_BOX_TAGS = `${prefix}_COMMENT_BOX_CLEAR_TAGS`; +export const REMOVE_COMMENT_BOX_TAG = `${prefix}_COMMENT_BOX_REMOVE_TAG`; export const REMOVE_COMMENT_CLASSNAME = 'REMOVE_COMMENT_CLASSNAME'; -export const THREADING_LEVEL = process.env.TALK_THREADING_LEVEL; +export const SET_ACTIVE_REPLY_BOX = 'SET_ACTIVE_REPLY_BOX'; export const SET_ACTIVE_TAB = 'CORAL_STREAM_SET_ACTIVE_TAB'; export const SET_SORT = 'CORAL_STREAM_SET_SORT'; -export const ADD_COMMENT_BOX_TAG = `${prefix}_COMMENT_BOX_ADD_TAG`; -export const REMOVE_COMMENT_BOX_TAG = `${prefix}_COMMENT_BOX_REMOVE_TAG`; -export const CLEAR_COMMENT_BOX_TAGS = `${prefix}_COMMENT_BOX_CLEAR_TAGS`; +export const VIEW_ALL_COMMENTS = 'VIEW_ALL_COMMENTS'; +export const VIEW_COMMENT = 'VIEW_COMMENT'; diff --git a/client/coral-embed-stream/src/tabs/stream/containers/Comment.js b/client/coral-embed-stream/src/tabs/stream/containers/Comment.js index 9ed421273..5a6c332e0 100644 --- a/client/coral-embed-stream/src/tabs/stream/containers/Comment.js +++ b/client/coral-embed-stream/src/tabs/stream/containers/Comment.js @@ -4,7 +4,10 @@ import Comment from '../components/Comment'; import { withFragments } from 'coral-framework/hocs'; import { getSlotFragmentSpreads } from 'coral-framework/utils'; import { withSetCommentStatus } from 'coral-framework/graphql/mutations'; -import { THREADING_LEVEL } from '../../../constants/stream'; +import { + THREADING_LEVEL, + REPLY_COMMENTS_LOAD_DEPTH, +} from '../../../constants/stream'; import hoistStatics from 'recompose/hoistStatics'; import { nest } from '../../../graphql/utils'; @@ -118,7 +121,7 @@ const withCommentFragments = withFragments({ ...CoralEmbedStream_Comment_SingleComment ${nest( ` - replies(query: {limit: 3, excludeIgnored: $excludeIgnored}) { + replies(query: {limit: ${REPLY_COMMENTS_LOAD_DEPTH}, excludeIgnored: $excludeIgnored}) { nodes { ...CoralEmbedStream_Comment_SingleComment ...nest diff --git a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js index 51b4d9d9e..d69966dc3 100644 --- a/client/coral-embed-stream/src/tabs/stream/containers/Stream.js +++ b/client/coral-embed-stream/src/tabs/stream/containers/Stream.js @@ -4,6 +4,7 @@ import { connect } from 'react-redux'; import { bindActionCreators } from 'redux'; import { ADDTL_COMMENTS_ON_LOAD_MORE, + ASSET_COMMENTS_LOAD_DEPTH, THREADING_LEVEL, } from '../../../constants/stream'; import { @@ -379,7 +380,7 @@ const fragments = { requireEmailConfirmation } totalCommentCount @skip(if: $hasComment) - comments(query: {limit: 10, excludeIgnored: $excludeIgnored, sortOrder: $sortOrder, sortBy: $sortBy}) @skip(if: $hasComment) { + comments(query: {limit: ${ASSET_COMMENTS_LOAD_DEPTH}, excludeIgnored: $excludeIgnored, sortOrder: $sortOrder, sortBy: $sortBy}) @skip(if: $hasComment) { nodes { ...CoralEmbedStream_Stream_comment } diff --git a/docs/_docs/02-02-advanced-configuration.md b/docs/_docs/02-02-advanced-configuration.md index fb083f63c..7d2ea81df 100644 --- a/docs/_docs/02-02-advanced-configuration.md +++ b/docs/_docs/02-02-advanced-configuration.md @@ -492,4 +492,29 @@ tracing of GraphQL requests. Setting this to `TRUE` will allow queries to execute without a limit (returns all documents). This introduces a significant performance regression, and should -be used with caution. (Default `FALSE`) \ No newline at end of file +be used with caution. (Default `FALSE`) + + +## TALK_ADDTL_COMMENTS_ON_LOAD_MORE + +This is a **Build Variable** and must be consumed during build. If using the +[Docker-onbuild]({{ "/installation-from-docker/#onbuild" | relative_url }}) +image you can specify it with `--build-arg TALK_ADDTL_COMMENTS_ON_LOAD_MORE=10`. + +Specifies the number of additional comments to load when a user clicks `Load More`. (Default `10`) + +## TALK_ASSET_COMMENTS_LOAD_DEPTH + +This is a **Build Variable** and must be consumed during build. If using the +[Docker-onbuild]({{ "/installation-from-docker/#onbuild" | relative_url }}) +image you can specify it with `--build-arg TALK_ASSET_COMMENTS_LOAD_DEPTH=10`. + +Specifies the initial number of comments to load for an asset. (Default `10`) + +## TALK_REPLY_COMMENTS_LOAD_DEPTH + +This is a **Build Variable** and must be consumed during build. If using the +[Docker-onbuild]({{ "/installation-from-docker/#onbuild" | relative_url }}) +image you can specify it with `--build-arg TALK_REPLY_COMMENTS_LOAD_DEPTH=3`. + +Specifies the initial replies to load for a comment. (Default `3`) \ No newline at end of file diff --git a/graph/loaders/actions.js b/graph/loaders/actions.js index 96e45b0e3..2a501b6c6 100644 --- a/graph/loaders/actions.js +++ b/graph/loaders/actions.js @@ -1,9 +1,7 @@ const DataLoader = require('dataloader'); - const util = require('./util'); - const ActionsService = require('../../services/actions'); -const ActionModel = require('../../models/action'); +const { first, get, remove, groupBy, reduce, defaultTo } = require('lodash'); /** * Gets actions based on their item id's. @@ -15,40 +13,122 @@ const genActionsByItemID = (_, item_ids) => { }; /** - * Looks up actions based on the requested id's all bounded by the user. - * @param {Object} context the context of the request - * @param {Array} ids array of id's to get - * @return {Promise} resolves to the promises of the requested actions + * Looks up the actions for each of the items. + * + * @param {Object} ctx the graph context of the request + * @param {Array} itemIDs the items that we need to get the actions for */ -const genActionSummariessByItemID = ({ user = {} }, item_ids) => { - return ActionsService.getActionSummaries(item_ids, user.id).then( - util.arrayJoinBy(item_ids, 'item_id') +const genActionsAuthoredWithID = ({ user = {} }, itemIDs) => + ActionsService.getUserActions(user.id, itemIDs).then( + util.arrayJoinBy(itemIDs, 'item_id') ); -}; /** - * Search for actions based on their action_type and item_type and ensures that - * the actions returned have unique item id's. - * @param {String} action_type the action to search by - * @param {String} item_type the item id to search by - * @return {Promise} resolves to distinct items actions + * Looks up the action summaries for a set of items. + * + * @param {Object} ctx the graph context of the request + * @param {Array} items the items that should have their items looked up for */ -const getItemIdsByActionTypeAndItemType = (_, action_type, item_type) => { - return ActionModel.distinct('item_id', { action_type, item_type }); +const genActionSummariesByItem = async (ctx, items) => { + const { loaders: { Actions } } = ctx; + + // This is designed to match the action_counts value that is embedded on + // documents which cache action counts. For users that are not logged in, we + // don't need to hit the actions collection at all! + + // This will match any action count that is specific for a group id. + const nonGroupIDTest = /^([A-Z]+)_([A-Z_]+)$/; + + // We will literate over all the items that we're comparing. + return items.map(async ({ id, action_counts = {} }) => { + // Cache all those entries for which we got the group id of, because we + // don't want to include them twice. + const groupIDCache = {}; + + // Possibly get the list of user actions completed by the user. This will be + // used later to join together with the action summaries to provide context. + const userActions = + ctx.user && reduce(action_counts, (total, count) => total + count, 0) > 0 + ? await Actions.getAuthoredByID.load(id) + : []; + + // Group the user actions in the same way that the action counts are + // grouped. This will let us extract it easy. + const groupedUserActions = groupBy( + userActions, + ({ action_type, group_id }) => + (group_id ? `${action_type}_${group_id}` : action_type).toUpperCase() + ); + + // Generate the action summaries for the item. + return Object.keys(action_counts) + .map(action_type => ({ + count: action_counts[action_type], + action_type: action_type.toUpperCase(), + })) + .reduce((actionTypeList, { count, action_type }) => { + // Get the current user's actions (if they have any). + const current_user = defaultTo( + first(get(groupedUserActions, action_type, [])), + null + ); + + // Check to see if this is a action without a corresponding group id. + if (nonGroupIDTest.test(action_type)) { + // This action type does have a group id associated with it. + const results = nonGroupIDTest.exec(action_type); + const groupActionType = results[1]; + const groupID = results[2]; + + // Purge out the summary if it already exists, and mark that this + // group id has been found so we don't include it in the future. + remove( + actionTypeList, + ({ action_type }) => action_type === groupActionType + ); + groupIDCache[groupActionType] = true; + + // Push the new entry in. + actionTypeList.push({ + action_type: groupActionType, + group_id: groupID, + count, + current_user, + }); + } else { + // This does not have a group id. Check to see if this group id + // already has an specific (group id) entry. + if (groupIDCache[action_type]) { + // It does. Don't add anything. + return actionTypeList; + } + + // It does not, add the entry. + actionTypeList.push({ + action_type, + group_id: null, + count, + current_user, + }); + } + + return actionTypeList; + }, []); + }); }; /** * Creates a set of loaders based on a GraphQL context. - * @param {Object} context the context of the GraphQL request + * @param {Object} ctx the context of the GraphQL request * @return {Object} object of loaders */ -module.exports = context => ({ +module.exports = ctx => ({ Actions: { - getByID: new DataLoader(ids => genActionsByItemID(context, ids)), - getSummariesByItemID: new DataLoader(ids => - genActionSummariessByItemID(context, ids) + getByID: new DataLoader(ids => genActionsByItemID(ctx, ids)), + getSummariesByItem: new DataLoader( + items => genActionSummariesByItem(ctx, items), + { cacheKeyFn: ({ id }) => id } ), - getByTypes: ({ action_type, item_type }) => - getItemIdsByActionTypeAndItemType(context, action_type, item_type), + getAuthoredByID: new DataLoader(ids => genActionsAuthoredWithID(ctx, ids)), }, }); diff --git a/graph/resolvers/comment.js b/graph/resolvers/comment.js index 9949a5e2e..37594ce49 100644 --- a/graph/resolvers/comment.js +++ b/graph/resolvers/comment.js @@ -40,12 +40,12 @@ const Comment = { return Actions.getByID.load(id); }, - action_summaries({ id, action_summaries }, _, { loaders: { Actions } }) { - if (action_summaries) { - return action_summaries; + action_summaries(comment, _, { loaders: { Actions } }) { + if (comment.action_summaries) { + return comment.action_summaries; } - return Actions.getSummariesByItemID.load(id); + return Actions.getSummariesByItem.load(comment); }, asset({ asset_id }, _, { loaders: { Assets } }) { return Assets.getByID.load(asset_id); diff --git a/graph/resolvers/user.js b/graph/resolvers/user.js index 11db4c366..c72a5923c 100644 --- a/graph/resolvers/user.js +++ b/graph/resolvers/user.js @@ -10,8 +10,8 @@ const { } = require('../../perms/constants'); const User = { - action_summaries({ id }, _, { loaders: { Actions } }) { - return Actions.getSummariesByItemID.load(id); + action_summaries(user, _, { loaders: { Actions } }) { + return Actions.getSummariesByItem.load(user); }, actions({ id }, _, { user, loaders: { Actions } }) { // Only return the actions if the user is not an admin. diff --git a/services/actions.js b/services/actions.js index a1cef9ff6..055a812f8 100644 --- a/services/actions.js +++ b/services/actions.js @@ -104,110 +104,19 @@ module.exports = class ActionsService { } /** - * Fetches the action summaries for the given asset, and comments around the - * given user id. + * Get the actions for a specific user on the specific items. * - * @param {[type]} asset_id [description] - * @param {[type]} comments [description] - * @param {String} [current_user_id=''] [description] - * @return {[type]} [description] + * @param {String} userID the id of the user to find their actions for + * @param {Array} itemIDs the ids of the items to find their actions + * for */ - static getActionSummariesFromComments( - asset_id = '', - comments, - current_user_id = '' - ) { - // Get the user id's from the author id's as a unique array that gets - // sorted. - let userIDs = _.uniq(comments.map(comment => comment.author_id)).sort(); - - // Fetch the actions for pretty much everything at this point. - return ActionsService.getActionSummaries( - _.uniq( - [ - // Actions can be on assets... - asset_id, - - // Comments... - ...comments.map(comment => comment.id), - - // Or Authors... - ...userIDs, - ].filter(e => e) - ), - current_user_id - ); - } - - /** - * Returns summaries of actions for an array of ids. - * - * @param {String} ids array of user identifiers (uuid) - */ - static getActionSummaries(item_ids, current_user_id = '') { - // only grab items that match the specified item id's - let $match = { + static getUserActions(userID, itemIDs) { + return ActionModel.find({ + user_id: userID, item_id: { - $in: item_ids, + $in: itemIDs, }, - }; - - let $group = { - // group unique documents by these properties, we are leveraging the - // fact that each uuid is completely unique. - _id: { - item_id: '$item_id', - action_type: '$action_type', - group_id: '$group_id', - }, - - // and sum up all actions matching the above grouping criteria - count: { - $sum: 1, - }, - - // we are leveraging the fact that each uuid is completely unique and - // just grabbing the last instance of the item type here. - item_type: { - $first: '$item_type', - }, - - current_user: { - $max: { - $cond: { - if: { - $eq: ['$user_id', current_user_id], - }, - then: '$$CURRENT', - else: null, - }, - }, - }, - }; - - let $project = { - // suppress the _id field - _id: false, - - // map the fields from the _id grouping down a level - item_id: '$_id.item_id', - action_type: '$_id.action_type', - group_id: '$_id.group_id', - - // map the field directly - count: '$count', - item_type: '$item_type', - - // set the current user to false here - current_user: '$current_user', - }; - - return ActionModel.aggregate([ - { $match }, - { $group }, - { $project }, - { $sort: { action_type: 1, group_id: 1 } }, - ]); + }); } /** diff --git a/test/server/services/actions.js b/test/server/services/actions.js index d1cab11bd..96170338e 100644 --- a/test/server/services/actions.js +++ b/test/server/services/actions.js @@ -151,67 +151,4 @@ describe('services.ActionsService', () => { ); }); }); - - describe('#getActionSummaries()', () => { - it('should return properly formatted summaries from an array of item_ids', () => { - return ActionsService.getActionSummaries([comment.id, '789']).then( - summaries => { - expect(summaries).to.have.length(2); - - expect(summaries).to.deep.include({ - action_type: 'LIKE', - count: 1, - item_id: comment.id, - item_type: 'COMMENTS', - current_user: null, - }); - - expect(summaries).to.deep.include({ - action_type: 'FLAG', - count: 2, - item_id: comment.id, - item_type: 'COMMENTS', - current_user: null, - }); - } - ); - }); - - it('should include a current user when one is passed', () => { - return ActionsService.getActionSummaries( - [comment.id], - 'flagginguserid' - ).then(summaries => { - expect(summaries).to.have.length(2); - - let summary = summaries.find( - s => s.item_id === comment.id && s.action_type === 'FLAG' - ); - - expect(summary).to.not.be.undefined; - expect(summary.current_user).to.not.be.null; - expect(summary.current_user).to.have.property('item_id', comment.id); - expect(summary.current_user).to.have.property('item_type', 'COMMENTS'); - expect(summary.current_user).to.have.property( - 'user_id', - 'flagginguserid' - ); - expect(summary.current_user).to.have.property('action_type', 'FLAG'); - }); - }); - - it("should not include a current user when one is passed for a user that doesn't have an action", () => { - return ActionsService.getActionSummaries( - [comment.id], - 'flagginguserid2' - ).then(summaries => { - expect(summaries).to.have.length(2); - - summaries.forEach(summary => { - expect(summary).to.not.be.undefined; - expect(summary).to.have.property('current_user', null); - }); - }); - }); - }); }); diff --git a/webpack.config.js b/webpack.config.js index 6a018caba..124766686 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -128,6 +128,9 @@ const config = { new webpack.EnvironmentPlugin({ TALK_PLUGINS_JSON: '{}', TALK_THREADING_LEVEL: '3', + TALK_ADDTL_COMMENTS_ON_LOAD_MORE: '10', + TALK_ASSET_COMMENTS_LOAD_DEPTH: '10', + TALK_REPLY_COMMENTS_LOAD_DEPTH: '3', TALK_DEFAULT_STREAM_TAB: 'all', TALK_DEFAULT_LANG: 'en', }),