Skip to content
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

refactor: generate notification for new messages - WPB-11647 #2340

Open
wants to merge 17 commits into
base: refactor/nse-pull-pending-events
Choose a base branch
from

Conversation

jullianm
Copy link
Contributor

@jullianm jullianm commented Jan 3, 2025

WPB-11647

Key points

This PR is about generating notification content related to new messages events specifically populating the right data in the notification (title, body, sound, category..) given the content of a message (a new message text, a reply, sharing a picture, an audio file, a video etc).

It introduces notification components such as NotificationCategory, NotificationTitle, NotificationBody, NotificationSound and dedicated builders to help generate the notification content.

Testing

  • Notifications are correctly populated with the right content given different types of messages.

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

@echoes-hq echoes-hq bot added echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. echoes/initiative: ios-sync-refactoring labels Jan 3, 2025
@datadog-wireapp
Copy link

Datadog Report

Branch report: refactor/generate-notification-new-messages
Commit report: 6141c2c
Test service: wire-ios-mono

✅ 0 Failed, 1510 Passed, 2 Skipped, 3m 10.43s Total Time

Copy link
Contributor

github-actions bot commented Jan 3, 2025

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 8823f80.

♻️ This comment has been updated with latest results.

@jullianm jullianm requested review from netbe and johnxnguyen January 3, 2025 14:51
Copy link
Collaborator

@netbe netbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a couple of comments as a first review

@@ -98,10 +93,6 @@ extension Assembly {
self.pushChannel
}

Injector.register(UpdateEventDecryptorProtocol.self) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this remove from Assembly?

Comment on lines +371 to +373
userInfo["selfUserIDString"] = context.selfUserID
userInfo["senderIDString"] = context.senderID
userInfo["conversationIDString"] = context.conversationID.uuid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this going to be consumed to the main app? if so might be good to use constant for keys for safe access

Comment on lines +64 to +74
Injector.register(ConversationLocalStoreProtocol.self) {
self.conversationLocalStore
}

Injector.register(UserLocalStoreProtocol.self) {
self.userLocalStore
}

Injector.register(MessageLocalStoreProtocol.self) {
self.messageLocalStore
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we need to unregister the deps on teardown?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes/initiative: ios-sync-refactoring echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants