Conversation
9df24f1 to
1680dc7
Compare
brh28
left a comment
There was a problem hiding this comment.
Looks good. When we jump on the call this morning, I want to look into that sendAdminPushNotification function that's being used, since Dread mentioned we should start moving away from the Galoy notification code
1680dc7 to
68ace26
Compare
brh28
left a comment
There was a problem hiding this comment.
Note, if you make a change to the graphql schema, you should run make codegen to generate the updated schema
|
|
||
| const { accountId, title, body, amount, currency, notificationCategory } = args.input; | ||
|
|
||
| const success = await Admin.sendAdminPushNotification({ |
There was a problem hiding this comment.
My PR is removing this function since we're changing the notification system. Unless you think it better to use this function, I'm thinking we should write sendNotificationToUser which queries the user's deviceTokens and then sends to each (which should already exist)
There was a problem hiding this comment.
@brh28 i think it is revelant part of the process because it is in that method we check the account Id and fetch the userId and device token. We can keep the same logic and just refactor the Notification domain from Galoy to our own
There was a problem hiding this comment.
Yes, we'll definitely need to query the deviceTokens as you're describing, though I'm curious why we might want to refactor the Notification domain. Afaik, we're not using any of these categories or this code
There was a problem hiding this comment.
The GaloyNotificationCategories are defined in the notification domain layer. So if we want to have custom notification category we have to settle them in the notification layers.
I’m thinking the notification category is more revelant in the logs to identify various type of notifications we’ve sent to the user.
Actually I think we have 3-4 categories of notifications settled by Galoy, the default one is the balance notification. The cashout Notification category is currently missing and if we have to add it I think we have to do it in the notification domain layer.
In this commit i've added : - Graph QL mutation that resolves the cashout (payment entry) notification from Admin action - At App Layer, this commit added the send cashout notification for cashout settled on Galoy Payments notification type and delegate the notification action to admin push notification filtered send from the notification service - Unit test for cashout notification in test flash unit directory. all unit tests passed End to End test notification push made and passed - GraphQL schema updated
68ace26 to
89bc2f9
Compare
feature : Cashout Notifiction from Admin
In this PR i've added :
🧪 Testing Coverage
📚 Documentation Updates
🔄 Future Phases
This PR delivers Phase X of Y.
Next:
Branch:
Reviewer Notes:
Deployment Instructions: