Skip to content

Commit 3b831be

Browse files
github-actions[bot]David-Henner
github-actions[bot]
and
David-Henner
authored
fix: incorrect "X added you" (to conversation) notifications - WPB-22820 🍒 (#4217)
Co-authored-by: David-Henner <david.henner@wire.com>
1 parent 91c925a commit 3b831be

File tree

3 files changed

+114
-18
lines changed

3 files changed

+114
-18
lines changed

‎WireDomain/Sources/WireDomain/Notifications/Builders/Conversation/ConversationMemberJoinEventNotificationBuilder.swift‎

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
//
1818

1919
import WireDataModel
20+
import WireLogging
2021
import WireNetwork
2122

2223
struct ConversationMemberJoinEventNotificationBuilder: ConversationMemberJoinEventNotificationBuilderProtocol {
@@ -30,7 +31,8 @@ struct ConversationMemberJoinEventNotificationBuilder: ConversationMemberJoinEve
3031
let addedUserIDs = Set(event.members.compactMap(\.id))
3132

3233
let canBuildNotification = await validator.validate(
33-
addedUserIDs: addedUserIDs
34+
addedUserIDs: addedUserIDs,
35+
conversationID: event.conversationID
3436
)
3537

3638
guard canBuildNotification else {
@@ -163,15 +165,69 @@ struct ConversationMemberJoinEventNotificationBuilder: ConversationMemberJoinEve
163165
extension ConversationMemberJoinEventNotificationBuilder {
164166
struct Validator {
165167
let userLocalStore: any UserLocalStoreProtocol
168+
let conversationLocalStore: any ConversationLocalStoreProtocol
166169

167170
func validate(
168-
addedUserIDs: Set<UUID>
171+
addedUserIDs: Set<UUID>,
172+
conversationID: ConversationID
169173
) async -> Bool {
170174

175+
var logAttributes: LogAttributes = [
176+
LogAttributesKey.conversationId: conversationID.toDomainModel().safeForLoggingDescription
177+
]
178+
171179
let selfUser = await userLocalStore.fetchSelfUser()
172180
let selfUserID = await userLocalStore.id(for: selfUser)
173181

174-
return addedUserIDs.contains(selfUserID)
182+
// Check if self user is in the added members list
183+
guard addedUserIDs.contains(selfUserID) else {
184+
logSkippingNotification(
185+
reason: "user isn't in the added members list",
186+
attributes: logAttributes
187+
)
188+
return false
189+
}
190+
191+
// Check if the conversation exists
192+
guard let conversation = await conversationLocalStore.fetchConversation(
193+
id: conversationID.id,
194+
domain: conversationID.domain
195+
) else {
196+
logSkippingNotification(
197+
reason: "conversation doesn't exist",
198+
attributes: logAttributes
199+
)
200+
return false
201+
}
202+
203+
// Check if self user is already a participant
204+
let participants = await conversationLocalStore.localParticipants(in: conversation)
205+
206+
guard !participants.contains(selfUser) else {
207+
logSkippingNotification(
208+
reason: "self user is already a participant",
209+
attributes: logAttributes
210+
)
211+
return false
212+
}
213+
214+
WireLogger.notifications.info(
215+
"Creating memberJoin notification: self user is being added",
216+
attributes: logAttributes,
217+
.safePublic
218+
)
219+
return true
220+
}
221+
222+
private func logSkippingNotification(
223+
reason: String,
224+
attributes: LogAttributes
225+
) {
226+
WireLogger.notifications.info(
227+
"Skipping memberJoin notification: \(reason)",
228+
attributes: attributes,
229+
.safePublic
230+
)
175231
}
176232
}
177233

‎WireDomain/Sources/WireDomain/Notifications/Components/NSEClientScope.swift‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -545,7 +545,8 @@ final class NSEClientScope: Component<NSEClientScopeDependency> {
545545
)
546546

547547
let validator = ConversationMemberJoinEventNotificationBuilder.Validator(
548-
userLocalStore: userLocalStore
548+
userLocalStore: userLocalStore,
549+
conversationLocalStore: conversationLocalStore
549550
)
550551

551552
return ConversationMemberJoinEventNotificationBuilder(

‎WireDomain/Tests/WireDomainTests/Notifications/Conversations/ConversationMemberJoinEventNotificationBuilderTests.swift‎

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,17 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
6363
let isGroup = true
6464
let isTeam = true
6565

66-
await setupMock(isGroup: isGroup, isTeam: isTeam)
66+
await setupMock(isGroup: isGroup, isTeam: isTeam, participants: [])
6767

6868
sut = ConversationMemberJoinEventNotificationBuilder(
6969
context: .init(
7070
conversationLocalStore: conversationLocalStore,
7171
userLocalStore: userLocalStore
7272
),
73-
validator: .init(userLocalStore: userLocalStore)
73+
validator: .init(
74+
userLocalStore: userLocalStore,
75+
conversationLocalStore: conversationLocalStore
76+
)
7477
)
7578

7679
// When
@@ -91,14 +94,17 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
9194
let isGroup = true
9295
let isTeam = false
9396

94-
await setupMock(isGroup: isGroup, isTeam: isTeam)
97+
await setupMock(isGroup: isGroup, isTeam: isTeam, participants: [])
9598

9699
sut = ConversationMemberJoinEventNotificationBuilder(
97100
context: .init(
98101
conversationLocalStore: conversationLocalStore,
99102
userLocalStore: userLocalStore
100103
),
101-
validator: .init(userLocalStore: userLocalStore)
104+
validator: .init(
105+
userLocalStore: userLocalStore,
106+
conversationLocalStore: conversationLocalStore
107+
)
102108
)
103109

104110
// When
@@ -119,14 +125,17 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
119125
let isGroup = false
120126
let isTeam = true
121127

122-
await setupMock(isGroup: isGroup, isTeam: isTeam)
128+
await setupMock(isGroup: isGroup, isTeam: isTeam, participants: [])
123129

124130
sut = ConversationMemberJoinEventNotificationBuilder(
125131
context: .init(
126132
conversationLocalStore: conversationLocalStore,
127133
userLocalStore: userLocalStore
128134
),
129-
validator: .init(userLocalStore: userLocalStore)
135+
validator: .init(
136+
userLocalStore: userLocalStore,
137+
conversationLocalStore: conversationLocalStore
138+
)
130139
)
131140

132141
// When
@@ -142,20 +151,22 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
142151
}
143152

144153
func testGenerateConversationMemberJoinEventNotification_It_Should_Not_Build_Notification() async throws {
145-
146154
// Mock
147155

148156
let isGroup = false
149157
let isTeam = true
150158

151-
await setupMock(isGroup: isGroup, isTeam: isTeam)
159+
await setupMock(isGroup: isGroup, isTeam: isTeam, participants: [])
152160

153161
sut = ConversationMemberJoinEventNotificationBuilder(
154162
context: .init(
155163
conversationLocalStore: conversationLocalStore,
156164
userLocalStore: userLocalStore
157165
),
158-
validator: .init(userLocalStore: userLocalStore)
166+
validator: .init(
167+
userLocalStore: userLocalStore,
168+
conversationLocalStore: conversationLocalStore
169+
)
159170
)
160171

161172
// When
@@ -165,6 +176,33 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
165176
XCTAssertNil(userNotification)
166177
}
167178

179+
func test_ItDoesNotCreateNotification_WhenSelfUserIsAlreadyParticipant() async throws {
180+
// Given
181+
let selfUser = await context.perform { [modelHelper, context] in
182+
modelHelper.createSelfUser(id: Scaffolding.selfUserID, in: context)
183+
}
184+
185+
// Mock
186+
await setupMock(isGroup: true, isTeam: false, participants: [selfUser])
187+
188+
sut = ConversationMemberJoinEventNotificationBuilder(
189+
context: .init(
190+
conversationLocalStore: conversationLocalStore,
191+
userLocalStore: userLocalStore
192+
),
193+
validator: .init(
194+
userLocalStore: userLocalStore,
195+
conversationLocalStore: conversationLocalStore
196+
)
197+
)
198+
199+
// When
200+
let userNotification = await sut.buildContent(event: Scaffolding.selfUserAddedEvent)
201+
202+
// Then: should NOT create notification
203+
XCTAssertNil(userNotification, "Should not create notification when self user is already a participant")
204+
}
205+
168206
private func internalTest_assertNotificationContent(
169207
_ userNotification: UserNotification,
170208
isGroup: Bool,
@@ -219,20 +257,22 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
219257
XCTAssertEqual(notificationContent.userInfo["conversationIDString"] as! String, UUID.mockID2.uuidString)
220258
}
221259

222-
private func setupMock(isGroup: Bool, isTeam: Bool) async {
223-
let conversation = await context.perform { [self] in
260+
private func setupMock(isGroup: Bool, isTeam: Bool, participants: Set<ZMUser>) async {
261+
let conversation = await context.perform { [modelHelper, context] in
224262
modelHelper.createGroupConversation(in: context)
225263
}
226264
conversationLocalStore.fetchOrCreateConversationIdDomain_MockValue = conversation
265+
conversationLocalStore.fetchConversationIdDomain_MockValue = conversation
266+
conversationLocalStore.localParticipantsIn_MockValue = participants
227267
conversationLocalStore.conversationMutedMessageTypesIncludingAvailability_MockValue = .some(.none)
228268
conversationLocalStore.lastReadServerTimestamp_MockValue = .now
229-
userLocalStore.fetchOrCreateUserIdDomain_MockValue = await context.perform { [self] in
269+
userLocalStore.fetchOrCreateUserIdDomain_MockValue = await context.perform { [modelHelper, context] in
230270
modelHelper.createUser(in: context)
231271
}
232272
userLocalStore.nameFor_MockValue = Scaffolding.senderName
233273
conversationLocalStore.nameFor_MockValue = Scaffolding.conversationName
234274
conversationLocalStore.isGroupConversation_MockValue = isGroup
235-
userLocalStore.fetchSelfUser_MockValue = await context.perform { [self] in
275+
userLocalStore.fetchSelfUser_MockValue = await context.perform { [modelHelper, context] in
236276
modelHelper.createSelfUser(id: Scaffolding.selfUserID, in: context)
237277
}
238278
conversationLocalStore.isConversationForcedReadOnly_MockValue = false
@@ -295,5 +335,4 @@ final class ConversationMemberJoinEventNotificationBuilderTests: XCTestCase {
295335
mutedReference: nil
296336
)
297337
}
298-
299338
}

0 commit comments

Comments
 (0)