From 7d8f2aa876c1e9c3154c2faa4fbf3f511b78dfde Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Tue, 13 Jun 2017 22:02:54 +0700 Subject: [PATCH] More robust reactions --- errors.js | 15 +++++++ graph/mutators/action.js | 2 +- locales/en.yml | 1 + plugin-api/beta/client/hocs/withReaction.js | 50 ++++++++++++++++----- plugin-api/beta/server/getReactionConfig.js | 15 ++++++- services/actions.js | 37 ++++++++++----- 6 files changed, 98 insertions(+), 22 deletions(-) diff --git a/errors.js b/errors.js index c203b1d25..5d2593d66 100644 --- a/errors.js +++ b/errors.js @@ -104,6 +104,20 @@ class ErrAuthentication extends APIError { } } +/** + * ErrAlreadyExists is returned when an attempt to create a resource failed due to an existing one. + */ +class ErrAlreadyExists extends APIError { + constructor(existing = null) { + super('authentication error occured', { + translation_key: 'ALREADY_EXISTS', + status: 400 + }, { + existing + }); + } +} + // ErrContainsProfanity is returned in the event that the middleware detects // profanity/wordlisted words in the payload. const ErrContainsProfanity = new APIError('This username contains elements which are not permitted in our community. If you think this is in error, please contact us or try again.', { @@ -171,6 +185,7 @@ const ErrCommentTooShort = new APIError('Comment was too short', { module.exports = { ExtendableError, APIError, + ErrAlreadyExists, ErrPasswordTooShort, ErrSettingsNotInit, ErrMissingEmail, diff --git a/graph/mutators/action.js b/graph/mutators/action.js index de9efea1a..96bf091cf 100644 --- a/graph/mutators/action.js +++ b/graph/mutators/action.js @@ -36,7 +36,7 @@ const createAction = async ({user = {}}, {item_id, item_type, action_type, group * Deletes an action based on the user id if the user owns that action. * @param {Object} user the user performing the request * @param {String} id the id of the action to delete - * @return {Promise} resolves when the action is deleted + * @return {Promise} resolves to the deleted action, or null if not found. */ const deleteAction = ({user}, {id}) => { return ActionModel.findOneAndRemove({ diff --git a/locales/en.yml b/locales/en.yml index ce9abdce5..4f287e36b 100644 --- a/locales/en.yml +++ b/locales/en.yml @@ -189,6 +189,7 @@ en: PASSWORD_REQUIRED: "Must input a password" COMMENTING_CLOSED: "Commenting is already closed" NOT_FOUND: "Resource not found" + ALREADY_EXISTS: "Resource already exists" INVALID_ASSET_URL: "Assert URL is invalid" email: "Not a valid E-Mail" confirm_password: "Passwords don't match. Please check again" diff --git a/plugin-api/beta/client/hocs/withReaction.js b/plugin-api/beta/client/hocs/withReaction.js index 42ac79ec5..1ed777996 100644 --- a/plugin-api/beta/client/hocs/withReaction.js +++ b/plugin-api/beta/client/hocs/withReaction.js @@ -149,6 +149,9 @@ export default (reaction) => (WrappedComponent) => { client: PropTypes.object.isRequired, }; + // Whether or not a mutation is currently active. + duringMutation = false; + constructor(props, context) { super(props, context); @@ -208,6 +211,26 @@ export default (reaction) => (WrappedComponent) => { } } + postReaction = () => { + if (this.duringMutation) { + return; + } + this.duringMutation = true; + return this.props.postReaction(this.props.comment) + .then((result) => {this.duringMutation = false; return Promise.resolve(result); }) + .catch((err) => {this.duringMutation = false; throw err; }); + } + + deleteReaction = () => { + if (this.duringMutation) { + return; + } + this.duringMutation = true; + return this.props.deleteReaction(this.props.comment) + .then((result) => {this.duringMutation = false; return Promise.resolve(result); }) + .catch((err) => {this.duringMutation = false; throw err; }); + } + render() { const {comment} = this.props; @@ -223,9 +246,16 @@ export default (reaction) => (WrappedComponent) => { const alreadyReacted = !!reactionSummary; - const withReactionProps = {reactionSummary, count, alreadyReacted}; - - return ; + return ; } } @@ -240,16 +270,16 @@ export default (reaction) => (WrappedComponent) => { } `, { - props: ({mutate, ownProps}) => ({ - deleteReaction: () => { + props: ({mutate}) => ({ + deleteReaction: (comment) => { const reactionSummary = getMyActionSummary( `${Reaction}ActionSummary`, - ownProps.comment + comment ); const id = reactionSummary.current_user.id; - const item_id = ownProps.comment.id; + const item_id = comment.id; const input = {id}; return mutate({ @@ -283,11 +313,11 @@ export default (reaction) => (WrappedComponent) => { } `, { - props: ({mutate, ownProps}) => ({ - postReaction: () => { + props: ({mutate}) => ({ + postReaction: (comment) => { const input = { - item_id: ownProps.comment.id, + item_id: comment.id, }; return mutate({ diff --git a/plugin-api/beta/server/getReactionConfig.js b/plugin-api/beta/server/getReactionConfig.js index 83c45dcb5..78bc45dfd 100644 --- a/plugin-api/beta/server/getReactionConfig.js +++ b/plugin-api/beta/server/getReactionConfig.js @@ -1,5 +1,6 @@ const wrapResponse = require('../../../graph/helpers/response'); const {SEARCH_OTHER_USERS} = require('../../../perms/constants'); +const errors = require('../../../errors'); function getReactionConfig(reaction) { reaction = reaction.toLowerCase(); @@ -134,13 +135,24 @@ function getReactionConfig(reaction) { // The comment is needed to allow better filtering e.g. by asset_id. pubsub.publish(`${reaction}ActionCreated`, {action, comment}); return Promise.resolve(action); - }); + }) + .catch((err) => { + if (err instanceof errors.ErrAlreadyExists) { + return Promise.resolve(err.metadata.existing); + } + throw err; + }); }); return wrapResponse(reaction)(response); }, [`delete${Reaction}Action`]: (_, {input: {id}}, {mutators: {Action}, pubsub, loaders: {Comments}}) => { const response = Action.delete({id}) .then((action) => { + + // Action doesn't exist or was already deleted. + if (!action) { + return Promise.resolve(null); + } return Comments.get.load(action.item_id).then((comment) => { // The comment is needed to allow better filtering e.g. by asset_id. @@ -148,6 +160,7 @@ function getReactionConfig(reaction) { return Promise.resolve(action); }); }); + return wrapResponse(reaction)(response); } }, diff --git a/services/actions.js b/services/actions.js index 40bef65bb..c59b3a3f9 100644 --- a/services/actions.js +++ b/services/actions.js @@ -1,5 +1,6 @@ const ActionModel = require('../models/action'); const _ = require('lodash'); +const errors = require('../errors'); module.exports = class ActionsService { @@ -12,10 +13,10 @@ module.exports = class ActionsService { } /** - * Add an action. - * @param {String} item_id identifier of the comment (uuid) + * Inserts an action. + * @param {String} item_id identifier of the item (uuid) * @param {String} user_id user id of the action (uuid) - * @param {String} action the new action to the comment + * @param {String} action the new action to the item * @return {Promise} */ static insertUserAction(action) { @@ -31,16 +32,32 @@ module.exports = class ActionsService { }; // Create/Update the action. - return ActionModel.findOneAndUpdate(query, action, { + return new Promise((resolve, reject) => { + ActionModel.findOneAndUpdate( + query, { - // Ensure that if it's new, we return the new object created. - new: true, + // Only set when not existing. + $setOnInsert: action, + }, { - // Perform an upsert in the event that this doesn't exist. - upsert: true, + // Ensure that if it's new, we return the new object created. + new: true, - // Set the default values if not provided based on the mongoose models. - setDefaultsOnInsert: true + // Use raw result to get `updatedExisting`. + passRawResult: true, + + // Perform an upsert in the event that this doesn't exist. + upsert: true, + + // Set the default values if not provided based on the mongoose models. + setDefaultsOnInsert: true + }, (err, doc, raw) => { + if (err) { return reject(err); } + if (raw.lastErrorObject.updatedExisting) { + return reject(new errors.ErrAlreadyExists(raw.value)); + } + return resolve(raw.value); + }); }); }