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

Implement a "sync this group" notification type #362

Open
nmalzieu opened this issue Jul 24, 2024 · 4 comments · May be fixed by #950
Open

Implement a "sync this group" notification type #362

nmalzieu opened this issue Jul 24, 2024 · 4 comments · May be fixed by #950
Assignees

Comments

@nmalzieu
Copy link
Collaborator

nmalzieu commented Jul 24, 2024

Some notifications for some commit messages (adding and removing users in a big group) would also be big enough to go over Apple's limit.

Let's catch it and just send a "sync group" notification which might help with forked state

@nmalzieu nmalzieu self-assigned this Jul 24, 2024
@nmalzieu nmalzieu removed their assignment Oct 1, 2024
@alexrisch
Copy link
Collaborator

@nmalzieu trying to clean up some of these older issues

I believe we already catch most of these from a push notification perspective (correct me if I'm wrong), but we are not doing syncing only notifications

My understanding of this is is for messages like group updates (member or metadata changes), we will want to sync the group

Is that understanding correct?

@nmalzieu
Copy link
Collaborator Author

nmalzieu commented Oct 2, 2024

@alexrisch yes sometimes the group change notification payload is bigger than APNS / FCM's payload max size and so it fails

We could probably

  • compute size of the payload before sending it rather than waiting for it to fail
  • when size of the payload is bigger than size limit, send a new kind of notification that just syncs the group

Maybe we can be smarter than that and also understand during the sync what the latest event was and still show a notification but more work for sure

@alexrisch alexrisch self-assigned this Oct 2, 2024
@alexrisch
Copy link
Collaborator

@nmalzieu thank you for the background info, I'll add additional details and add it to the backlog, assigning back to me for now

@alexrisch
Copy link
Collaborator

alexrisch commented Oct 4, 2024

Dev Direction:

iOS

Update ios/ConverseNotificationExtension/NotificationService.swift to handle new type from backend changes
Add new function to handle syncing the appropriate group for the account and silencing the push notification

Android

Update android/app/src/main/java/com/converse/dev/NotificationParser.kt to parse new type from backend changes
Add new function to handle syncing the appropriate group for the account and silencing the push notification

@alexrisch alexrisch linked a pull request Oct 8, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants