From 649491f7cbe312fb4101bc32e6a4d81af94c38e0 Mon Sep 17 00:00:00 2001 From: Ulf Gebhardt Date: Thu, 24 Apr 2025 00:58:53 +0200 Subject: [PATCH] fix(backend): fix notification emails with different name (#8419) * fix diffent name notifications We had emails sent with incorrect names. This PR combines the query for the email with the user the notification is sent to since the notification in database was correct. The underlying problem is the unstable order in which the database can return values. The results of the two queries were matched by id since it was assumed that they always return the same order of elements. lint fixes fix typo fix factory fix tests * fix tests accoridng to review also test for the right amount of emails in every test --- .../notificationsMiddleware.emails.spec.ts | 86 +++++++---- ...ficationsMiddleware.followed-users.spec.ts | 80 ++++++++-- ...tionsMiddleware.mentions-in-groups.spec.ts | 71 +++++++-- ...icationsMiddleware.observing-posts.spec.ts | 74 +++++++-- ...ificationsMiddleware.online-status.spec.ts | 6 +- ...icationsMiddleware.posts-in-groups.spec.ts | 30 +++- .../notificationsMiddleware.spec.ts | 42 ++--- .../notifications/notificationsMiddleware.ts | 146 +++++++----------- 8 files changed, 355 insertions(+), 180 deletions(-) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.emails.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.emails.spec.ts index 78c95b454..55edef940 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.emails.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.emails.spec.ts @@ -16,20 +16,21 @@ import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) -let server, query, mutate, authenticatedUser +let server, query, mutate, authenticatedUser, emaillessMember let postAuthor, groupMember const driver = getDriver() const neode = getNeode() -const mentionString = - '@group-member' +const mentionString = ` + @group-member + @email-less-member` const createPostMutation = gql` mutation ($id: ID, $title: String!, $content: String!, $groupId: ID) { @@ -148,6 +149,11 @@ describe('emails sent for notifications', () => { password: '1234', }, ) + emaillessMember = await neode.create('User', { + id: 'email-less-member', + name: 'Email-less Member', + slug: 'email-less-member', + }) authenticatedUser = await postAuthor.toJson() await mutate({ mutation: createGroupMutation(), @@ -171,6 +177,18 @@ describe('emails sent for notifications', () => { mutation: followUserMutation, variables: { id: 'post-author' }, }) + authenticatedUser = await emaillessMember.toJson() + await mutate({ + mutation: joinGroupMutation(), + variables: { + groupId: 'public-group', + userId: 'group-member', + }, + }) + await mutate({ + mutation: followUserMutation, + variables: { id: 'post-author' }, + }) }) afterEach(async () => { @@ -188,7 +206,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -213,7 +231,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'post_in_group', @@ -225,7 +243,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'followed_user_posted', @@ -237,7 +255,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_post', @@ -260,7 +278,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -285,7 +303,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'post_in_group', @@ -297,7 +315,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'followed_user_posted', @@ -309,7 +327,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_post', @@ -333,7 +351,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -358,7 +376,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'post_in_group', @@ -370,7 +388,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'followed_user_posted', @@ -382,7 +400,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_post', @@ -407,7 +425,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -432,7 +450,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'post_in_group', @@ -444,7 +462,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'followed_user_posted', @@ -456,7 +474,7 @@ describe('emails sent for notifications', () => { __typename: 'Post', id: 'post', content: - 'Hello, @group-member, my trusty follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_post', @@ -481,7 +499,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -501,7 +519,7 @@ describe('emails sent for notifications', () => { mutation: createCommentMutation, variables: { id: 'comment-2', - content: `Hello, ${mentionString}, my beloved follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, postId: 'post', }, }) @@ -529,7 +547,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'commented_on_post', @@ -541,7 +559,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_comment', @@ -563,7 +581,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -583,7 +601,7 @@ describe('emails sent for notifications', () => { mutation: createCommentMutation, variables: { id: 'comment-2', - content: `Hello, ${mentionString}, my beloved follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, postId: 'post', }, }) @@ -611,7 +629,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'commented_on_post', @@ -623,7 +641,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_comment', @@ -646,7 +664,7 @@ describe('emails sent for notifications', () => { variables: { id: 'post', title: 'This is the post', - content: `Hello, ${mentionString}, my trusty follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, groupId: 'public-group', }, }) @@ -666,7 +684,7 @@ describe('emails sent for notifications', () => { mutation: createCommentMutation, variables: { id: 'comment-2', - content: `Hello, ${mentionString}, my beloved follower.`, + content: `Hello, ${mentionString}, my trusty followers.`, postId: 'post', }, }) @@ -694,7 +712,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'commented_on_post', @@ -706,7 +724,7 @@ describe('emails sent for notifications', () => { __typename: 'Comment', id: 'comment-2', content: - 'Hello, @group-member, my beloved follower.', + 'Hello,
@group-member
@email-less-member, my trusty followers.', }, read: false, reason: 'mentioned_in_comment', diff --git a/backend/src/middleware/notifications/notificationsMiddleware.followed-users.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.followed-users.spec.ts index 5be4ea5b5..18da6bff8 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.followed-users.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.followed-users.spec.ts @@ -14,14 +14,14 @@ import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) let server, query, mutate, authenticatedUser -let postAuthor, firstFollower, secondFollower +let postAuthor, firstFollower, secondFollower, thirdFollower, emaillessFollower const driver = getDriver() const neode = getNeode() @@ -107,7 +107,7 @@ describe('following users notifications', () => { slug: 'post-author', }, { - email: 'test@example.org', + email: 'post-author@example.org', password: '1234', }, ) @@ -119,7 +119,7 @@ describe('following users notifications', () => { slug: 'first-follower', }, { - email: 'test2@example.org', + email: 'first-follower@example.org', password: '1234', }, ) @@ -131,10 +131,27 @@ describe('following users notifications', () => { slug: 'second-follower', }, { - email: 'test3@example.org', + email: 'second-follower@example.org', password: '1234', }, ) + thirdFollower = await Factory.build( + 'user', + { + id: 'third-follower', + name: 'Third Follower', + slug: 'third-follower', + }, + { + email: 'third-follower@example.org', + password: '1234', + }, + ) + emaillessFollower = await neode.create('User', { + id: 'email-less-follower', + name: 'Email-less Follower', + slug: 'email-less-follower', + }) await secondFollower.update({ emailNotificationsFollowingUsers: false }) authenticatedUser = await firstFollower.toJson() await mutate({ @@ -146,6 +163,16 @@ describe('following users notifications', () => { mutation: followUserMutation, variables: { id: 'post-author' }, }) + authenticatedUser = await thirdFollower.toJson() + await mutate({ + mutation: followUserMutation, + variables: { id: 'post-author' }, + }) + authenticatedUser = await emaillessFollower.toJson() + await mutate({ + mutation: followUserMutation, + variables: { id: 'post-author' }, + }) jest.clearAllMocks() }) @@ -221,8 +248,43 @@ describe('following users notifications', () => { }) }) - it('sends only one email, as second follower has emails disabled', () => { - expect(sendMailMock).toHaveBeenCalledTimes(1) + it('sends notification to the email-less follower', async () => { + authenticatedUser = await emaillessFollower.toJson() + await expect( + query({ + query: notificationQuery, + }), + ).resolves.toMatchObject({ + data: { + notifications: [ + { + from: { + __typename: 'Post', + id: 'post', + }, + read: false, + reason: 'followed_user_posted', + }, + ], + }, + errors: undefined, + }) + }) + + it('sends only two emails, as second follower has emails disabled and email-less follower has no email', () => { + expect(sendMailMock).toHaveBeenCalledTimes(2) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + html: expect.stringContaining('Hello First Follower'), + to: 'first-follower@example.org', + }), + ) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + html: expect.stringContaining('Hello Third Follower'), + to: 'third-follower@example.org', + }), + ) }) }) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.mentions-in-groups.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.mentions-in-groups.spec.ts index 7058efd25..1c7ca4c71 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.mentions-in-groups.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.mentions-in-groups.spec.ts @@ -17,22 +17,23 @@ import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) let server, query, mutate, authenticatedUser -let postAuthor, groupMember, pendingMember, noMember +let postAuthor, groupMember, pendingMember, noMember, emaillessMember const driver = getDriver() const neode = getNeode() const mentionString = ` - @no-meber + @no-member @pending-member @group-member. + @email-less-member. ` const createPostMutation = gql` @@ -168,6 +169,12 @@ describe('mentions in groups', () => { password: '1234', }, ) + emaillessMember = await neode.create('User', { + id: 'email-less-member', + name: 'Email-less Member', + slug: 'email-less-member', + }) + authenticatedUser = await postAuthor.toJson() await mutate({ mutation: createGroupMutation(), @@ -243,6 +250,28 @@ describe('mentions in groups', () => { userId: 'pending-member', }, }) + authenticatedUser = await emaillessMember.toJson() + await mutate({ + mutation: joinGroupMutation(), + variables: { + groupId: 'public-group', + userId: 'group-member', + }, + }) + await mutate({ + mutation: joinGroupMutation(), + variables: { + groupId: 'closed-group', + userId: 'group-member', + }, + }) + await mutate({ + mutation: joinGroupMutation(), + variables: { + groupId: 'hidden-group', + userId: 'group-member', + }, + }) authenticatedUser = await postAuthor.toJson() await mutate({ mutation: changeGroupMemberRoleMutation(), @@ -260,8 +289,26 @@ describe('mentions in groups', () => { roleInGroup: 'usual', }, }) + await mutate({ + mutation: changeGroupMemberRoleMutation(), + variables: { + groupId: 'closed-group', + userId: 'email-less-member', + roleInGroup: 'usual', + }, + }) + await mutate({ + mutation: changeGroupMemberRoleMutation(), + variables: { + groupId: 'hidden-group', + userId: 'email-less-member', + roleInGroup: 'usual', + }, + }) authenticatedUser = await groupMember.toJson() await markAllAsRead() + authenticatedUser = await emaillessMember.toJson() + await markAllAsRead() }) afterEach(async () => { @@ -327,7 +374,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'public-post', content: - 'Hey
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'post_in_group', @@ -339,7 +386,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'public-post', content: - 'Hey
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'mentioned_in_post', @@ -351,7 +398,7 @@ describe('mentions in groups', () => { }) }) - it('sends 3 emails, one for each user', () => { + it('sends only 3 emails, one for each user with an email', () => { expect(sendMailMock).toHaveBeenCalledTimes(3) }) }) @@ -423,7 +470,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'closed-post', content: - 'Hey members
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey members
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'post_in_group', @@ -435,7 +482,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'closed-post', content: - 'Hey members
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey members
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'mentioned_in_post', @@ -519,7 +566,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'hidden-post', content: - 'Hey hiders
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey hiders
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'post_in_group', @@ -531,7 +578,7 @@ describe('mentions in groups', () => { __typename: 'Post', id: 'hidden-post', content: - 'Hey hiders
@no-meber
@pending-member
@group-member.
! Please read this', + 'Hey hiders
@no-member
@pending-member
@group-member.
@email-less-member.
! Please read this', }, read: false, reason: 'mentioned_in_post', diff --git a/backend/src/middleware/notifications/notificationsMiddleware.observing-posts.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.observing-posts.spec.ts index 2c73d2beb..9f193eaeb 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.observing-posts.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.observing-posts.spec.ts @@ -6,15 +6,20 @@ import { createTestClient } from 'apollo-server-testing' import gql from 'graphql-tag' import CONFIG from '@config/index' -import { cleanDatabase } from '@db/factories' +import Factory, { cleanDatabase } from '@db/factories' import { getNeode, getDriver } from '@db/neo4j' import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), +})) + let server, query, mutate, authenticatedUser -let postAuthor, firstCommenter, secondCommenter +let postAuthor, firstCommenter, secondCommenter, emaillessObserver const driver = getDriver() const neode = getNeode() @@ -102,42 +107,47 @@ afterAll(async () => { describe('notifications for users that observe a post', () => { beforeAll(async () => { - postAuthor = await neode.create( - 'User', + postAuthor = await Factory.build( + 'user', { id: 'post-author', name: 'Post Author', slug: 'post-author', }, { - email: 'test@example.org', + email: 'post-author@example.org', password: '1234', }, ) - firstCommenter = await neode.create( - 'User', + firstCommenter = await Factory.build( + 'user', { id: 'first-commenter', name: 'First Commenter', slug: 'first-commenter', }, { - email: 'test2@example.org', + email: 'first-commenter@example.org', password: '1234', }, ) - secondCommenter = await neode.create( - 'User', + secondCommenter = await Factory.build( + 'user', { id: 'second-commenter', name: 'Second Commenter', slug: 'second-commenter', }, { - email: 'test3@example.org', + email: 'second-commenter@example.org', password: '1234', }, ) + emaillessObserver = await neode.create('User', { + id: 'email-less-observer', + name: 'Email-less Observer', + slug: 'email-less-observer', + }) authenticatedUser = await postAuthor.toJson() await mutate({ mutation: createPostMutation, @@ -147,6 +157,14 @@ describe('notifications for users that observe a post', () => { content: 'This is the content of the post', }, }) + authenticatedUser = await emaillessObserver.toJson() + await mutate({ + mutation: toggleObservePostMutation, + variables: { + id: 'post', + value: true, + }, + }) }) describe('first comment on the post', () => { @@ -198,8 +216,18 @@ describe('notifications for users that observe a post', () => { }) }) + it('sends one email', () => { + expect(sendMailMock).toHaveBeenCalledTimes(1) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + to: 'post-author@example.org', + }), + ) + }) + describe('second comment on post', () => { beforeAll(async () => { + jest.clearAllMocks() authenticatedUser = await secondCommenter.toJson() await mutate({ mutation: createCommentMutation, @@ -277,10 +305,25 @@ describe('notifications for users that observe a post', () => { errors: undefined, }) }) + + it('sends two emails', () => { + expect(sendMailMock).toHaveBeenCalledTimes(2) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + to: 'post-author@example.org', + }), + ) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + to: 'first-commenter@example.org', + }), + ) + }) }) describe('first commenter unfollows the post and post author comments post', () => { beforeAll(async () => { + jest.clearAllMocks() authenticatedUser = await firstCommenter.toJson() await mutate({ mutation: toggleObservePostMutation, @@ -376,6 +419,15 @@ describe('notifications for users that observe a post', () => { errors: undefined, }) }) + + it('sends one email', () => { + expect(sendMailMock).toHaveBeenCalledTimes(1) + expect(sendMailMock).toHaveBeenCalledWith( + expect.objectContaining({ + to: 'second-commenter@example.org', + }), + ) + }) }) }) }) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.online-status.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.online-status.spec.ts index da9a6b250..47842029c 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.online-status.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.online-status.spec.ts @@ -13,9 +13,9 @@ import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) let isUserOnlineMock = jest.fn().mockReturnValue(false) diff --git a/backend/src/middleware/notifications/notificationsMiddleware.posts-in-groups.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.posts-in-groups.spec.ts index 9ca4ae7ab..6bde0aee2 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.posts-in-groups.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.posts-in-groups.spec.ts @@ -17,14 +17,14 @@ import createServer from '@src/server' CONFIG.CATEGORIES_ACTIVE = false -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) let server, query, mutate, authenticatedUser -let postAuthor, groupMember, pendingMember +let postAuthor, groupMember, pendingMember, emaillessMember const driver = getDriver() const neode = getNeode() @@ -159,6 +159,12 @@ describe('notify group members of new posts in group', () => { password: '1234', }, ) + emaillessMember = await neode.create('User', { + id: 'email-less-member', + name: 'Email-less Member', + slug: 'email-less-member', + }) + authenticatedUser = await postAuthor.toJson() await mutate({ mutation: createGroupMutation(), @@ -186,6 +192,14 @@ describe('notify group members of new posts in group', () => { userId: 'pending-member', }, }) + authenticatedUser = await emaillessMember.toJson() + await mutate({ + mutation: joinGroupMutation(), + variables: { + groupId: 'g-1', + userId: 'group-member', + }, + }) authenticatedUser = await postAuthor.toJson() await mutate({ mutation: changeGroupMemberRoleMutation(), @@ -195,6 +209,14 @@ describe('notify group members of new posts in group', () => { roleInGroup: 'usual', }, }) + await mutate({ + mutation: changeGroupMemberRoleMutation(), + variables: { + groupId: 'g-1', + userId: 'email-less-member', + roleInGroup: 'usual', + }, + }) }) afterEach(async () => { diff --git a/backend/src/middleware/notifications/notificationsMiddleware.spec.ts b/backend/src/middleware/notifications/notificationsMiddleware.spec.ts index 908ccac22..31e458e2a 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.spec.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.spec.ts @@ -18,9 +18,9 @@ import { leaveGroupMutation } from '@graphql/queries/leaveGroupMutation' import { removeUserFromGroupMutation } from '@graphql/queries/removeUserFromGroupMutation' import createServer, { pubsub } from '@src/server' -const sendMailMock = jest.fn() -jest.mock('../helpers/email/sendMail', () => ({ - sendMail: () => sendMailMock(), +const sendMailMock: (notification) => void = jest.fn() +jest.mock('@middleware/helpers/email/sendMail', () => ({ + sendMail: (notification) => sendMailMock(notification), })) const chatMessageTemplateMock = jest.fn() @@ -195,8 +195,8 @@ describe('notifications', () => { beforeEach(async () => { jest.clearAllMocks() commentContent = 'Commenters comment.' - commentAuthor = await neode.create( - 'User', + commentAuthor = await Factory.build( + 'user', { id: 'commentAuthor', name: 'Mrs Comment', @@ -345,8 +345,8 @@ describe('notifications', () => { beforeEach(async () => { jest.clearAllMocks() - postAuthor = await neode.create( - 'User', + postAuthor = await Factory.build( + 'user', { id: 'postAuthor', name: 'Mrs Post', @@ -658,8 +658,8 @@ describe('notifications', () => { beforeEach(async () => { commentContent = 'One mention about me with @al-capone.' - commentAuthor = await neode.create( - 'User', + commentAuthor = await Factory.build( + 'user', { id: 'commentAuthor', name: 'Mrs Comment', @@ -673,15 +673,15 @@ describe('notifications', () => { }) it('sends only one notification with reason mentioned_in_comment', async () => { - postAuthor = await neode.create( - 'User', + postAuthor = await Factory.build( + 'user', { id: 'MrPostAuthor', name: 'Mr Author', slug: 'mr-author', }, { - email: 'post-author@example.org', + email: 'post-author2@example.org', password: '1234', }, ) @@ -756,8 +756,8 @@ describe('notifications', () => { await postAuthor.relateTo(notifiedUser, 'blocked') commentContent = 'One mention about me with @al-capone.' - commentAuthor = await neode.create( - 'User', + commentAuthor = await Factory.build( + 'user', { id: 'commentAuthor', name: 'Mrs Comment', @@ -807,8 +807,8 @@ describe('notifications', () => { await postAuthor.relateTo(notifiedUser, 'muted') commentContent = 'One mention about me with @al-capone.' - commentAuthor = await neode.create( - 'User', + commentAuthor = await Factory.build( + 'user', { id: 'commentAuthor', name: 'Mrs Comment', @@ -879,8 +879,8 @@ describe('notifications', () => { beforeEach(async () => { jest.clearAllMocks() - chatSender = await neode.create( - 'User', + chatSender = await Factory.build( + 'user', { id: 'chatSender', name: 'chatSender', @@ -931,7 +931,7 @@ describe('notifications', () => { content: 'Some nice message to chatReceiver', senderId: 'chatSender', username: 'chatSender', - avatar: null, + avatar: expect.any(String), date: expect.any(String), saved: true, distributed: false, @@ -967,7 +967,7 @@ describe('notifications', () => { content: 'Some nice message to chatReceiver', senderId: 'chatSender', username: 'chatSender', - avatar: null, + avatar: expect.any(String), date: expect.any(String), saved: true, distributed: false, @@ -1046,7 +1046,7 @@ describe('notifications', () => { content: 'Some nice message to chatReceiver', senderId: 'chatSender', username: 'chatSender', - avatar: null, + avatar: expect.any(String), date: expect.any(String), saved: true, distributed: false, diff --git a/backend/src/middleware/notifications/notificationsMiddleware.ts b/backend/src/middleware/notifications/notificationsMiddleware.ts index f7be031c8..b9f5c4284 100644 --- a/backend/src/middleware/notifications/notificationsMiddleware.ts +++ b/backend/src/middleware/notifications/notificationsMiddleware.ts @@ -18,59 +18,28 @@ import { pubsub, NOTIFICATION_ADDED, ROOM_COUNT_UPDATED, CHAT_MESSAGE_ADDED } fr import extractMentionedUsers from './mentions/extractMentionedUsers' -const queryNotificationEmails = async (context, notificationUserIds) => { - if (!notificationUserIds?.length) return [] - const userEmailCypher = ` - MATCH (user: User) - // blocked users are filtered out from notifications already - WHERE user.id in $notificationUserIds - WITH user - MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) - RETURN emailAddress {.email} - ` - const session = context.driver.session() - const writeTxResultPromise = session.readTransaction(async (transaction) => { - const emailAddressTransactionResponse = await transaction.run(userEmailCypher, { - notificationUserIds, - }) - return emailAddressTransactionResponse.records.map((record) => record.get('emailAddress')) - }) - try { - const emailAddresses = await writeTxResultPromise - return emailAddresses - } catch (error) { - throw new Error(error) - } finally { - session.close() - } -} - const publishNotifications = async ( context, - promises, + notificationsPromise, emailNotificationSetting: string, emailsSent: string[] = [], ): Promise => { - let notifications = await Promise.all(promises) - notifications = notifications.flat() - const notificationsEmailAddresses = await queryNotificationEmails( - context, - notifications.map((notification) => notification.to.id), - ) - notifications.forEach((notificationAdded, index) => { + const notifications = await notificationsPromise + notifications.forEach((notificationAdded) => { pubsub.publish(NOTIFICATION_ADDED, { notificationAdded }) if ( + notificationAdded.email && // no primary email was found (notificationAdded.to[emailNotificationSetting] ?? true) && !isUserOnline(notificationAdded.to) && - !emailsSent.includes(notificationsEmailAddresses[index].email) + !emailsSent.includes(notificationAdded.email) ) { sendMail( notificationTemplate({ - email: notificationsEmailAddresses[index].email, + email: notificationAdded.email, variables: { notification: notificationAdded }, }), ) - emailsSent.push(notificationsEmailAddresses[index].email) + emailsSent.push(notificationAdded.email) } }) return emailsSent @@ -82,7 +51,7 @@ const handleJoinGroup = async (resolve, root, args, context, resolveInfo) => { if (user) { await publishNotifications( context, - [notifyOwnersOfGroup(groupId, userId, 'user_joined_group', context)], + notifyOwnersOfGroup(groupId, userId, 'user_joined_group', context), 'emailNotificationsGroupMemberJoined', ) } @@ -95,7 +64,7 @@ const handleLeaveGroup = async (resolve, root, args, context, resolveInfo) => { if (user) { await publishNotifications( context, - [notifyOwnersOfGroup(groupId, userId, 'user_left_group', context)], + notifyOwnersOfGroup(groupId, userId, 'user_left_group', context), 'emailNotificationsGroupMemberLeft', ) } @@ -108,7 +77,7 @@ const handleChangeGroupMemberRole = async (resolve, root, args, context, resolve if (user) { await publishNotifications( context, - [notifyMemberOfGroup(groupId, userId, 'changed_group_member_role', context)], + notifyMemberOfGroup(groupId, userId, 'changed_group_member_role', context), 'emailNotificationsGroupMemberRoleChanged', ) } @@ -121,7 +90,7 @@ const handleRemoveUserFromGroup = async (resolve, root, args, context, resolveIn if (user) { await publishNotifications( context, - [notifyMemberOfGroup(groupId, userId, 'removed_user_from_group', context)], + notifyMemberOfGroup(groupId, userId, 'removed_user_from_group', context), 'emailNotificationsGroupMemberRemoved', ) } @@ -135,20 +104,20 @@ const handleContentDataOfPost = async (resolve, root, args, context, resolveInfo if (post) { const sentEmails: string[] = await publishNotifications( context, - [notifyUsersOfMention('Post', post.id, idsOfUsers, 'mentioned_in_post', context)], + notifyUsersOfMention('Post', post.id, idsOfUsers, 'mentioned_in_post', context), 'emailNotificationsMention', ) sentEmails.concat( await publishNotifications( context, - [notifyFollowingUsers(post.id, groupId, context)], + notifyFollowingUsers(post.id, groupId, context), 'emailNotificationsFollowingUsers', sentEmails, ), ) await publishNotifications( context, - [notifyGroupMembersOfNewPost(post.id, groupId, context)], + notifyGroupMembersOfNewPost(post.id, groupId, context), 'emailNotificationsPostInGroup', sentEmails, ) @@ -164,20 +133,18 @@ const handleContentDataOfComment = async (resolve, root, args, context, resolveI idsOfMentionedUsers = idsOfMentionedUsers.filter((id) => id !== postAuthor.id) const sentEmails: string[] = await publishNotifications( context, - [ - notifyUsersOfMention( - 'Comment', - comment.id, - idsOfMentionedUsers, - 'mentioned_in_comment', - context, - ), - ], + notifyUsersOfMention( + 'Comment', + comment.id, + idsOfMentionedUsers, + 'mentioned_in_comment', + context, + ), 'emailNotificationsMention', ) await publishNotifications( context, - [notifyUsersOfComment('Comment', comment.id, 'commented_on_post', context)], + notifyUsersOfComment('Comment', comment.id, 'commented_on_post', context), 'emailNotificationsCommentOnObservedPost', sentEmails, ) @@ -208,17 +175,20 @@ const notifyFollowingUsers = async (postId, groupId, context) => { const cypher = ` MATCH (post:Post { id: $postId })<-[:WROTE]-(author:User { id: $userId })<-[:FOLLOWS]-(user:User) OPTIONAL MATCH (post)-[:IN]->(group:Group { id: $groupId }) - WITH post, author, user, group WHERE group IS NULL OR group.groupType = 'public' + OPTIONAL MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) + WITH post, author, user, emailAddress, group + WHERE group IS NULL OR group.groupType = 'public' MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) - WITH notification, author, user, + WITH notification, author, user, emailAddress.email as email, post {.*, author: properties(author) } AS finalResource RETURN notification { .*, from: finalResource, to: properties(user), + email: email, relatedUser: properties(author) } ` @@ -233,8 +203,7 @@ const notifyFollowingUsers = async (postId, groupId, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } catch (error) { throw new Error(error) } finally { @@ -247,23 +216,25 @@ const notifyGroupMembersOfNewPost = async (postId, groupId, context) => { const reason = 'post_in_group' const cypher = ` MATCH (post:Post { id: $postId })<-[:WROTE]-(author:User { id: $userId }) + OPTIONAL MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) MATCH (post)-[:IN]->(group:Group { id: $groupId })<-[membership:MEMBER_OF]-(user:User) WHERE NOT membership.role = 'pending' AND NOT (user)-[:MUTED]->(group) AND NOT (user)-[:MUTED]->(author) AND NOT (user)-[:BLOCKED]-(author) AND NOT user.id = $userId - WITH post, author, user + WITH post, author, user, emailAddress MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) - WITH notification, author, user, + WITH notification, author, user, emailAddress.email as email, post {.*, author: properties(author) } AS finalResource RETURN notification { .*, from: finalResource, to: properties(user), + email: email, relatedUser: properties(author) } ` @@ -278,8 +249,7 @@ const notifyGroupMembersOfNewPost = async (postId, groupId, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } catch (error) { throw new Error(error) } finally { @@ -295,12 +265,13 @@ const notifyOwnersOfGroup = async (groupId, userId, reason, context) => { WITH owner, group, user, membership MERGE (group)-[notification:NOTIFIED {reason: $reason}]->(owner) WITH group, owner, notification, user, membership + OPTIONAL MATCH (owner)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) SET notification.relatedUserId = $userId - WITH owner, group { __typename: 'Group', .*, myRole: membership.roleInGroup } AS finalGroup, user, notification - RETURN notification {.*, from: finalGroup, to: properties(owner), relatedUser: properties(user) } + WITH owner, emailAddress.email as email, group { __typename: 'Group', .*, myRole: membership.roleInGroup } AS finalGroup, user, notification + RETURN notification {.*, from: finalGroup, to: properties(owner), email: email, relatedUser: properties(user) } ` const session = context.driver.session() const writeTxResultPromise = session.writeTransaction(async (transaction) => { @@ -312,8 +283,7 @@ const notifyOwnersOfGroup = async (groupId, userId, reason, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } catch (error) { throw new Error(error) } finally { @@ -327,17 +297,18 @@ const notifyMemberOfGroup = async (groupId, userId, reason, context) => { MATCH (owner:User { id: $ownerId }) MATCH (user:User { id: $userId }) MATCH (group:Group { id: $groupId }) + OPTIONAL MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) OPTIONAL MATCH (user)-[membership:MEMBER_OF]->(group) - WITH user, group, owner, membership + WITH user, group, owner, membership, emailAddress MERGE (group)-[notification:NOTIFIED {reason: $reason}]->(user) - WITH group, user, notification, owner, membership + WITH group, user, notification, owner, membership, emailAddress SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) SET notification.relatedUserId = $ownerId WITH group { __typename: 'Group', .*, myRole: membership.roleInGroup } AS finalGroup, - notification, user, owner - RETURN notification {.*, from: finalGroup, to: properties(user), relatedUser: properties(owner) } + notification, user, emailAddress.email as email, owner + RETURN notification {.*, from: finalGroup, to: properties(user), email: email, relatedUser: properties(owner) } ` const session = context.driver.session() const writeTxResultPromise = session.writeTransaction(async (transaction) => { @@ -350,8 +321,7 @@ const notifyMemberOfGroup = async (groupId, userId, reason, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } catch (error) { throw new Error(error) } finally { @@ -371,11 +341,13 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { WHERE user.id in $idsOfUsers AND NOT (user)-[:BLOCKED]-(author) AND NOT (user)-[:MUTED]->(author) + OPTIONAL MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) OPTIONAL MATCH (post)-[:IN]->(group:Group) OPTIONAL MATCH (group)<-[membership:MEMBER_OF]-(user) - WITH post, author, user, group WHERE group IS NULL OR group.groupType = 'public' OR membership.role IN ['usual', 'admin', 'owner'] + WITH post, author, user, group, emailAddress + WHERE group IS NULL OR group.groupType = 'public' OR membership.role IN ['usual', 'admin', 'owner'] MERGE (post)-[notification:NOTIFIED {reason: $reason}]->(user) - WITH post AS resource, notification, user + WITH post AS resource, notification, user, emailAddress ` break } @@ -388,25 +360,27 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { AND NOT (user)-[:BLOCKED]-(postAuthor) AND NOT (user)-[:MUTED]->(commenter) AND NOT (user)-[:MUTED]->(postAuthor) + OPTIONAL MATCH (user)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) OPTIONAL MATCH (post)-[:IN]->(group:Group) OPTIONAL MATCH (group)<-[membership:MEMBER_OF]-(user) - WITH comment, user, group WHERE group IS NULL OR group.groupType = 'public' OR membership.role IN ['usual', 'admin', 'owner'] + WITH comment, user, group, emailAddress + WHERE group IS NULL OR group.groupType = 'public' OR membership.role IN ['usual', 'admin', 'owner'] MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(user) - WITH comment AS resource, notification, user + WITH comment AS resource, notification, user, emailAddress ` break } } mentionedCypher += ` - WITH notification, user, resource, + WITH notification, user, resource, emailAddress, [(resource)<-[:WROTE]-(author:User) | author {.*}] AS authors, [(resource)-[:COMMENTS]->(post:Post)<-[:WROTE]-(author:User) | post{.*, author: properties(author)} ] AS posts - WITH resource, user, notification, authors, posts, + WITH resource, user, emailAddress.email as email, notification, authors, posts, resource {.*, __typename: [l IN labels(resource) WHERE l IN ['Post', 'Comment', 'Group']][0], author: authors[0], post: posts[0]} AS finalResource SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) - RETURN notification {.*, from: finalResource, to: properties(user), relatedUser: properties(user) } + RETURN notification {.*, from: finalResource, to: properties(user), email: email, relatedUser: properties(user) } ` const session = context.driver.session() const writeTxResultPromise = session.writeTransaction(async (transaction) => { @@ -418,8 +392,7 @@ const notifyUsersOfMention = async (label, id, idsOfUsers, reason, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } catch (error) { throw new Error(error) } finally { @@ -437,18 +410,20 @@ const notifyUsersOfComment = async (label, commentId, reason, context) => { WHERE NOT (observingUser)-[:BLOCKED]-(commenter) AND NOT (observingUser)-[:MUTED]->(commenter) AND NOT observingUser.id = $userId - WITH observingUser, post, comment, commenter + OPTIONAL MATCH (observingUser)-[:PRIMARY_EMAIL]->(emailAddress:EmailAddress) + WITH observingUser, emailAddress, post, comment, commenter MATCH (postAuthor:User)-[:WROTE]->(post) MERGE (comment)-[notification:NOTIFIED {reason: $reason}]->(observingUser) SET notification.read = FALSE SET notification.createdAt = COALESCE(notification.createdAt, toString(datetime())) SET notification.updatedAt = toString(datetime()) - WITH notification, observingUser, post, commenter, postAuthor, + WITH notification, observingUser, emailAddress.email as email, post, commenter, postAuthor, comment {.*, __typename: labels(comment)[0], author: properties(commenter), post: post {.*, author: properties(postAuthor) } } AS finalResource RETURN notification { .*, from: finalResource, to: properties(observingUser), + email: email, relatedUser: properties(commenter) } `, @@ -461,8 +436,7 @@ const notifyUsersOfComment = async (label, commentId, reason, context) => { return notificationTransactionResponse.records.map((record) => record.get('notification')) }) try { - const notifications = await writeTxResultPromise - return notifications + return await writeTxResultPromise } finally { session.close() }