-
Notifications
You must be signed in to change notification settings - Fork 15
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 user contact join - WPB-11660 #2383
base: refactor/generate-notification-conversation-member-leave
Are you sure you want to change the base?
refactor: generate notification user contact join - WPB-11660 #2383
Conversation
…nnection builders, add UTs
Test Results 3 files 4 suites 5m 12s ⏱️ Results for commit 0eb4f18. |
Datadog ReportBranch report: ✅ 0 Failed, 3200 Passed, 28 Skipped, 3m 10.71s Total Time |
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;)
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.
I didn't do a full review, since I think the question about whether this notification needs to be supported should be clarified first.
@@ -19,7 +19,7 @@ | |||
import WireAPI | |||
import WireDataModel | |||
|
|||
struct NewSystemMessageNotificationBuilder: NotificationBuilder { | |||
struct ConversationSystemMessageNotificationBuilder: NotificationBuilder { |
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.
I'm confused by the naming, what is the relevance of "system message"?
@@ -19,7 +19,7 @@ | |||
import WireAPI | |||
import WireDataModel | |||
|
|||
struct NewUserMessageNotificationBuilder: NotificationBuilder { | |||
struct ConversationUserMessageNotificationBuilder: NotificationBuilder { |
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.
I guess this was renamed from another PR, but it still confuses me slightly. I think the structure of the builders could follow the structure of events, so ConversationEventNotificationBuilder
builds notifications for all conversation events. Internally, there could be separate builders for each events, i.e NewMessageNotificationBuilder
or ConversationMemberJoinNotificationBuilder
, et.c
struct UserConnectionNotificationBuilder: NotificationBuilder { | ||
|
||
enum ConnectionStatus { | ||
case joined |
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.
I guess this is for the case where someone in your address book creates an account on Wire. But I don't think we get this event anymore, in which case I don't think we need to support this notification. Best to check with BE.
Key points
This PR is about generating notification content related to the user join event (connections) populating the right data in the notification (title, body, sound, category..).
Testing
Checklist
[WPB-XXX]
.