Skip to content

feat(admin): Send notification by topic#303

Merged
brh28 merged 2 commits intomainfrom
feat/send-notification-to-topic
Mar 24, 2026
Merged

feat(admin): Send notification by topic#303
brh28 merged 2 commits intomainfrom
feat/send-notification-to-topic

Conversation

@brh28
Copy link
Contributor

@brh28 brh28 commented Mar 20, 2026

  • Introduces two admin api endpoints: query notificationTopics, mutation sendNotification
  • Notification types defined in yaml

Notification types defined in yaml
@islandbitcoin islandbitcoin self-requested a review March 20, 2026 17:04
@islandbitcoin islandbitcoin self-requested a review March 20, 2026 20:28
Copy link
Contributor

@islandbitcoin islandbitcoin left a comment

Choose a reason for hiding this comment

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

LGTM

@heyolaniran
Copy link

Hello @brh28 , thanks for the pull request. I think the sendNotification method, which delegates the action to the PushNotificationService, is a good approach for sending bulk notifications about a specific topic. However, it does not currently replace the specific administrative notification system mentioned in ticket #297 . Correct me if I’m misunderstanding the process, but I didn’t see anywhere how you retrieve a specific user( of course that’s not the purpose of topic-based push notifications), so I’d say your sendNotification mutation and the administrative push notification should coexist in the codebase.

@brh28
Copy link
Contributor Author

brh28 commented Mar 23, 2026

so I’d say your sendNotification mutation and the administrative push notification should coexist in the codebase.

Yes, I agree. I think there should be two functions in the notification service: sendToTopic and sendToUser. My PR is calling the PushNotificationService directly because there was no need for the extra layer of abstraction, but can add it if needed/desired.

Yes, sendToUser first needs to get the device tokens as done in the admin send function. Then, this function should just be able to call the existing sendToDevice (probably should be renamed to devices since it accepts an array of device token)

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 this pull request may close these issues.

3 participants