From 2a3e6ad76f3d871c99f9257c8948371cc93704af Mon Sep 17 00:00:00 2001 From: roschaefer Date: Fri, 13 Sep 2019 12:13:03 +0200 Subject: [PATCH 1/8] Alternative solution for #1494 --- .../notifications/notificationsMiddleware.js | 11 ++-- backend/src/schema/resolvers/comments.js | 52 ++++++++----------- backend/src/schema/resolvers/posts.js | 16 +++--- 3 files changed, 37 insertions(+), 42 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index 64386800d..b55f798f1 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -16,7 +16,6 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { } const session = context.driver.session() - const createdAt = new Date().toISOString() let cypher switch (reason) { case 'mentioned_in_post': { @@ -27,7 +26,8 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { AND NOT (user)<-[:BLOCKED]-(author) MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.createdAt = $createdAt + SET notification.updatedAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } @@ -40,7 +40,8 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { AND NOT (user)<-[:BLOCKED]-(postAuthor) MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.createdAt = $createdAt + SET notification.updatedAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } @@ -53,7 +54,8 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { AND NOT (author)<-[:BLOCKED]-(user) MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.createdAt = $createdAt + SET notification.updatedAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } @@ -63,7 +65,6 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { id, idsOfUsers, reason, - createdAt, }) session.close() } diff --git a/backend/src/schema/resolvers/comments.js b/backend/src/schema/resolvers/comments.js index 1f6803e09..7310b7af8 100644 --- a/backend/src/schema/resolvers/comments.js +++ b/backend/src/schema/resolvers/comments.js @@ -1,4 +1,4 @@ -import { neo4jgraphql } from 'neo4j-graphql-js' +import uuid from 'uuid/v4' import Resolver from './helpers/Resolver' export default { @@ -10,42 +10,34 @@ export default { // because we use relationships for this. So, we are deleting it from params // before comment creation. delete params.postId + params.id = params.id || uuid() + const session = context.driver.session() - const commentWithoutRelationships = await neo4jgraphql( - object, - params, - context, - resolveInfo, - false, - ) - - const transactionRes = await session.run( - ` - MATCH (post:Post {id: $postId}), (comment:Comment {id: $commentId}), (author:User {id: $userId}) + const createCommentCypher = ` + MATCH (post:Post {id: $postId}) + MATCH (author:User {id: $userId}) + WITH post, author + CREATE (comment:Comment {params}) + SET comment.createdAt = toString(datetime()) + SET comment.updatedAt = toString(datetime()) MERGE (post)<-[:COMMENTS]-(comment)<-[:WROTE]-(author) - RETURN comment, author`, - { - userId: context.user.id, - postId, - commentId: commentWithoutRelationships.id, - }, - ) + RETURN comment, author + ` + const transactionRes = await session.run(createCommentCypher, { + userId: context.user.id, + postId, + params, + }) + session.close() - const [commentWithAuthor] = transactionRes.records.map(record => { + const [response] = transactionRes.records.map(record => { return { - comment: record.get('comment'), - author: record.get('author'), + ...record.get('comment').properties, + author: record.get('author').properties, } }) - const { comment, author } = commentWithAuthor - - const commentReturnedWithAuthor = { - ...comment.properties, - author: author.properties, - } - session.close() - return commentReturnedWithAuthor + return response }, DeleteComment: async (object, args, context, resolveInfo) => { const session = context.driver.session() diff --git a/backend/src/schema/resolvers/posts.js b/backend/src/schema/resolvers/posts.js index 86dc78d62..2be17d3ec 100644 --- a/backend/src/schema/resolvers/posts.js +++ b/backend/src/schema/resolvers/posts.js @@ -82,6 +82,8 @@ export default { let post const createPostCypher = `CREATE (post:Post {params}) + SET post.createdAt = toString(datetime()) + SET post.updatedAt = toString(datetime()) WITH post MATCH (author:User {id: $userId}) MERGE (post)<-[:WROTE]-(author) @@ -96,9 +98,7 @@ export default { const session = context.driver.session() try { const transactionRes = await session.run(createPostCypher, createPostVariables) - const posts = transactionRes.records.map(record => { - return record.get('post').properties - }) + const posts = transactionRes.records.map(record => record.get('post').properties) post = posts[0] } catch (e) { if (e.code === 'Neo.ClientError.Schema.ConstraintValidationFailed') @@ -118,6 +118,7 @@ export default { let updatePostCypher = `MATCH (post:Post {id: $params.id}) SET post = $params + SET post.updatedAt = toString(datetime()) ` if (categoryIds && categoryIds.length) { @@ -129,10 +130,11 @@ export default { await session.run(cypherDeletePreviousRelations, { params }) - updatePostCypher += `WITH post - UNWIND $categoryIds AS categoryId - MATCH (category:Category {id: categoryId}) - MERGE (post)-[:CATEGORIZED]->(category) + updatePostCypher += ` + WITH post + UNWIND $categoryIds AS categoryId + MATCH (category:Category {id: categoryId}) + MERGE (post)-[:CATEGORIZED]->(category) ` } From ce487f1e0f94732df68e360fa78485cb2456deaa Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Fri, 13 Sep 2019 17:15:50 +0200 Subject: [PATCH 2/8] Set notifications.createdAt only at creation - we should not be setting it every time a notification is created --- .../notifications/notificationsMiddleware.js | 15 ++++++++++++--- .../notifications/notificationsMiddleware.spec.js | 2 +- backend/src/schema/resolvers/comments.js | 9 ++------- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index b55f798f1..effb7f4d0 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -27,7 +27,10 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE SET notification.updatedAt = toString(datetime()) - SET notification.createdAt = toString(datetime()) + SET ( + CASE + WHEN notification.createdAt IS NULL + THEN notification END ).createdAt = toString(datetime()) ` break } @@ -41,7 +44,10 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE SET notification.updatedAt = toString(datetime()) - SET notification.createdAt = toString(datetime()) + SET ( + CASE + WHEN notification.createdAt IS NULL + THEN notification END ).createdAt = toString(datetime()) ` break } @@ -55,7 +61,10 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE SET notification.updatedAt = toString(datetime()) - SET notification.createdAt = toString(datetime()) + SET ( + CASE + WHEN notification.createdAt IS NULL + THEN notification END ).createdAt = toString(datetime()) ` break } diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index b737768f2..94fb18668 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -391,7 +391,7 @@ describe('notifications', () => { expect(Date.parse(createdAtBefore)).toEqual(expect.any(Number)) expect(createdAtAfter).toBeTruthy() expect(Date.parse(createdAtAfter)).toEqual(expect.any(Number)) - expect(createdAtBefore).not.toEqual(createdAtAfter) + expect(createdAtBefore).toEqual(createdAtAfter) }) }) }) diff --git a/backend/src/schema/resolvers/comments.js b/backend/src/schema/resolvers/comments.js index 7310b7af8..f507897c5 100644 --- a/backend/src/schema/resolvers/comments.js +++ b/backend/src/schema/resolvers/comments.js @@ -21,7 +21,7 @@ export default { SET comment.createdAt = toString(datetime()) SET comment.updatedAt = toString(datetime()) MERGE (post)<-[:COMMENTS]-(comment)<-[:WROTE]-(author) - RETURN comment, author + RETURN comment ` const transactionRes = await session.run(createCommentCypher, { userId: context.user.id, @@ -30,12 +30,7 @@ export default { }) session.close() - const [response] = transactionRes.records.map(record => { - return { - ...record.get('comment').properties, - author: record.get('author').properties, - } - }) + const [response] = transactionRes.records.map(record => record.get('comment').properties) return response }, From 67d68db23109bae5740f600d564fe45f8ba805c1 Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Fri, 13 Sep 2019 18:26:02 +0200 Subject: [PATCH 3/8] Update post resolver to fix embarrasing bugs - when a user updates a post, we should not override every property in our database with the new params, since we have read-only properties like createdAt that we don't want to go deleting aimlessly. --- .../mentions/extractMentionedUsers.js | 2 +- .../notifications/notificationsMiddleware.js | 26 +++++-------------- backend/src/schema/resolvers/notifications.js | 2 +- backend/src/schema/resolvers/posts.js | 10 +++---- .../NotificationMenu/NotificationMenu.vue | 13 +++++----- 5 files changed, 21 insertions(+), 32 deletions(-) diff --git a/backend/src/middleware/notifications/mentions/extractMentionedUsers.js b/backend/src/middleware/notifications/mentions/extractMentionedUsers.js index e245e84b5..3ba845043 100644 --- a/backend/src/middleware/notifications/mentions/extractMentionedUsers.js +++ b/backend/src/middleware/notifications/mentions/extractMentionedUsers.js @@ -1,6 +1,6 @@ import cheerio from 'cheerio' -export default function(content) { +export default content => { if (!content) return [] const $ = cheerio.load(content) const userIds = $('a.mention[data-mention-id]') diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index effb7f4d0..ad1012c7a 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -24,13 +24,9 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { MATCH (user: User) WHERE user.id in $idsOfUsers AND NOT (user)<-[:BLOCKED]-(author) - MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) + CREATE (post)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.updatedAt = toString(datetime()) - SET ( - CASE - WHEN notification.createdAt IS NULL - THEN notification END ).createdAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } @@ -41,13 +37,9 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { WHERE user.id in $idsOfUsers AND NOT (user)<-[:BLOCKED]-(author) AND NOT (user)<-[:BLOCKED]-(postAuthor) - MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) + CREATE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.updatedAt = toString(datetime()) - SET ( - CASE - WHEN notification.createdAt IS NULL - THEN notification END ).createdAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } @@ -58,19 +50,14 @@ const notifyUsers = async (label, id, idsOfUsers, reason, context) => { WHERE user.id in $idsOfUsers AND NOT (user)<-[:BLOCKED]-(author) AND NOT (author)<-[:BLOCKED]-(user) - MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) + CREATE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE - SET notification.updatedAt = toString(datetime()) - SET ( - CASE - WHEN notification.createdAt IS NULL - THEN notification END ).createdAt = toString(datetime()) + SET notification.createdAt = toString(datetime()) ` break } } await session.run(cypher, { - label, id, idsOfUsers, reason, @@ -92,6 +79,7 @@ const handleContentDataOfPost = async (resolve, root, args, context, resolveInfo const handleContentDataOfComment = async (resolve, root, args, context, resolveInfo) => { const idsOfUsers = extractMentionedUsers(args.content) + const comment = await resolve(root, args, context, resolveInfo) if (comment) { diff --git a/backend/src/schema/resolvers/notifications.js b/backend/src/schema/resolvers/notifications.js index 0219df02c..3c1adbc5c 100644 --- a/backend/src/schema/resolvers/notifications.js +++ b/backend/src/schema/resolvers/notifications.js @@ -15,7 +15,7 @@ const transformReturnType = record => { export default { Query: { - notifications: async (parent, args, context, resolveInfo) => { + notifications: async (_parent, args, context, _resolveInfo) => { const { user: currentUser } = context const session = context.driver.session() let notifications diff --git a/backend/src/schema/resolvers/posts.js b/backend/src/schema/resolvers/posts.js index 2be17d3ec..e65fa9b76 100644 --- a/backend/src/schema/resolvers/posts.js +++ b/backend/src/schema/resolvers/posts.js @@ -74,7 +74,7 @@ export default { }, }, Mutation: { - CreatePost: async (object, params, context, resolveInfo) => { + CreatePost: async (_parent, params, context, _resolveInfo) => { const { categoryIds } = params delete params.categoryIds params = await fileUpload(params, { file: 'imageUpload', url: 'image' }) @@ -110,14 +110,14 @@ export default { return post }, - UpdatePost: async (object, params, context, resolveInfo) => { + UpdatePost: async (_parent, params, context, _resolveInfo) => { const { categoryIds } = params delete params.categoryIds params = await fileUpload(params, { file: 'imageUpload', url: 'image' }) const session = context.driver.session() let updatePostCypher = `MATCH (post:Post {id: $params.id}) - SET post = $params + SET post += $params SET post.updatedAt = toString(datetime()) ` @@ -143,12 +143,12 @@ export default { const transactionRes = await session.run(updatePostCypher, updatePostVariables) const [post] = transactionRes.records.map(record => { - return record.get('post') + return record.get('post').properties }) session.close() - return post.properties + return post }, DeletePost: async (object, args, context, resolveInfo) => { diff --git a/webapp/components/notifications/NotificationMenu/NotificationMenu.vue b/webapp/components/notifications/NotificationMenu/NotificationMenu.vue index 51b90089f..0b71f3911 100644 --- a/webapp/components/notifications/NotificationMenu/NotificationMenu.vue +++ b/webapp/components/notifications/NotificationMenu/NotificationMenu.vue @@ -1,12 +1,13 @@