-
Notifications
You must be signed in to change notification settings - Fork 25
fix: incorrect "X added you" (to conversation) notifications - WPB-22820 #4169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/cycle-4.14
Are you sure you want to change the base?
fix: incorrect "X added you" (to conversation) notifications - WPB-22820 #4169
Conversation
21ddfbb to
46c81e5
Compare
Test Results 5 files 644 suites 4m 33s ⏱️ Results for commit de5f287. ♻️ This comment has been updated with latest results. |
samwyndham
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
| } | ||
|
|
||
| // Check if self user is already a participant | ||
| guard !conversation.localParticipants.contains(selfUser) else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Could there be a situation where self user is added to this conversation just before this notification code is triggered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's difficult to say with certainty because showing notifications is handled by the NSE while processing the event and applying its effects (adding the user) is performed by the main app. I tested it and it works, i.e: I get the notification when I'm supposed to. But there could perhaps be a case where the order of execution between the main app and the NSE cause the user to be added before the notification is created?
...Notifications/Push Notifications/Notification Types/Content/ZMLocalNotification+Events.swift
Outdated
Show resolved
Hide resolved
| override func shouldCreateNotification() -> Bool { | ||
| // we don't want to create notifications when other people join or leave conversation | ||
| let concernsSelfUser = event.userIDs.contains(ZMUser.selfUser(in: moc).remoteIdentifier) | ||
| let selfUser = ZMUser.selfUser(in: moc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: is this code still live? Maybe only when the app is open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's unused & the code is already removed on develop. I removed the changes and left only the ones being used by the new sync system
This reverts commit a99c805.
… github.com:wireapp/wire-ios into fix/incorrect-added-to-group-notification-WPB-22820
…otification-WPB-22820
Issue
Some users are experiencing push notifications being presented, indicating they've been added to a conversation, when they're already members of that conversation. They reported seeing these notifications more than once for the same conversation.
Cause
Clients receive and process
member-joinevents with their self user being included in the payload as one of the added members, even though they're already members of the conversation. This causes notifications to be shown incorrectly.Solution
The cause for these
member-joinis unclear, but we can prevent them from triggering notifications by verifying whether the user is already a member of the conversation.Testing
No clear steps to reproduce. We'll rely on unit tests to verify that notifications aren't created incorrectly.
We can still manual verify that notifications are being shown when needed, by adding a user to a conversation and verifying that their client receives a notification.
Checklist
[WPB-XXX].