From 529a8a636567324fd3197b111ef87b89e3954b8d Mon Sep 17 00:00:00 2001 From: Kapil Jain Date: Mon, 28 Oct 2019 15:40:29 -0400 Subject: [PATCH 1/6] send only one notification for mention and comment --- .../notifications/notificationsMiddleware.js | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index a494783cf..ff1f50c59 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -91,11 +91,30 @@ 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) { - await notifyUsers('Comment', comment.id, idsOfUsers, 'mentioned_in_comment', context) + const session = context.driver.session() + const cypherFindUser = ` + MATCH (user: User)-[:WROTE]->(:Post)<-[:COMMENTS]-(:Comment { id: $commentId }) + RETURN user { .id } + ` + const result = await session.run(cypherFindUser, { + commentId: comment.id, + }) + session.close() + const [postAuthor] = await result.records.map(record => { + return record.get('user') + }) + const idsOfUsersExcludingPostAuthor = idsOfUsers.filter(res => !res.equals([postAuthor])) + cosole.log('idsOfUsers1') + await notifyUsers( + 'Comment', + comment.id, + idsOfUsersExcludingPostAuthor, + 'mentioned_in_comment', + context, + ) } return comment From aecc21890eb6c9134ff04c0150d03d7ea637de38 Mon Sep 17 00:00:00 2001 From: Kapil Jain Date: Tue, 29 Oct 2019 14:20:51 -0400 Subject: [PATCH 2/6] fixed removing original post's user --- .../notifications/notificationsMiddleware.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index ff1f50c59..2b3f1c2dd 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -106,15 +106,13 @@ const handleContentDataOfComment = async (resolve, root, args, context, resolveI const [postAuthor] = await result.records.map(record => { return record.get('user') }) - const idsOfUsersExcludingPostAuthor = idsOfUsers.filter(res => !res.equals([postAuthor])) - cosole.log('idsOfUsers1') - await notifyUsers( - 'Comment', - comment.id, - idsOfUsersExcludingPostAuthor, - 'mentioned_in_comment', - context, - ) + var index = idsOfUsers.indexOf(postAuthor.id) + + if (index > -1) { + idsOfUsers.splice(index) + } + + await notifyUsers('Comment', comment.id, idsOfUsers, 'mentioned_in_comment', context) } return comment From 063ff17d8bc96a41fa38cb6bcc675bc722bdcbf6 Mon Sep 17 00:00:00 2001 From: Kapil Jain Date: Wed, 30 Oct 2019 20:39:50 -0400 Subject: [PATCH 3/6] add software tests for two notifications bug --- .../notificationsMiddleware.spec.js | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index 88f91d688..9d397cbe0 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -105,6 +105,7 @@ describe('notifications', () => { let title let postContent let postAuthor + const createPostAction = async () => { authenticatedUser = await postAuthor.toJson() await mutate({ @@ -239,6 +240,7 @@ describe('notifications', () => { describe('mentions me in a post', () => { beforeEach(async () => { title = 'Mentioning Al Capone' + postContent = 'Hey @al-capone how do you do?' }) @@ -439,7 +441,15 @@ describe('notifications', () => { }) }) - it('sends a notification', async () => { + it('sends only one notification with reason mentioned_in_comment', async () => { + postAuthor = await instance.create('User', { + id: 'postAuthor', + name: 'Mr Author', + slug: 'mr-author', + email: 'post-author@example.org', + password: '1234', + }) + await createCommentOnPostAction() const expected = expect.objectContaining({ data: { @@ -467,6 +477,53 @@ describe('notifications', () => { }), ).resolves.toEqual(expected) }) + + beforeEach(async () => { + title = 'Post where Im the author and I get mentioned in a comment' + postContent = 'Content of post where I get mentioned in a comment.' + postAuthor = notifiedUser + const createPostAction = async () => { + authenticatedUser = await postAuthor.toJson() + await mutate({ + mutation: createPostMutation, + variables: { + id: 'p49', + title, + postContent, + categoryIds, + }, + }) + authenticatedUser = await notifiedUser.toJson() + } + }) + it('sends only one notification with reason commented_on_post, no notification with reason mentioned_in_comment', async () => { + await createCommentOnPostAction() + const expected = expect.objectContaining({ + data: { + notifications: [ + { + read: false, + createdAt: expect.any(String), + reason: 'commented_on_post', + from: { + __typename: 'Comment', + id: 'c47', + content: commentContent, + }, + }, + ], + }, + }) + const { query } = createTestClient(server) + await expect( + query({ + query: notificationQuery, + variables: { + read: false, + }, + }), + ).resolves.toEqual(expected) + }) }) describe('but the author of the post blocked me', () => { From c21ad2244f1f024c5d1f68289a4be650e3f5317c Mon Sep 17 00:00:00 2001 From: Kapil Jain Date: Wed, 30 Oct 2019 22:03:52 -0400 Subject: [PATCH 4/6] add software tests for two notifications bug --- .../middleware/notifications/notificationsMiddleware.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index 9d397cbe0..5d28ec627 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -443,7 +443,7 @@ describe('notifications', () => { it('sends only one notification with reason mentioned_in_comment', async () => { postAuthor = await instance.create('User', { - id: 'postAuthor', + id: 'MrPostAuthor', name: 'Mr Author', slug: 'mr-author', email: 'post-author@example.org', From b4a9e3e551253846aec6c54bd74e7230056777cd Mon Sep 17 00:00:00 2001 From: Kapil Jain Date: Thu, 31 Oct 2019 05:08:15 -0400 Subject: [PATCH 5/6] add software tests for two notifications bug --- .../notifications/notificationsMiddleware.spec.js | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index 5d28ec627..cabcefa19 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -482,19 +482,6 @@ describe('notifications', () => { title = 'Post where Im the author and I get mentioned in a comment' postContent = 'Content of post where I get mentioned in a comment.' postAuthor = notifiedUser - const createPostAction = async () => { - authenticatedUser = await postAuthor.toJson() - await mutate({ - mutation: createPostMutation, - variables: { - id: 'p49', - title, - postContent, - categoryIds, - }, - }) - authenticatedUser = await notifiedUser.toJson() - } }) it('sends only one notification with reason commented_on_post, no notification with reason mentioned_in_comment', async () => { await createCommentOnPostAction() From cbc547a86b2599f80baba206ca25a39b29e5b5cc Mon Sep 17 00:00:00 2001 From: roschaefer Date: Thu, 31 Oct 2019 16:11:05 +0100 Subject: [PATCH 6/6] refactor: small refactoring for readability @KapilJ your solution is right now the best solution, I think. Probably the ideal solution would be if we could implement the `CreateComment` resolver in such a way that it is doing eager loading of the `comment->post->author` relationship and resolving a commment object which has the required objects all set. Then you wouldn't have to refetch all the stuff. But I think this is OK for now :+1: --- .../notifications/notificationsMiddleware.js | 37 ++++++++++--------- .../notificationsMiddleware.spec.js | 2 +- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index 2b3f1c2dd..718f0b1e4 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -1,5 +1,21 @@ import extractMentionedUsers from './mentions/extractMentionedUsers' +const postAuthorOfComment = async (comment, { context }) => { + const session = context.driver.session() + const cypherFindUser = ` + MATCH (user: User)-[:WROTE]->(:Post)<-[:COMMENTS]-(:Comment { id: $commentId }) + RETURN user { .id } + ` + const result = await session.run(cypherFindUser, { + commentId: comment.id, + }) + session.close() + const [postAuthor] = await result.records.map(record => { + return record.get('user') + }) + return postAuthor +} + const notifyUsers = async (label, id, idsOfUsers, reason, context) => { if (!idsOfUsers.length) return @@ -90,27 +106,12 @@ const handleContentDataOfPost = async (resolve, root, args, context, resolveInfo } const handleContentDataOfComment = async (resolve, root, args, context, resolveInfo) => { - const idsOfUsers = extractMentionedUsers(args.content) + let idsOfUsers = extractMentionedUsers(args.content) const comment = await resolve(root, args, context, resolveInfo) if (comment) { - const session = context.driver.session() - const cypherFindUser = ` - MATCH (user: User)-[:WROTE]->(:Post)<-[:COMMENTS]-(:Comment { id: $commentId }) - RETURN user { .id } - ` - const result = await session.run(cypherFindUser, { - commentId: comment.id, - }) - session.close() - const [postAuthor] = await result.records.map(record => { - return record.get('user') - }) - var index = idsOfUsers.indexOf(postAuthor.id) - - if (index > -1) { - idsOfUsers.splice(index) - } + const postAuthor = await postAuthorOfComment(comment, { context }) + idsOfUsers = idsOfUsers.filter(id => id !== postAuthor.id) await notifyUsers('Comment', comment.id, idsOfUsers, 'mentioned_in_comment', context) } diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index cabcefa19..18ee998db 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -479,7 +479,7 @@ describe('notifications', () => { }) beforeEach(async () => { - title = 'Post where Im the author and I get mentioned in a comment' + title = "Post where I'm the author and I get mentioned in a comment" postContent = 'Content of post where I get mentioned in a comment.' postAuthor = notifiedUser })