From 74a2ed4a4141cb322321d01371049169d3dedb6b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 7 Feb 2018 13:29:59 -0700 Subject: [PATCH 1/6] introduce no limit --- config.js | 4 + docs/_docs/02-02-advanced-configuration.md | 8 +- graph/loaders/comments.js | 87 +++++++++++----------- 3 files changed, 55 insertions(+), 44 deletions(-) diff --git a/config.js b/config.js index dd9aed13c..9b7c7adcc 100644 --- a/config.js +++ b/config.js @@ -44,6 +44,10 @@ const CONFIG = { // fetching again. SETTINGS_CACHE_TIME: ms(process.env.TALK_SETTINGS_CACHE_TIME || '1hr'), + // ALLOW_NO_LIMIT_QUERIES enables some queries to specify a limit of -1 to + // request all of the records. Otherwise, minimum limits of 0 are enforced. + ALLOW_NO_LIMIT_QUERIES: process.env.TALK_ALLOW_NO_LIMIT_QUERIES === 'TRUE', + //------------------------------------------------------------------------------ // JWT based configuration //------------------------------------------------------------------------------ diff --git a/docs/_docs/02-02-advanced-configuration.md b/docs/_docs/02-02-advanced-configuration.md index e985f8f20..fb083f63c 100644 --- a/docs/_docs/02-02-advanced-configuration.md +++ b/docs/_docs/02-02-advanced-configuration.md @@ -486,4 +486,10 @@ Used to set the key for use with [Apollo Engine](https://www.apollographql.com/engine/){:target="_blank"} for tracing of GraphQL requests. -**Note: Apollo Engine is a premium service, charges may apply.** \ No newline at end of file +**Note: Apollo Engine is a premium service, charges may apply.** + +## ALLOW_NO_LIMIT_QUERIES + +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 diff --git a/graph/loaders/comments.js b/graph/loaders/comments.js index 7f0dbea00..37d40a219 100644 --- a/graph/loaders/comments.js +++ b/graph/loaders/comments.js @@ -4,7 +4,10 @@ const { SEARCH_NON_NULL_OR_ACCEPTED_COMMENTS, SEARCH_OTHERS_COMMENTS, } = require('../../perms/constants'); -const { CACHE_EXPIRY_COMMENT_COUNT } = require('../../config'); +const { + CACHE_EXPIRY_COMMENT_COUNT, + ALLOW_NO_LIMIT_QUERIES, +} = require('../../config'); const ms = require('ms'); const sc = require('snake-case'); @@ -148,12 +151,11 @@ const getCommentCountByQuery = (ctx, options) => { * @param {Object} params the params from the client describing the query */ const getStartCursor = (ctx, nodes, { cursor, sortBy }) => { - switch (sortBy) { - case 'CREATED_AT': - return nodes.length ? nodes[0].created_at : null; - case 'REPLIES': - // The cursor is the start! This is using numeric pagination. - return cursor != null ? cursor : 0; + if (sortBy === 'CREATED_AT') { + return nodes.length ? nodes[0].created_at : null; + } else if (sortBy === 'REPLIES') { + // The cursor is the start! This is using numeric pagination. + return cursor != null ? cursor : 0; } const SORT_KEY = sortBy.toLowerCase(); @@ -181,11 +183,10 @@ const getStartCursor = (ctx, nodes, { cursor, sortBy }) => { * @param {Object} params the params from the client describing the query */ const getEndCursor = (ctx, nodes, { cursor, sortBy }) => { - switch (sortBy) { - case 'CREATED_AT': - return nodes.length ? nodes[nodes.length - 1].created_at : null; - case 'REPLIES': - return nodes.length ? (cursor != null ? cursor : 0) + nodes.length : null; + if (sortBy === 'CREATED_AT') { + return nodes.length ? nodes[nodes.length - 1].created_at : null; + } else if (sortBy === 'REPLIES') { + return nodes.length ? (cursor != null ? cursor : 0) + nodes.length : null; } const SORT_KEY = sortBy.toLowerCase(); @@ -212,36 +213,33 @@ const getEndCursor = (ctx, nodes, { cursor, sortBy }) => { * @param {Object} params the params from the client describing the query */ const applySort = (ctx, query, { cursor, sortOrder, sortBy }) => { - switch (sortBy) { - case 'CREATED_AT': { - if (cursor) { - if (sortOrder === 'DESC') { - query = query.where({ - created_at: { - $lt: cursor, - }, - }); - } else { - query = query.where({ - created_at: { - $gt: cursor, - }, - }); - } + if (sortBy === 'CREATED_AT') { + if (cursor) { + if (sortOrder === 'DESC') { + query = query.where({ + created_at: { + $lt: cursor, + }, + }); + } else { + query = query.where({ + created_at: { + $gt: cursor, + }, + }); } - - return query.sort({ created_at: sortOrder === 'DESC' ? -1 : 1 }); } - case 'REPLIES': { - if (cursor) { - query = query.skip(cursor); - } - return query.sort({ - reply_count: sortOrder === 'DESC' ? -1 : 1, - created_at: sortOrder === 'DESC' ? -1 : 1, - }); + return query.sort({ created_at: sortOrder === 'DESC' ? -1 : 1 }); + } else if (sortBy === 'REPLIES') { + if (cursor) { + query = query.skip(cursor); } + + return query.sort({ + reply_count: sortOrder === 'DESC' ? -1 : 1, + created_at: sortOrder === 'DESC' ? -1 : 1, + }); } const SORT_KEY = sortBy.toLowerCase(); @@ -280,7 +278,7 @@ const executeWithSort = async ( query = applySort(ctx, query, { cursor, sortOrder, sortBy }); // Apply the limit (if it exists, as it's applied universally). - if (limit) { + if (limit >= 0) { query = query.limit(limit + 1); } @@ -290,7 +288,7 @@ const executeWithSort = async ( // The hasNextPage is always handled the same (ask for one more than we need, // if there is one more, than there is more). let hasNextPage = false; - if (limit && nodes.length > limit) { + if (limit >= 0 && nodes.length > limit) { // There was one more than we expected! Set hasNextPage = true and remove // the last item from the array that we requested. hasNextPage = true; @@ -302,11 +300,9 @@ const executeWithSort = async ( return { startCursor: getStartCursor(ctx, nodes, { cursor, - sortOrder, sortBy, - limit, }), - endCursor: getEndCursor(ctx, nodes, { cursor, sortOrder, sortBy, limit }), + endCursor: getEndCursor(ctx, nodes, { cursor, sortBy }), hasNextPage, nodes, }; @@ -338,6 +334,11 @@ const getCommentsByQuery = async ( ) => { let comments = CommentModel.find(); + // Enforce that the limit must be gte 0 if this option is not true. + if (!ALLOW_NO_LIMIT_QUERIES && limit < 0) { + throw new Error('cannot query for limit < 0'); + } + // If user queries for statuses other than NONE and/or ACCEPTED statuses, it needs // special privileges. if ( From 73034a20f0e7b564206b679aed0b3f60271f4928 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 13 Feb 2018 17:24:57 -0700 Subject: [PATCH 2/6] 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', }), From 1e329c8967ffb0ecfb9dca00e20c1f68adc08d94 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 13 Feb 2018 17:45:26 -0700 Subject: [PATCH 3/6] code cleanup --- graph/loaders/actions.js | 205 +++++++++++++++++++++++---------------- 1 file changed, 124 insertions(+), 81 deletions(-) diff --git a/graph/loaders/actions.js b/graph/loaders/actions.js index 2a501b6c6..6e8c42355 100644 --- a/graph/loaders/actions.js +++ b/graph/loaders/actions.js @@ -1,7 +1,7 @@ const DataLoader = require('dataloader'); const util = require('./util'); const ActionsService = require('../../services/actions'); -const { first, get, remove, groupBy, reduce, defaultTo } = require('lodash'); +const { first, get, merge, remove, groupBy, reduce } = require('lodash'); /** * Gets actions based on their item id's. @@ -23,6 +23,128 @@ const genActionsAuthoredWithID = ({ user = {} }, itemIDs) => util.arrayJoinBy(itemIDs, 'item_id') ); +/** + * iterateActionCounts will create an iterable object that can be used to + * compute action summaries. + * + * @param {Object} action_counts the action count object + */ +const iterateActionCounts = action_counts => + Object.keys(action_counts).map(action_type => ({ + count: action_counts[action_type], + action_type: action_type.toUpperCase(), + })); + +/** + * getUserActions will get the actions made by the user for this specific + * item. + * + * @param {Object} ctx the graph context of the request + * @param {Object} item the item that we're getting the actions for + */ +async function getUserActions(ctx, { action_counts, id }) { + const { loaders: { Actions } } = ctx; + + // Get the total count for all action types. + const totalActionCount = reduce( + action_counts, + (total, count) => total + count, + 0 + ); + + // Check to see if there are any user actions to get. + const hasUserActions = ctx.user && totalActionCount > 0; + if (!hasUserActions) { + return {}; + } + + // 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 = await Actions.getAuthoredByID.load(id); + if (userActions.length === 0) { + return {}; + } + + // Group the user actions in the same way that the action counts are + // grouped. This will let us extract it easy. + return reduce( + groupBy(userActions, ({ action_type, group_id }) => + (group_id ? `${action_type}_${group_id}` : action_type).toUpperCase() + ), + (allUserActions, userActions, actionType) => + merge(allUserActions, { [actionType]: first(userActions) }), + {} + ); +} + +// This will match any action count that is specific for a group id. +const nonGroupIDTest = /^([A-Z]+)_([A-Z_]+)$/; + +/** + * resolveActionSummariesForItem will resolve the action summaries for an item. + * + * @param {Object} ctx the graph context of the request + * @param {Object} item the item that we are resolving an action summary for + */ +async function resolveActionSummariesForItem(ctx, { 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 = {}; + + // Get the user actions for this specific item. + const groupedUserActions = await getUserActions(ctx, { id, action_counts }); + + // Generate the action summaries for the item. + return iterateActionCounts(action_counts).reduce( + (actionTypeList, { count, action_type }) => { + // Get the current user's actions (if they have any). + const current_user = 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; + }, + [] + ); +} + /** * Looks up the action summaries for a set of items. * @@ -30,91 +152,12 @@ const genActionsAuthoredWithID = ({ user = {} }, itemIDs) => * @param {Array} items the items that should have their items looked up for */ 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; - }, []); - }); + return items.map(resolveActionSummariesForItem); }; /** From 9820e575b271299da4a918b253baebdcc6959d9b Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Tue, 13 Feb 2018 18:29:07 -0700 Subject: [PATCH 4/6] patch --- graph/loaders/actions.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graph/loaders/actions.js b/graph/loaders/actions.js index 6e8c42355..99aaa6015 100644 --- a/graph/loaders/actions.js +++ b/graph/loaders/actions.js @@ -157,7 +157,7 @@ const genActionSummariesByItem = async (ctx, items) => { // don't need to hit the actions collection at all! // We will literate over all the items that we're comparing. - return items.map(resolveActionSummariesForItem); + return items.map(item => resolveActionSummariesForItem(ctx, item)); }; /** From 18710e2345c5cfbb9cfe7c0b2d981a265d8dd6f5 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 14 Feb 2018 15:56:04 -0700 Subject: [PATCH 5/6] added tests --- graph/loaders/actions.js | 15 ++-- test/server/graph/loaders/actions.js | 122 +++++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 5 deletions(-) create mode 100644 test/server/graph/loaders/actions.js diff --git a/graph/loaders/actions.js b/graph/loaders/actions.js index 99aaa6015..b5ea31a3f 100644 --- a/graph/loaders/actions.js +++ b/graph/loaders/actions.js @@ -1,13 +1,15 @@ const DataLoader = require('dataloader'); const util = require('./util'); -const ActionsService = require('../../services/actions'); const { first, get, merge, remove, groupBy, reduce } = require('lodash'); /** * Gets actions based on their item id's. */ -const genActionsByItemID = (_, item_ids) => { - return ActionsService.findByItemIdArray(item_ids).then( +const genActionsByItemID = ( + { connectors: { services: { Actions } } }, + item_ids +) => { + return Actions.findByItemIdArray(item_ids).then( util.arrayJoinBy(item_ids, 'item_id') ); }; @@ -18,8 +20,11 @@ const genActionsByItemID = (_, item_ids) => { * @param {Object} ctx the graph context of the request * @param {Array} itemIDs the items that we need to get the actions for */ -const genActionsAuthoredWithID = ({ user = {} }, itemIDs) => - ActionsService.getUserActions(user.id, itemIDs).then( +const genActionsAuthoredWithID = ( + { user = {}, connectors: { services: { Actions } } }, + itemIDs +) => + Actions.getUserActions(user.id, itemIDs).then( util.arrayJoinBy(itemIDs, 'item_id') ); diff --git a/test/server/graph/loaders/actions.js b/test/server/graph/loaders/actions.js new file mode 100644 index 000000000..f8e96175b --- /dev/null +++ b/test/server/graph/loaders/actions.js @@ -0,0 +1,122 @@ +const chai = require('chai'); +chai.use(require('chai-as-promised')); +const { expect } = chai; +const sinon = require('sinon'); +const { find } = require('lodash'); +const loaders = require('../../../../graph/loaders/actions'); + +describe('graph.loaders.Actions', () => { + describe('#getAuthoredByID', () => { + it('loads the correct entries', async () => { + const spy = sinon.spy(async () => [ + { item_id: 'comment_1' }, + { item_id: 'comment_2' }, + ]); + const { Actions: { getAuthoredByID } } = loaders({ + user: { id: 'user_1' }, + connectors: { services: { Actions: { getUserActions: spy } } }, + }); + + const actions = await getAuthoredByID.loadMany([ + 'comment_2', + 'comment_1', + ]); + + expect(spy.calledWith('user_1', ['comment_2', 'comment_1'])); + expect(actions).to.have.length(2); + expect(actions[0]).to.have.length(1); + expect(actions[0][0]).to.have.property('item_id', 'comment_2'); + expect(actions[1]).to.have.length(1); + expect(actions[1][0]).to.have.property('item_id', 'comment_1'); + }); + }); + + describe('#getSummariesByItem', () => { + describe('logged out user', () => { + it('does not include any user data', async () => { + const { Actions: { getSummariesByItem } } = loaders({ + loaders: { + Actions: { + getAuthoredByID: { + load: () => Promise.reject(new Error('should not be called')), + }, + }, + }, + user: null, + }); + + const summaries = await getSummariesByItem.load({ + id: '1', + action_counts: { flag: 1, flag_comment_offensive: 1, respect: 2 }, + }); + + expect(summaries).to.have.length(2); + + const flag = find(summaries, { action_type: 'FLAG' }); + expect(flag).to.be.defined; + + expect(flag).to.have.property('current_user', null); + expect(flag).to.have.property('action_type', 'FLAG'); + expect(flag).to.have.property('group_id', 'COMMENT_OFFENSIVE'); + expect(flag).to.have.property('count', 1); + + const respect = find(summaries, { action_type: 'RESPECT' }); + expect(respect).to.be.defined; + + expect(respect).to.have.property('current_user', null); + expect(respect).to.have.property('action_type', 'RESPECT'); + expect(respect).to.have.property('group_id', null); + expect(respect).to.have.property('count', 2); + }); + }); + + describe('logged in user', () => { + it('does include user', async () => { + const { Actions: { getSummariesByItem } } = loaders({ + loaders: { + Actions: { + getAuthoredByID: { + load: commentID => { + expect(commentID).to.equal('comment_1'); + return [ + { + id: 'action_1', + action_type: 'FLAG', + group_id: 'COMMENT_OFFENSIVE', + }, + ]; + }, + }, + }, + }, + user: { id: 'user_1' }, + }); + + const summaries = await getSummariesByItem.load({ + id: 'comment_1', + action_counts: { flag: 1, flag_comment_offensive: 1, respect: 2 }, + }); + + expect(summaries).to.have.length(2); + + const flag = find(summaries, { action_type: 'FLAG' }); + expect(flag).to.be.defined; + + expect(flag).to.have.property('action_type', 'FLAG'); + expect(flag).to.have.property('group_id', 'COMMENT_OFFENSIVE'); + expect(flag).to.have.property('count', 1); + + expect(flag).to.have.property('current_user').not.null; + expect(flag.current_user).to.have.property('id', 'action_1'); + + const respect = find(summaries, { action_type: 'RESPECT' }); + expect(respect).to.be.defined; + + expect(respect).to.have.property('current_user', null); + expect(respect).to.have.property('action_type', 'RESPECT'); + expect(respect).to.have.property('group_id', null); + expect(respect).to.have.property('count', 2); + }); + }); + }); +}); From 0519b55707e74e17e0a1897aa30f335bd0f87139 Mon Sep 17 00:00:00 2001 From: Kim Gardner Date: Thu, 15 Feb 2018 15:55:51 -0500 Subject: [PATCH 6/6] Translation typo is breaking Akismet --- plugins/talk-plugin-akismet/client/translations.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/talk-plugin-akismet/client/translations.yml b/plugins/talk-plugin-akismet/client/translations.yml index a21f4b3c5..6cc19a492 100644 --- a/plugins/talk-plugin-akismet/client/translations.yml +++ b/plugins/talk-plugin-akismet/client/translations.yml @@ -68,7 +68,7 @@ nl_NL: spam_comment: "Spam" detected: "Gedetecteerd door Akismet" still_spam: | - Dank je wel. Ons moderatieteam zal je reactie beoordelen. + Dank je wel. Ons moderatieteam zal je reactie beoordelen. flags: reasons: comment: