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 conversation deleted - WPB-11658 #2381

Open
wants to merge 1 commit into
base: refactor/generate-notification-conversation-member-leave
Choose a base branch
from

Conversation

jullianm
Copy link
Contributor

@jullianm jullianm commented Jan 13, 2025

TaskWPB-11658 [iOS] Generate UNNotificationContent for deleted conversations

Key points

This PR is about generating notification content related to the conversation deleted event populating the right data in the notification (title, body, sound, category..).

Testing

  • Notification for deleted conversation is correctly populated

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 13, 2025
@jullianm jullianm requested a review from johnxnguyen January 13, 2025 10:52
@jullianm jullianm requested a review from netbe January 13, 2025 10:52
Copy link
Contributor

Test Results

    3 files      4 suites   4m 19s ⏱️
3 338 tests 3 310 ✅ 28 💤 0 ❌
3 348 runs  3 320 ✅ 28 💤 0 ❌

Results for commit 8ac66ab.

@datadog-wireapp
Copy link

Datadog Report

Branch report: refactor/generate-notification-conversation-deleted
Commit report: 9eabb77
Test service: wire-ios-mono

✅ 0 Failed, 3199 Passed, 28 Skipped, 2m 14.72s Total Time

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.

LGTM;)

.deletedGroup(senderName: context.senderName)
)

content.body = body.make()
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: one of the things that I don't like in our legacy code is how the actual values for the notification content separated from the content object because it usually means that there is some work involved in understanding exactly how a notification is configured.

I know that in some cases there is some complexity in generating content such as the body, and that it should also be reused among various notifications types, but I believe that mostly applies to new message notifications. In this case, I think it would be better to have the actual values closer to the content, something like:

content.body = "A conversation was deleted"
content.categoryIdentifier = .nonActionable
content.sound = .click

What do you think?

@echoes-hq echoes-hq bot removed the echoes: technical-roadmap Work contributing to the Technical Roadmap, to improve our velocity or reduce the technical debt. label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants