-
Notifications
You must be signed in to change notification settings - Fork 204
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
Do not store messages from extension if app is receiving events #2481
Conversation
5bb3c82
to
b1625ac
Compare
Generated by 🚫 Danger |
.../Mocks/StreamChat/WebSocketClient/EventMiddlewares/NotificationExtensionLifecycle_Mock.swift
Outdated
Show resolved
Hide resolved
Tests/StreamChatTests/APIClient/ChatPushNotificationContent_Tests.swift
Outdated
Show resolved
Hide resolved
Tests/StreamChatTests/Repositories/MessageRepository_Tests.swift
Outdated
Show resolved
Hide resolved
@@ -14,6 +15,52 @@ final class NSManagedObject_Tests: XCTestCase { | |||
func test_entityName_custom() { | |||
XCTAssertEqual(EntityWithCustomName.entityName, "CustomName") | |||
} | |||
|
|||
func test_discardChanges_shouldClearChangesForTheObject() throws { |
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.
Instead of ClearChanges consider using Discard to match the action you are performing and help the reader understand better
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.
Isn't it like trying to describe a word with the word itself? 😂 That's why a went for it, but happy to change it if is not that readable
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 not that it isn't readable just for consistency :). Up to you :)
Tests/StreamChatTests/Workers/Background/ConnectionRecoveryHandler_Tests.swift
Outdated
Show resolved
Hide resolved
Tests/StreamChatTests/Workers/Background/ConnectionRecoveryHandler_Tests.swift
Outdated
Show resolved
Hide resolved
.../Mocks/StreamChat/WebSocketClient/EventMiddlewares/NotificationExtensionLifecycle_Mock.swift
Show resolved
Hide resolved
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.
LGTM
82b719c
to
90d725a
Compare
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.
👍
📏 Size AnalysisTotal install size 10.6 MB | This change: ⬆️ +179.3 kB (+1.71%)🗂 See size breakdown
🔎 See the full size analysis (c53fbe1) merging into develop (2c2ef9e)
|
c53fbe1
to
c454f3b
Compare
Kudos, SonarCloud Quality Gate passed! |
* Do not store messages from extension if app is receiving events * Add tests for NotificationExtensionLifecycle * Update tests for ChatRemoteNotificationHandler * Add tests for NSManagedObjectContext.discard * Address PR comments * Do not set a default value for 'store' in MessageRepository.getMessage * Fix specs compilation issue
🔗 Issue Links
Resolves #2309
Resolves https://github.com/GetStream/ios-issues-tracking/issues/264
🎯 Goal
Make channel unread counts accurate
📝 Summary
Unread counts were not accurate when the app was in the foreground handling the updates both using WS events and Push Notifications
🛠 Implementation
This PR stops the notification extension from saving data to the DB if the host app is connected through WS, and processing those events.
This communication is done using a shared UserDefaults based on App Groups.
🧪 Manual Testing Notes
The issue was frequently visible when sending replies to a thread (sending those messages to the channel, as well)
☑️ Contributor Checklist
🎁 Meme