From f32bfc7e36a0c0eeba4e46b264a1c614564e3e55 Mon Sep 17 00:00:00 2001 From: roschaefer Date: Tue, 18 Feb 2020 15:20:48 +0100 Subject: [PATCH 1/4] fix(subscriptions): Don't publish undefined Fix #3088 --- .../notifications/notificationsMiddleware.js | 34 ++++++---- .../notificationsMiddleware.spec.js | 66 +++++++++++++++---- 2 files changed, 75 insertions(+), 25 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index 4636b8e9f..26c27bb10 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -2,11 +2,21 @@ import extractMentionedUsers from './mentions/extractMentionedUsers' import { validateNotifyUsers } from '../validation/validationMiddleware' import { pubsub, NOTIFICATION_ADDED } from '../../server' +const publishNotifications = async (...promises) => { + const notifications = await Promise.all(promises) + notifications + .flat() + .forEach(notificationAdded => pubsub.publish(NOTIFICATION_ADDED, { notificationAdded })) +} + const handleContentDataOfPost = async (resolve, root, args, context, resolveInfo) => { const idsOfUsers = extractMentionedUsers(args.content) const post = await resolve(root, args, context, resolveInfo) - if (post && idsOfUsers && idsOfUsers.length) - await notifyUsersOfMention('Post', post.id, idsOfUsers, 'mentioned_in_post', context) + if (post) { + await publishNotifications( + notifyUsersOfMention('Post', post.id, idsOfUsers, 'mentioned_in_post', context), + ) + } return post } @@ -16,10 +26,10 @@ const handleContentDataOfComment = async (resolve, root, args, context, resolveI const comment = await resolve(root, args, context, resolveInfo) const [postAuthor] = await postAuthorOfComment(comment.id, { context }) idsOfUsers = idsOfUsers.filter(id => id !== postAuthor.id) - if (idsOfUsers && idsOfUsers.length) - await notifyUsersOfMention('Comment', comment.id, idsOfUsers, 'mentioned_in_comment', context) - if (context.user.id !== postAuthor.id) - await notifyUsersOfComment('Comment', comment.id, postAuthor.id, 'commented_on_post', context) + await publishNotifications( + notifyUsersOfMention('Comment', comment.id, idsOfUsers, 'mentioned_in_comment', context), + notifyUsersOfComment('Comment', comment.id, postAuthor.id, 'commented_on_post', context), + ) return comment } @@ -29,7 +39,7 @@ const postAuthorOfComment = async (commentId, { context }) => { try { postAuthorId = await session.readTransaction(transaction => { return transaction.run( - ` + ` MATCH (author:User)-[:WROTE]->(:Post)<-[:COMMENTS]-(:Comment { id: $commentId }) RETURN author { .id } as authorId `, @@ -43,6 +53,7 @@ const postAuthorOfComment = async (commentId, { context }) => { } const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { + if (!(idsOfUsers && idsOfUsers.length)) return [] await validateNotifyUsers(label, reason) let mentionedCypher switch (reason) { @@ -91,8 +102,8 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { return notificationTransactionResponse.records.map(record => record.get('notification')) }) try { - const [notification] = await writeTxResultPromise - return pubsub.publish(NOTIFICATION_ADDED, { notificationAdded: notification }) + const notifications = await writeTxResultPromise + return notifications } catch (error) { throw new Error(error) } finally { @@ -101,6 +112,7 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { } const notifyUsersOfComment = async (label, commentId, postAuthorId, reason, context) => { + if (!(context.user.id !== postAuthorId)) return [] await validateNotifyUsers(label, reason) const session = context.driver.session() const writeTxResultPromise = await session.writeTransaction(async transaction => { @@ -121,8 +133,8 @@ const notifyUsersOfComment = async (label, commentId, postAuthorId, reason, cont return notificationTransactionResponse.records.map(record => record.get('notification')) }) try { - const [notification] = await writeTxResultPromise - return pubsub.publish(NOTIFICATION_ADDED, { notificationAdded: notification }) + const notifications = await writeTxResultPromise + return notifications } finally { session.close() } diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index 95c0037b8..16e33bdfb 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -2,9 +2,10 @@ import { gql } from '../../helpers/jest' import { cleanDatabase } from '../../db/factories' import { createTestClient } from 'apollo-server-testing' import { getNeode, getDriver } from '../../db/neo4j' -import createServer from '../../server' +import createServer, { pubsub } from '../../server' let server, query, mutate, notifiedUser, authenticatedUser +let publishSpy const driver = getDriver() const neode = getNeode() const categoryIds = ['cat9'] @@ -36,6 +37,7 @@ const createCommentMutation = gql` beforeAll(async () => { await cleanDatabase() + publishSpy = jest.spyOn(pubsub, 'publish') const createServerResult = createServer({ context: () => { return { @@ -52,6 +54,7 @@ beforeAll(async () => { }) beforeEach(async () => { + publishSpy.mockClear() notifiedUser = await neode.create( 'User', { @@ -259,7 +262,15 @@ describe('notifications', () => { await createPostAction() const expectedContent = 'Hey @al-capone how do you do?' - const expected = expect.objectContaining({ + await expect( + query({ + query: notificationQuery, + variables: { + read: false, + }, + }), + ).resolves.toMatchObject({ + errors: undefined, data: { notifications: [ { @@ -275,15 +286,22 @@ describe('notifications', () => { ], }, }) + }) - await expect( - query({ - query: notificationQuery, - variables: { - read: false, - }, + it('publishes `NOTIFICATION_ADDED` to me', async () => { + await createPostAction() + expect(publishSpy).toHaveBeenCalledWith( + 'NOTIFICATION_ADDED', + expect.objectContaining({ + notificationAdded: expect.objectContaining({ + reason: 'mentioned_in_post', + to: expect.objectContaining({ + id: 'you', + }), + }), }), - ).resolves.toEqual(expected) + ) + expect(publishSpy).toHaveBeenCalledTimes(1) }) describe('updates the post and mentions me again', () => { @@ -429,6 +447,11 @@ describe('notifications', () => { }), ).resolves.toEqual(expected) }) + + it('does not publish `NOTIFICATION_ADDED`', async () => { + await createPostAction() + expect(publishSpy).not.toHaveBeenCalled() + }) }) }) @@ -554,10 +577,6 @@ describe('notifications', () => { it('sends no notification', async () => { await createCommentOnPostAction() - const expected = expect.objectContaining({ - data: { notifications: [] }, - }) - await expect( query({ query: notificationQuery, @@ -565,7 +584,26 @@ describe('notifications', () => { read: false, }, }), - ).resolves.toEqual(expected) + ).resolves.toMatchObject({ + data: { notifications: [] }, + errors: undefined, + }) + }) + + it('does not publish `NOTIFICATION_ADDED` to authenticated user', async () => { + await createCommentOnPostAction() + expect(publishSpy).toHaveBeenCalledWith( + 'NOTIFICATION_ADDED', + expect.objectContaining({ + notificationAdded: expect.objectContaining({ + reason: 'commented_on_post', + to: expect.objectContaining({ + id: 'postAuthor', // that's expected, it's not me but the post author + }), + }), + }), + ) + expect(publishSpy).toHaveBeenCalledTimes(1) }) }) }) From 7fe6fb110b795095095d3d9d5517e45e700a998a Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 18 Feb 2020 22:21:49 +0100 Subject: [PATCH 2/4] Remove unnecessary negation of negation, refactor - tests to use `toMatchObject`, which checks that only one notification is indeed created --- .../src/middleware/notifications/notificationsMiddleware.js | 2 +- .../notifications/notificationsMiddleware.spec.js | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index 26c27bb10..64eca97c8 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -112,7 +112,7 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { } const notifyUsersOfComment = async (label, commentId, postAuthorId, reason, context) => { - if (!(context.user.id !== postAuthorId)) return [] + if (context.user.id === postAuthorId) return [] await validateNotifyUsers(label, reason) const session = context.driver.session() const writeTxResultPromise = await session.writeTransaction(async transaction => { diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.js b/backend/src/middleware/notifications/notificationsMiddleware.spec.js index 16e33bdfb..af4ed9693 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.js @@ -528,7 +528,7 @@ describe('notifications', () => { }) it('sends only one notification with reason commented_on_post, no notification with reason mentioned_in_comment', async () => { await createCommentOnPostAction() - const expected = expect.objectContaining({ + const expected = { data: { notifications: [ { @@ -543,7 +543,7 @@ describe('notifications', () => { }, ], }, - }) + } await expect( query({ @@ -552,7 +552,7 @@ describe('notifications', () => { read: false, }, }), - ).resolves.toEqual(expected) + ).resolves.toMatchObject(expected, { errors: undefined }) }) }) From 54caaaaa0a5ae6d450f0b07f2be64afac20ddbce Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 18 Feb 2020 22:43:19 +0100 Subject: [PATCH 3/4] Remove unnecessary guard clause - filter returns an empty array, and no notification is created if idsOfUsers is an empty array --- backend/src/middleware/notifications/notificationsMiddleware.js | 1 - 1 file changed, 1 deletion(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index 64eca97c8..f84e7d392 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -53,7 +53,6 @@ const postAuthorOfComment = async (commentId, { context }) => { } const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { - if (!(idsOfUsers && idsOfUsers.length)) return [] await validateNotifyUsers(label, reason) let mentionedCypher switch (reason) { From a346632fffbfa92e3457627d25ab7a13d21622e9 Mon Sep 17 00:00:00 2001 From: mattwr18 Date: Tue, 18 Feb 2020 23:19:14 +0100 Subject: [PATCH 4/4] Avoid running cypher statement if not needed - it just feels safer to not run a cypher statement if there is no user in the idsOfUsers array. Why risk notifications pointing to empty nodes? It doesn't seem like this happens, but I'm not sure how to write a test to verify, and why run cypher statements, if there is no need to? --- backend/src/middleware/notifications/notificationsMiddleware.js | 1 + 1 file changed, 1 insertion(+) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.js b/backend/src/middleware/notifications/notificationsMiddleware.js index f84e7d392..64eca97c8 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.js +++ b/backend/src/middleware/notifications/notificationsMiddleware.js @@ -53,6 +53,7 @@ const postAuthorOfComment = async (commentId, { context }) => { } const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { + if (!(idsOfUsers && idsOfUsers.length)) return [] await validateNotifyUsers(label, reason) let mentionedCypher switch (reason) {