diff --git a/bin/cli-settings b/bin/cli-settings index 72f42672d..05c7f2888 100755 --- a/bin/cli-settings +++ b/bin/cli-settings @@ -18,7 +18,7 @@ async function changeOrgName() { await cache.init(); // Get the original settings. - const settings = await Settings.retrieve('organizationName'); + const settings = await Settings.select('organizationName'); const { organizationName } = await inquirer.prompt([ { diff --git a/client/coral-admin/src/routes/Moderation/containers/Moderation.js b/client/coral-admin/src/routes/Moderation/containers/Moderation.js index f35255732..c908404cb 100644 --- a/client/coral-admin/src/routes/Moderation/containers/Moderation.js +++ b/client/coral-admin/src/routes/Moderation/containers/Moderation.js @@ -432,6 +432,7 @@ const withModQueueQuery = withQuery( ${Object.keys(queueConfig).map( queue => ` ${queue}: comments(query: { + excludeDeleted: true, statuses: ${ queueConfig[queue].statuses ? `[${queueConfig[queue].statuses.join(', ')}],` @@ -458,6 +459,7 @@ const withModQueueQuery = withQuery( ${Object.keys(queueConfig).map( queue => ` ${queue}Count: commentCount(query: { + excludeDeleted: true, statuses: ${ queueConfig[queue].statuses ? `[${queueConfig[queue].statuses.join(', ')}],` diff --git a/graph/loaders/assets.js b/graph/loaders/assets.js index 40305760c..868c8af68 100644 --- a/graph/loaders/assets.js +++ b/graph/loaders/assets.js @@ -71,7 +71,7 @@ const findOrCreateAssetByURL = async (ctx, url) => { // Check for whitelisting + get the settings at the same time. const [whitelisted, settings] = await Promise.all([ DomainList.urlCheck(url), - Settings.load('autoCloseStream closedTimeout'), + Settings.select('autoCloseStream', 'closedTimeout'), ]); // If the domain wasn't whitelisted, then we shouldn't create this asset! diff --git a/graph/loaders/comments.js b/graph/loaders/comments.js index 37d40a219..b42a636bc 100644 --- a/graph/loaders/comments.js +++ b/graph/loaders/comments.js @@ -94,6 +94,7 @@ const getCommentCountByQuery = (ctx, options) => { author_id, tags, action_type, + excludeDeleted, } = options; // If user queries for statuses other than NONE and/or ACCEPTED statuses, it needs @@ -120,6 +121,12 @@ const getCommentCountByQuery = (ctx, options) => { query.merge({ author_id }); } + if (excludeDeleted) { + // The null query matches documents that either contain the `deleted_at` + // field whose value is null or that do not contain the `deleted_at` field. + query.merge({ deleted_at: null }); + } + if (ctx.user != null && ctx.user.can(SEARCH_OTHERS_COMMENTS) && action_type) { query.merge({ [`action_counts.${sc(action_type.toLowerCase())}`]: { @@ -328,11 +335,12 @@ const getCommentsByQuery = async ( sortOrder, sortBy, excludeIgnored, + excludeDeleted, tags, action_type, } ) => { - let comments = CommentModel.find(); + const query = CommentModel.find(); // Enforce that the limit must be gte 0 if this option is not true. if (!ALLOW_NO_LIMIT_QUERIES && limit < 0) { @@ -350,11 +358,17 @@ const getCommentsByQuery = async ( } if (statuses) { - comments = comments.where({ status: { $in: statuses } }); + query.merge({ status: { $in: statuses } }); + } + + if (excludeDeleted) { + // The null query matches documents that either contain the `deleted_at` + // field whose value is null or that do not contain the `deleted_at` field. + query.merge({ deleted_at: null }); } if (ctx.user != null && ctx.user.can(SEARCH_OTHERS_COMMENTS) && action_type) { - comments = comments.where({ + query.merge({ [`action_counts.${sc(action_type.toLowerCase())}`]: { $gt: 0, }, @@ -362,7 +376,7 @@ const getCommentsByQuery = async ( } if (ids) { - comments = comments.find({ + query.merge({ id: { $in: ids, }, @@ -370,7 +384,7 @@ const getCommentsByQuery = async ( } if (tags) { - comments = comments.find({ + query.merge({ 'tags.tag.name': { $in: tags, }, @@ -383,17 +397,17 @@ const getCommentsByQuery = async ( (ctx.user.can(SEARCH_OTHERS_COMMENTS) || ctx.user.id === author_id) && author_id != null ) { - comments = comments.where({ author_id }); + query.merge({ author_id }); } if (asset_id) { - comments = comments.where({ asset_id }); + query.merge({ asset_id }); } // We perform the undefined check because, null, is a valid state for the // search to be with, which indicates that it is at depth 0. if (parent_id !== undefined) { - comments = comments.where({ parent_id }); + query.merge({ parent_id }); } if ( @@ -402,12 +416,12 @@ const getCommentsByQuery = async ( ctx.user.ignoresUsers && ctx.user.ignoresUsers.length > 0 ) { - comments = comments.where({ + query.merge({ author_id: { $nin: ctx.user.ignoresUsers }, }); } - return executeWithSort(ctx, comments, { cursor, sortOrder, sortBy, limit }); + return executeWithSort(ctx, query, { cursor, sortOrder, sortBy, limit }); }; /** diff --git a/graph/loaders/settings.js b/graph/loaders/settings.js index fa0a17459..8b07d712b 100644 --- a/graph/loaders/settings.js +++ b/graph/loaders/settings.js @@ -1,23 +1,60 @@ -const SettingsService = require('../../services/settings'); +const Settings = require('../../services/settings'); const DataLoader = require('dataloader'); +const { zipObject } = require('lodash'); /** - * Creates a set of loaders based on a GraphQL context. - * @param {Object} context the context of the GraphQL request - * @return {Object} object of loaders + * SettingsLoader manages loading specific fields only of the Settings object. */ -module.exports = () => { - const loader = new DataLoader(selections => - Promise.all( - selections.map(fields => { - return SettingsService.retrieve(fields); - }) - ) - ); +class SettingsLoader { + constructor() { + this._loader = new DataLoader(this._batchLoadFn.bind(this)); + this._cache = null; + } - return { - Settings: { - load: (fields = false) => loader.load(fields), - }, - }; -}; + async _batchLoadFn(fields) { + // Load a settings object with all the requested fields, unless we have the + // entire object cached, in which case we'll return the whole cache. + const obj = this._cache + ? await this._cache + : await Settings.select(...fields); + + // Return the specific fields for each of the fields that were loaded. + return fields.map(field => obj[field]); + } + + /** + * load will return the entire Settings object with all fields. + */ + load() { + if (this._cache) { + // Return the cached settings promise. + return this._cache; + } + + // Create a promise that will return the settings object. + const promise = Settings.retrieve(); + + // Set this as the cached value. + this._cache = promise; + + // Return the promised settings. + return promise; + } + + /** + * select will return a promise which resolves to the Settings object that + * contains the requested fields only. + * + * @param {Array} fields the fields from Settings we want to load. + */ + async select(...fields) { + // Load all the values for the specific fields. + const values = await this._loader.loadMany(fields); + + // Zip up the fields and values to create an object to return and return the + // assembled Settings object. + return zipObject(fields, values); + } +} + +module.exports = () => ({ Settings: new SettingsLoader() }); diff --git a/graph/mutators/action.js b/graph/mutators/action.js index d759449b0..5557e1a9b 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -14,8 +14,15 @@ const getActionItem = async (ctx, { item_id, item_type }) => { const { loaders: { Comments, Users } } = ctx; switch (item_type) { - case 'COMMENTS': - return Comments.get.load(item_id); + case 'COMMENTS': { + // Get a comment by ID, unless the comment is deleted, then return null. + const comment = await Comments.get.load(item_id); + if (comment.deleted_at) { + return null; + } + + return comment; + } case 'USERS': return Users.getByID.load(item_id); default: diff --git a/graph/mutators/user.js b/graph/mutators/user.js index 1f43ca838..8b9afc206 100644 --- a/graph/mutators/user.js +++ b/graph/mutators/user.js @@ -181,10 +181,10 @@ const changeUserPassword = async (ctx, oldPassword, newPassword) => { await Users.changePassword(user.id, newPassword); // Get some context for the email to be sent. - const { organizationName, organizationContactEmail } = await Settings.load([ + const { organizationName, organizationContactEmail } = await Settings.select( 'organizationName', - 'organizationContactEmail', - ]); + 'organizationContactEmail' + ); // Send the password change email. await Users.sendEmail(user, { diff --git a/graph/resolvers/asset.js b/graph/resolvers/asset.js index 6bdf4d19e..3ed5305b7 100644 --- a/graph/resolvers/asset.js +++ b/graph/resolvers/asset.js @@ -1,4 +1,4 @@ -const { decorateWithTags } = require('./util'); +const { decorateWithTags, getRequestedFields } = require('./util'); const Asset = { async comment({ id }, { id: commentId }, { loaders: { Comments } }) { @@ -64,14 +64,17 @@ const Asset = { return Comments.countByAssetID.load(id); }, - async settings({ settings = null }, _, { loaders: { Settings } }) { + async settings({ settings = null }, _, { loaders: { Settings } }, info) { + // Get the fields we want from the settings. + const fields = getRequestedFields(info); + // Load the global settings, and merge them into the asset specific settings // if we have some. - let globalSettings = await Settings.load(); + let globalSettings = await Settings.select(...fields); if (settings !== null) { - settings = Object.assign({}, globalSettings.toObject(), settings); + settings = Object.assign({}, globalSettings, settings); } else { - settings = globalSettings.toObject(); + settings = globalSettings; } return settings; diff --git a/graph/resolvers/comment.js b/graph/resolvers/comment.js index e562c062c..c40aa49e0 100644 --- a/graph/resolvers/comment.js +++ b/graph/resolvers/comment.js @@ -58,10 +58,14 @@ const Comment = { return Assets.getByID.load(asset_id); }, async editing(comment, _, { loaders: { Settings } }) { - const settings = await Settings.load(); - const editableUntil = new Date( - Number(new Date(comment.created_at)) + settings.editCommentWindowLength + const { editCommentWindowLength } = await Settings.select( + 'editCommentWindowLength' ); + + const editableUntil = new Date( + Number(new Date(comment.created_at)) + editCommentWindowLength + ); + return { edited: comment.edited, editableUntil: editableUntil, diff --git a/graph/resolvers/root_query.js b/graph/resolvers/root_query.js index 23fdef288..53296a2ca 100644 --- a/graph/resolvers/root_query.js +++ b/graph/resolvers/root_query.js @@ -1,4 +1,4 @@ -const { decorateWithPermissionCheck } = require('./util'); +const { decorateWithPermissionCheck, getRequestedFields } = require('./util'); const { SEARCH_ASSETS, SEARCH_OTHERS_COMMENTS, @@ -16,8 +16,12 @@ const RootQuery = { return Assets.getByURL(query.url); }, - settings(_, args, { loaders: { Settings } }) { - return Settings.load(); + settings(_, args, { loaders: { Settings } }, info) { + // Get the fields we want from the settings. + const fields = getRequestedFields(info); + + // Load only the requested fields. + return Settings.select(...fields); }, // This endpoint is used for loading moderation queues, so hide it in the diff --git a/graph/resolvers/util.js b/graph/resolvers/util.js index cec2a71dd..a1a745ab2 100644 --- a/graph/resolvers/util.js +++ b/graph/resolvers/util.js @@ -2,7 +2,8 @@ const { ADD_COMMENT_TAG, SEARCH_OTHER_USERS, } = require('../../perms/constants'); -const { property, isBoolean } = require('lodash'); +const { property, isBoolean, pull } = require('lodash'); +const graphqlFields = require('graphql-fields'); /** * getResolver will get the resolver from the typeResolver or apply the default @@ -207,7 +208,11 @@ const decorateWithTags = ( }; }; +const getRequestedFields = info => + pull(Object.keys(graphqlFields(info)), '__typename'); + module.exports = { + getRequestedFields, decorateUserField, decorateWithTags, decorateWithPermissionCheck, diff --git a/graph/typeDefs.graphql b/graph/typeDefs.graphql index b5c193e3d..03c97ce58 100644 --- a/graph/typeDefs.graphql +++ b/graph/typeDefs.graphql @@ -418,6 +418,9 @@ input CommentsQuery { # Exclude comments ignored by the requesting user excludeIgnored: Boolean + + # excludeDeleted when true will exclude deleted comments from the response. + excludeDeleted: Boolean = false } input RepliesQuery { @@ -434,6 +437,9 @@ input RepliesQuery { # Exclude comments ignored by the requesting user excludeIgnored: Boolean + + # excludeDeleted when true will exclude deleted comments from the response. + excludeDeleted: Boolean = false } # CommentCountQuery allows the ability to query comment counts by specific @@ -463,6 +469,9 @@ input CommentCountQuery { # Filter by a specific tag name. tags: [String!] + + # excludeDeleted when true will exclude deleted comments from the count. + excludeDeleted: Boolean = false } # UserCountQuery allows the ability to query user counts by specific @@ -519,7 +528,7 @@ type Comment { replies(query: RepliesQuery = {}): CommentConnection! # replyCount is the number of replies with a depth of 1. Only direct replies - # to this comment are counted. + # to this comment are counted. Deleted comments are included in this count. replyCount: Int # Actions completed on the parent. Requires the `ADMIN` role. diff --git a/middleware/staticTemplate.js b/middleware/staticTemplate.js index 0dc05a7be..f16caaea9 100644 --- a/middleware/staticTemplate.js +++ b/middleware/staticTemplate.js @@ -95,7 +95,7 @@ const createResolveFactory = (() => { module.exports = async (req, res, next) => { try { // Attach the custom css url. - const { customCssUrl } = await SettingsService.retrieve('customCssUrl'); + const { customCssUrl } = await SettingsService.select('customCssUrl'); res.locals.customCssUrl = customCssUrl; } catch (err) { console.warn(err); diff --git a/models/schema/asset.js b/models/schema/asset.js index 043bc9a3f..82de92120 100644 --- a/models/schema/asset.js +++ b/models/schema/asset.js @@ -43,8 +43,7 @@ const Asset = new Schema( modified_date: Date, // This object is used exclusively for storing settings that are to override - // the base settings from the base Settings object. This is to be accessed - // always after running `rectifySettings` against it. + // the base settings from the base Settings object. settings: { default: {}, type: Object, diff --git a/models/schema/setting.js b/models/schema/setting.js index af7932910..eb9dc76d1 100644 --- a/models/schema/setting.js +++ b/models/schema/setting.js @@ -125,21 +125,4 @@ const Setting = new Schema( } ); -/** - * Merges two settings objects. - */ -Setting.method('merge', function(src) { - Setting.eachPath(path => { - // Exclude internal fields... - if (['id', '_id', '__v', 'created_at', 'updated_at'].includes(path)) { - return; - } - - // If the source object contains the path, shallow copy it. - if (path in src) { - this[path] = src[path]; - } - }); -}); - module.exports = Setting; diff --git a/package.json b/package.json index 9ec249f83..f6ff46a59 100644 --- a/package.json +++ b/package.json @@ -114,6 +114,7 @@ "graphql-ast-tools": "0.2.3", "graphql-docs": "0.2.0", "graphql-errors": "^2.1.0", + "graphql-fields": "^1.0.2", "graphql-redis-subscriptions": "1.3.0", "graphql-subscriptions": "^0.4.3", "graphql-tag": "^1.2.3", diff --git a/plugins/talk-plugin-local-auth/server/mutators.js b/plugins/talk-plugin-local-auth/server/mutators.js index 147d06cd2..7049cd546 100644 --- a/plugins/talk-plugin-local-auth/server/mutators.js +++ b/plugins/talk-plugin-local-auth/server/mutators.js @@ -55,9 +55,9 @@ async function updateUserEmailAddress(ctx, email, confirmPassword) { } // Get some context for the email to be sent. - const { organizationContactEmail } = await Settings.load([ - 'organizationContactEmail', - ]); + const { organizationContactEmail } = await Settings.select( + 'organizationContactEmail' + ); // Send off the email to the old email address that we have changed it. await Mailer.send({ diff --git a/plugins/talk-plugin-notifications-category-reply/index.js b/plugins/talk-plugin-notifications-category-reply/index.js index cce258daa..627d50ff8 100644 --- a/plugins/talk-plugin-notifications-category-reply/index.js +++ b/plugins/talk-plugin-notifications-category-reply/index.js @@ -1,17 +1,23 @@ const { get, map } = require('lodash'); const path = require('path'); -const handle = async (ctx, comment) => { +const commentAddedHandler = async (ctx, comment) => { // Check to see if this reply is visible. if (!comment.visible) { - ctx.log.info('comment was not visible, not sending notification'); + ctx.log.info( + { commentID: comment.id }, + 'comment was not visible, not sending notification' + ); return; } // Check to see if this is a reply to an existing comment. const parentID = get(comment, 'parent_id', null); - if (parentID === null) { - ctx.log.info('could not get parent comment id'); + if (!parentID) { + ctx.log.info( + { commentID: comment.id }, + 'could not get parent comment id, comment must be a top level comment' + ); return; } @@ -40,42 +46,60 @@ const handle = async (ctx, comment) => { return; } + const parentComment = get(reply, 'data.comment'); + if (!parentComment) { + ctx.log.info({ parentID }, 'could not get parent comment'); + return; + } + // Check if the user has notifications enabled. const enabled = get( - reply, - 'data.comment.user.notificationSettings.onReply', + parentComment, + 'user.notificationSettings.onReply', false ); if (!enabled) { + ctx.log.error( + 'parent comment author does not have notification category enabled' + ); return; } - const userID = get(reply, 'data.comment.user.id', null); - if (!userID) { - ctx.log.info('could not get parent comment user id'); + const parentAuthor = get(parentComment, 'user', null); + if (!parentAuthor) { + ctx.log.info('could not get parent author'); return; } - // Pull out the author of the new comment. + // Pull out the author of the new comment. This was outputted from Mongo, so + // we have to pull it out of the `author_id` field. const authorID = get(comment, 'author_id'); // Check to see if this is yourself replying to yourself, if that's the case // don't send a notification. - if (userID === authorID) { + if (parentAuthor.id === authorID) { ctx.log.info('user id of parent comment is the same as the new comment'); return; } // Check to see if this user is ignoring the user who replied to their // comment. - if (map(get(comment, 'user.ignoredUsers', []), 'id').indexOf(authorID)) { - ctx.log.info('parent user has ignored the author of the new comment'); + const ignoredUsers = map(get(parentAuthor, 'ignoredUsers', []), 'id'); + if (ignoredUsers.includes(authorID)) { + ctx.log.info( + { parentAuthorID: parentAuthor.id, authorID }, + 'parent user has ignored the author of the new comment' + ); return; } // The user does have notifications for replied comments enabled, queue the // notification to be sent. - return { userID, date: comment.created_at, context: comment.id }; + return { + userID: parentAuthor.id, + date: comment.created_at, + context: comment.id, + }; }; const hydrate = async (ctx, category, context) => { @@ -133,7 +157,7 @@ const commentAcceptedHandleAdapter = (ctx, comment) => { } // Delegate to the handle function. - return handle(ctx, comment); + return commentAddedHandler(ctx, comment); }; module.exports = { @@ -155,7 +179,7 @@ module.exports = { translations: path.join(__dirname, 'translations.yml'), notifications: [ { - handle, + handle: commentAddedHandler, category: 'reply', event: 'commentAdded', hydrate, diff --git a/plugins/talk-plugin-notifications/server/NotificationManager.js b/plugins/talk-plugin-notifications/server/NotificationManager.js index eb90dd48b..63af8efdb 100644 --- a/plugins/talk-plugin-notifications/server/NotificationManager.js +++ b/plugins/talk-plugin-notifications/server/NotificationManager.js @@ -32,7 +32,10 @@ const handleHandlers = (ctx, handlers, ...args) => // Attempt to create a notification out of it. const notification = await handle(ctx, ...args); if (!notification) { - ctx.log.info('no notification deemed by event handler'); + ctx.log.info( + { category, event }, + 'no notification deemed by event handler' + ); return; } diff --git a/plugins/talk-plugin-notifications/server/util.js b/plugins/talk-plugin-notifications/server/util.js index 7198df1e1..bbebef4b2 100644 --- a/plugins/talk-plugin-notifications/server/util.js +++ b/plugins/talk-plugin-notifications/server/util.js @@ -3,7 +3,7 @@ const getOrganizationName = async ctx => { const { loaders: { Settings } } = ctx; // Get the settings. - const { organizationName = null } = await Settings.load('organizationName'); + const { organizationName = null } = await Settings.select('organizationName'); return organizationName; }; diff --git a/plugins/talk-plugin-profile-data/client/translations.yml b/plugins/talk-plugin-profile-data/client/translations.yml index e139cfb57..d1fdccb73 100644 --- a/plugins/talk-plugin-profile-data/client/translations.yml +++ b/plugins/talk-plugin-profile-data/client/translations.yml @@ -1,7 +1,7 @@ en: download_request: section_title: "Download My Comment History" - you_will_get_a_copy: "You will recieve an email with a link to download your comment history. You can make" + you_will_get_a_copy: "You will receive an email with a link to download your comment history. You can make" download_rate: "one download request every {0} days" most_recent_request: "Your most recent request" request: "Request Comment History" diff --git a/plugins/talk-plugin-profile-data/server/connect.js b/plugins/talk-plugin-profile-data/server/connect.js index 0e5284e57..676d67176 100644 --- a/plugins/talk-plugin-profile-data/server/connect.js +++ b/plugins/talk-plugin-profile-data/server/connect.js @@ -36,10 +36,10 @@ module.exports = connectors => { const { organizationName, organizationContactEmail, - } = await Settings.load([ + } = await Settings.select( 'organizationName', - 'organizationContactEmail', - ]); + 'organizationContactEmail' + ); // rescheduledDeletionDate is the date in the future that we'll set the // user's account to be deleted on if this delete fails. diff --git a/plugins/talk-plugin-profile-data/server/mutators.js b/plugins/talk-plugin-profile-data/server/mutators.js index eec54772c..4a1e97791 100644 --- a/plugins/talk-plugin-profile-data/server/mutators.js +++ b/plugins/talk-plugin-profile-data/server/mutators.js @@ -81,7 +81,7 @@ async function sendDownloadLink(ctx) { // Generate the download links. const { downloadLandingURL } = await generateDownloadLinks(ctx, user.id); - const { organizationName } = await Settings.load('organizationName'); + const { organizationName } = await Settings.select('organizationName'); // Send the download link via the user's attached email account. await Users.sendEmail(user, { @@ -130,7 +130,7 @@ async function requestDeletion({ } ); - const { organizationName } = await Settings.load('organizationName'); + const { organizationName } = await Settings.select('organizationName'); // Send the download link via the user's attached email account. await Users.sendEmail(user, { @@ -171,7 +171,7 @@ async function cancelDeletion({ { $unset: { 'metadata.scheduledDeletionDate': 1 } } ); - const { organizationName } = await Settings.load('organizationName'); + const { organizationName } = await Settings.select('organizationName'); // Send the download link via the user's attached email account. await Users.sendEmail(user, { diff --git a/services/assets.js b/services/assets.js index fbed22287..794145eb3 100644 --- a/services/assets.js +++ b/services/assets.js @@ -1,13 +1,12 @@ const CommentModel = require('../models/comment'); const AssetModel = require('../models/asset'); -const SettingsService = require('./settings'); +const Settings = require('./settings'); const DomainList = require('./domain_list'); const { ErrAssetURLAlreadyExists, ErrNotFound, ErrInvalidAssetURL, } = require('../errors'); -const { merge, isEmpty } = require('lodash'); const { dotize } = require('./utils'); module.exports = class AssetsService { @@ -27,28 +26,6 @@ module.exports = class AssetsService { return AssetModel.findOne({ url }); } - /** - * Retrieves the settings given an asset query and rectifies it against the - * global settings. - * @param {Promise} assetQuery an asset query that returns a single asset. - * @return {Promise} - */ - static async rectifySettings(assetQuery, settings = null) { - const [globalSettings, asset] = await Promise.all([ - settings !== null ? settings : SettingsService.retrieve(), - assetQuery, - ]); - - // If the asset exists and has settings then return the merged object. - if (asset && asset.settings && !isEmpty(asset.settings)) { - settings = merge({}, globalSettings, asset.settings); - } else { - settings = globalSettings; - } - - return settings; - } - /** * Finds a asset by its url. * @@ -65,13 +42,13 @@ module.exports = class AssetsService { // Check the URL to confirm that is in the domain whitelist return Promise.all([ DomainList.urlCheck(url), - SettingsService.retrieve(), - ]).then(([whitelisted, settings]) => { + Settings.select('autoCloseStream', 'closedTimeout'), + ]).then(([whitelisted, { autoCloseStream, closedTimeout }]) => { const update = { $setOnInsert: { url } }; - if (settings.autoCloseStream) { + if (autoCloseStream) { update.$setOnInsert.closedAt = new Date( - Date.now() + settings.closedTimeout * 1000 + Date.now() + closedTimeout * 1000 ); } @@ -139,10 +116,10 @@ module.exports = class AssetsService { * @return {Promise} */ static search({ value, limit, open, sortOrder, cursor } = {}) { - let assets = AssetModel.find({}); + let query = AssetModel.find({}); if (value && value.length > 0) { - assets.merge({ + query.merge({ $text: { $search: value, }, @@ -151,7 +128,7 @@ module.exports = class AssetsService { if (open != null) { if (open) { - assets.merge({ + query.merge({ $or: [ { closedAt: null, @@ -164,7 +141,7 @@ module.exports = class AssetsService { ], }); } else { - assets.merge({ + query.merge({ closedAt: { $lt: Date.now(), }, @@ -174,13 +151,13 @@ module.exports = class AssetsService { if (cursor) { if (sortOrder === 'DESC') { - assets.merge({ + query.merge({ created_at: { $lt: cursor, }, }); } else { - assets.merge({ + query.merge({ created_at: { $gt: cursor, }, @@ -188,7 +165,7 @@ module.exports = class AssetsService { } } - return assets + return query .sort({ created_at: sortOrder === 'DESC' ? -1 : 1 }) .limit(limit); } diff --git a/services/cache.js b/services/cache.js index c413537a7..64f8ad934 100644 --- a/services/cache.js +++ b/services/cache.js @@ -271,69 +271,3 @@ cache.set = async (key, value, expiry, kf = keyfunc) => { return cache.client.set(kf(key), reply, 'EX', expiry); }; - -/** - * h is the hash form of the cache. - */ -cache.h = {}; - -cache.h.get = async (key, field = '__default__') => { - // Get the current value from redis. - const reply = await cache.client.hget(keyfunc(key), field); - - if (typeof reply !== 'undefined' && reply !== null) { - return JSON.parse(reply); - } - - return null; -}; - -cache.h.set = async (key, field = '__default__', value, expiry = 60) => { - // Serialize the value as JSON. - let reply = JSON.stringify(value); - - return cache.client - .pipeline() - .hset(keyfunc(key), field, reply) - .expire(keyfunc(key), expiry) - .exec(); -}; - -cache.h.invalidate = async (key, field = null) => { - if (field === null) { - return cache.invalidate(key); - } - - debug(`invalidate: ${keyfunc(key)} ${field}`); - - return cache.client.hdel(keyfunc(key), field); -}; - -cache.h.wrap = async (key, field, expiry, work) => { - let value = await cache.h.get(key, field); - if (value !== null) { - debug('wrap: hit', keyfunc(key)); - return value; - } - - debug('wrap: miss', keyfunc(key)); - - value = await work(); - - process.nextTick(async () => { - try { - await cache.h.set(key, field, value, expiry); - debug('wrap: set complete'); - } catch (err) { - console.error(err); - } - }); - - return value; -}; - -cache.h.incr = async (key, field = '__default__', expiry) => - cache.client.hincrbyex(keyfunc(key), field, 1, expiry); - -cache.h.decr = async (key, field = '__default__', expiry) => - cache.client.hincrbyex(keyfunc(key), field, -1, expiry); diff --git a/services/comments.js b/services/comments.js index b9d73d91c..19b611089 100644 --- a/services/comments.js +++ b/services/comments.js @@ -39,9 +39,12 @@ module.exports = { const created_at = new Date(); // Check to see if we are replying to a comment, and if that comment is - // visible. + // visible and that it's not deleted. if (parent_id !== null) { - const parent = await CommentModel.findOne({ id: parent_id }); + const parent = await CommentModel.findOne({ + id: parent_id, + deleted_at: null, + }); if (parent === null || !parent.visible) { throw new ErrParentDoesNotVisible(); } @@ -94,13 +97,14 @@ module.exports = { status: { $in: EDITABLE_STATUSES, }, + deleted_at: null, }; // Establish the edit window (if it exists) and add the condition to the // original query. const { editCommentWindowLength: editWindowMs, - } = await SettingsService.retrieve(); + } = await SettingsService.select('editCommentWindowLength'); const lastEditableCommentCreatedAt = new Date(Date.now() - editWindowMs); query.created_at = { $gt: lastEditableCommentCreatedAt, @@ -186,8 +190,10 @@ module.exports = { */ pushStatus: async (id, status, assigned_by = null) => { const created_at = new Date(); + + // Update the comment unless the comment was deleted. const originalComment = await CommentModel.findOneAndUpdate( - { id }, + { id, deleted_at: null }, { $push: { status_history: { diff --git a/services/domain_list.js b/services/domain_list.js index aab1f9ea8..b194305b1 100644 --- a/services/domain_list.js +++ b/services/domain_list.js @@ -1,6 +1,6 @@ const debug = require('debug')('talk:services:domain_list'); const _ = require('lodash'); -const SettingsService = require('./settings'); +const Settings = require('./settings'); const { ROOT_URL } = require('../config'); @@ -19,7 +19,7 @@ class DomainList { * Loads domains white list in from the database */ async load() { - const { domains } = await SettingsService.retrieve(); + const { domains } = await Settings.select('domains'); this.upsert(domains); } diff --git a/services/moderation/index.js b/services/moderation/index.js index 530d6f2a6..27346e620 100644 --- a/services/moderation/index.js +++ b/services/moderation/index.js @@ -1,5 +1,5 @@ const { ErrNotFound } = require('../../errors'); -const get = require('lodash/get'); +const { get, merge, isEmpty } = require('lodash'); // Load in the phases to use. const { @@ -80,10 +80,7 @@ const compose = phases => async (ctx, comment, options) => { * @param {Object} comment comment object to use */ const fetchOptions = async (ctx, comment) => { - const { - connectors: { services: { Assets: AssetsService } }, - loaders: { Settings, Assets }, - } = ctx; + const { loaders: { Settings, Assets } } = ctx; // Load the settings. const settings = await Settings.load(); @@ -102,8 +99,12 @@ const fetchOptions = async (ctx, comment) => { throw new ErrNotFound(); } - // Combine the asset and the settings to get the asset settings. - asset.settings = await AssetsService.rectifySettings(asset, settings); + // If the asset exists and has settings then return the merged object. + if (asset && asset.settings && !isEmpty(asset.settings)) { + asset.settings = merge({}, settings, asset.settings); + } else { + asset.settings = settings; + } // Create the options that will be consumed by the phases. return { diff --git a/services/passport.js b/services/passport.js index 0ae06afb8..e4813e419 100644 --- a/services/passport.js +++ b/services/passport.js @@ -1,7 +1,7 @@ const passport = require('passport'); const { set, get } = require('lodash'); const UsersService = require('./users'); -const SettingsService = require('./settings'); +const Settings = require('./settings'); const TokensService = require('./tokens'); const fetch = require('node-fetch'); const FormData = require('form-data'); @@ -157,7 +157,9 @@ async function ValidateUserLogin(loginProfile, user, done) { } // The user is a local user, check if we need email confirmation. - const { requireEmailConfirmation = false } = await SettingsService.retrieve(); + const { requireEmailConfirmation = false } = await Settings.select( + 'requireEmailConfirmation' + ); // If we have the requirement of checking that emails for users are // verified, then we need to check the email address to ensure that it has diff --git a/services/settings.js b/services/settings.js index ee5125183..def42187a 100644 --- a/services/settings.js +++ b/services/settings.js @@ -1,59 +1,56 @@ -const SettingModel = require('../models/setting'); -const cache = require('./cache'); +const Setting = require('../models/setting'); const { ErrSettingsNotInit } = require('../errors'); const { dotize } = require('./utils'); -const { SETTINGS_CACHE_TIME } = require('../config'); +const { isEmpty, zipObject, uniq } = require('lodash'); +const DataLoader = require('dataloader'); -/** - * The selector used to uniquely identify the settings document. - */ const selector = { id: '1' }; -const retrieve = async fields => { - let settings; - if (fields) { - settings = await SettingModel.findOne(selector).select(fields); - } else { - settings = await SettingModel.findOne(selector); - } - if (!settings) { +async function loadFn(fields = []) { + const model = await Setting.findOne(selector).select(uniq(fields)); + if (!model) { throw new ErrSettingsNotInit(); } - return settings; -}; + return model.toObject(); +} -/** - * The Setting Service object exposing the Setting model. - */ -module.exports = class SettingsService { - /** - * Gets the entire settings record and sends it back - * @return {Promise} settings the whole settings record - */ - static async retrieve(fields) { - if (process.env.NODE_ENV === 'production') { - // When in production, wrap the settings retrieval with a cache. - const settings = await cache.h.wrap( - 'settings', - fields, - SETTINGS_CACHE_TIME / 1000, - () => retrieve(fields) - ); +// batchLoadFn will load a settings object with all the requested fields. +async function batchLoadFn(fields) { + // Load a settings object with all the requested fields. + const obj = await loadFn(fields); - return new SettingModel(settings); + // Return the specific fields for each of the fields that were loaded. + return fields.map(field => obj[field]); +} + +// batchedSettingsLoader will load setting fields for each request. This isn't a +// cached loader, so this is really just for optimizing the requests made to the +// database by batching. +const batchedSettingsLoader = new DataLoader(batchLoadFn, { cache: false }); + +class Settings { + static async retrieve(...fields) { + if (!isEmpty(fields)) { + // Included for backwards compatibility. + return Settings.select(...fields); } - return retrieve(fields); + // Call the loadFn directly if we need to load all the fields. + return loadFn(fields); + } + + static async select(...fields) { + // Load all the values for the specific fields. + const values = await batchedSettingsLoader.loadMany(fields); + + // Zip up the fields and values to create an object to return and return the + // assembled Settings object. + return zipObject(fields, values); } - /** - * This will update the settings object with whatever you pass in - * @param {object} setting a hash of whatever settings you want to update - * @return {Promise} settings Promise that resolves to the entire (updated) settings object. - */ static async update(settings) { - const updatedSettings = await SettingModel.findOneAndUpdate( + const updatedSettings = await Setting.findOneAndUpdate( selector, { $set: dotize(settings), @@ -65,21 +62,16 @@ module.exports = class SettingsService { } ); - if (process.env.NODE_ENV === 'production') { - await cache.h.invalidate('settings'); - } - return updatedSettings; } - /** - * This is run once when the app starts to ensure settings are populated. - */ static init(defaults = {}) { - return SettingsService.retrieve().catch(() => { - let settings = new SettingModel(defaults); + return Settings.retrieve().catch(() => { + const settings = new Setting(defaults); return settings.save(); }); } -}; +} + +module.exports = Settings; diff --git a/services/setup.js b/services/setup.js index 2517d376b..a691550ef 100644 --- a/services/setup.js +++ b/services/setup.js @@ -26,7 +26,7 @@ module.exports = class SetupService { try { // Get the current settings, we are expecting an error here. - await SettingsService.retrieve(); + await SettingsService.select('id'); // We should NOT have gotten a settings object, this means that the // application is already setup. Error out here. diff --git a/services/tags.js b/services/tags.js index cc8934e0a..b4e70cb29 100644 --- a/services/tags.js +++ b/services/tags.js @@ -1,23 +1,24 @@ -const CommentModel = require('../models/comment'); -const AssetModel = require('../models/asset'); -const UserModel = require('../models/user'); -const AssetsService = require('./assets'); -const SettingsService = require('./settings'); +const Comment = require('../models/comment'); +const Asset = require('../models/asset'); +const User = require('../models/user'); +const Assets = require('./assets'); +const Settings = require('./settings'); const { ADD_COMMENT_TAG } = require('../perms/constants'); const { ErrNotAuthorized } = require('../errors'); +const { get, has } = require('lodash'); const updateModel = async (item_type, query, update) => { // Get the model to update with. let Model; switch (item_type) { case 'COMMENTS': - Model = CommentModel; + Model = Comment; break; case 'ASSETS': - Model = AssetModel; + Model = Asset; break; case 'USERS': - Model = UserModel; + Model = User; break; default: throw new Error( @@ -44,32 +45,23 @@ const ownershipQuery = async (item_type, link, query) => { class TagsService { /** - * Retrives a global tag from the settings based on the input_type. + * Retrieves a global tag from the settings based on the input_type. */ static async getAll({ id, item_type, asset_id = null }) { - // Extract the settings from the database. - let settings; - switch (item_type) { - case 'COMMENTS': - settings = await AssetsService.rectifySettings( - AssetsService.findById(asset_id) - ); - break; - case 'ASSETS': - settings = await AssetsService.rectifySettings( - AssetsService.findById(id) - ); - break; - case 'USERS': - settings = await SettingsService.retrieve(); - break; - default: - settings = await SettingsService.retrieve(); - break; + // Optionally get an asset. + let asset; + if (item_type === 'COMMENTS') { + asset = await Assets.findById(asset_id); + } else if (item_type === 'ASSETS') { + asset = await Assets.findById(id); + } + + if (asset && has(asset, 'settings.tags')) { + return get(asset, 'settings.tags'); } // Extract the tags from the settings object. - let { tags = [] } = settings; + const { tags = [] } = await Settings.select('tags'); // Return the first tag that matches the requested form. return tags; diff --git a/services/wordlist.js b/services/wordlist.js index 9e7e1581e..22bde090a 100644 --- a/services/wordlist.js +++ b/services/wordlist.js @@ -1,12 +1,12 @@ const debug = require('debug')('talk:services:wordlist'); const _ = require('lodash'); -const SettingsService = require('./settings'); +const Settings = require('./settings'); const { ErrContainsProfanity } = require('../errors'); const memoize = require('lodash/memoize'); const { escapeRegExp } = require('./regex'); /** - * Generate a regulare expression that catches the `phrases`. + * Generate a regular expression that catches the `phrases`. */ function generateRegExp(phrases) { const inner = phrases @@ -49,7 +49,7 @@ class Wordlist { * Loads wordlists in from the database */ load() { - return SettingsService.retrieve().then(settings => { + return Settings.select('wordlist').then(settings => { // Insert the settings wordlist. this.upsert(settings.wordlist); }); diff --git a/test/server/services/settings.js b/test/server/services/settings.js index ff2c898e1..ca6fdef8d 100644 --- a/test/server/services/settings.js +++ b/test/server/services/settings.js @@ -1,15 +1,18 @@ -const SettingsService = require('../../../services/settings'); +const Settings = require('../../../services/settings'); const chai = require('chai'); const expect = chai.expect; -describe('services.SettingsService', () => { +describe('services.Settings', () => { beforeEach(() => - SettingsService.init({ moderation: 'PRE', wordlist: ['donut'] }) + Settings.init({ + moderation: 'PRE', + wordlist: { banned: ['bannedWord'], suspect: [] }, + }) ); describe('#retrieve()', () => { it('should have a moderation field defined', () => { - return SettingsService.retrieve().then(settings => { + return Settings.retrieve().then(settings => { expect(settings) .to.have.property('moderation') .and.to.equal('PRE'); @@ -17,7 +20,7 @@ describe('services.SettingsService', () => { }); it('should have two infoBox fields defined', () => { - return SettingsService.retrieve().then(settings => { + return Settings.retrieve().then(settings => { expect(settings) .to.have.property('infoBoxEnable') .and.to.equal(false); @@ -28,6 +31,25 @@ describe('services.SettingsService', () => { }); }); + describe('#select()', () => { + it('should have a moderation field defined and not wordlist', () => { + return Settings.select('moderation').then(settings => { + expect(settings) + .to.have.property('moderation') + .and.to.equal('PRE'); + expect(settings).to.not.have.property('wordlist'); + }); + }); + it('should have a wordlist field defined and not moderation', () => { + return Settings.select('wordlist').then(settings => { + expect(settings).to.not.have.property('moderation'); + expect(settings).to.have.property('wordlist'); + expect(settings.wordlist).to.have.property('banned'); + expect(settings.wordlist.banned).to.contain('bannedWord'); + }); + }); + }); + describe('#update()', () => { it('should update the settings with a passed object', () => { const mockSettings = { @@ -35,7 +57,7 @@ describe('services.SettingsService', () => { infoBoxEnable: true, infoBoxContent: 'yeah', }; - return SettingsService.update(mockSettings).then(updatedSettings => { + return Settings.update(mockSettings).then(updatedSettings => { expect(updatedSettings).to.be.an('object'); expect(updatedSettings) .to.have.property('moderation') @@ -51,32 +73,20 @@ describe('services.SettingsService', () => { infoBoxEnable: true, infoBoxContent: 'yeah', }; - await SettingsService.update(mockSettings); + await Settings.update(mockSettings); - const settings = await SettingsService.retrieve(); + const settings = await Settings.retrieve(); settings.charCount = 500; - await SettingsService.update(settings.toObject()); + await Settings.update(settings); }); }); describe('#get', () => { it('should return the moderation settings', () => { - return SettingsService.retrieve().then(({ moderation }) => { + return Settings.retrieve().then(({ moderation }) => { expect(moderation).not.to.be.null; }); }); }); - - describe('#merge', () => { - it('should merge a settings object and its overrides', () => { - return SettingsService.retrieve().then(settings => { - let ovrSett = { moderation: 'POST' }; - - settings.merge(ovrSett); - - expect(settings).to.have.property('moderation', 'POST'); - }); - }); - }); }); diff --git a/yarn.lock b/yarn.lock index a043febf4..5cf363b6e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4451,6 +4451,10 @@ graphql-extensions@^0.0.x: core-js "^2.5.1" source-map-support "^0.5.0" +graphql-fields@^1.0.2: + version "1.0.2" + resolved "https://registry.yarnpkg.com/graphql-fields/-/graphql-fields-1.0.2.tgz#099ee1d4445b42d0f47e06d622acebb33abc6cce" + graphql-redis-subscriptions@1.3.0: version "1.3.0" resolved "https://registry.yarnpkg.com/graphql-redis-subscriptions/-/graphql-redis-subscriptions-1.3.0.tgz#bbc52b0f77bf7d50945c6bf4e8b8aba5135555b4"