-
Notifications
You must be signed in to change notification settings - Fork 15
update: improve universal notifications payload #216
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
update: improve universal notifications payload #216
Conversation
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.
Thanks, @eisbilir, I've noticed a few issues:
-
When using
userUUIDat least in one place, there is always an error https://monosnap.com/file/Sfz1mahb8V1HjGdRLFqactcVll6i2zExample message:
{ "topic": "notification.action.create", "originator": "tc-direct", "timestamp": "2018-02-16T00:00:00", "mime-type": "application/json", "payload": { "notifications": [ { "serviceId": "email", "type": "taas.notification.request-submitted", "details": { "from": "[email protected]", "recipients": [ { "userId": 40035291 }, { "email": "[email protected]" }, { "userUUID": "6910d2f4-a50a-4494-8f46-6de1f3d032c2" }, { "handle": "nkumar1" } ], "cc": [{ "email": "[email protected]" }], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
Btw, when getting something by
userUUIDwe most likely we would need to create a separate M2M token specially for u-bahn like in TaaS API https://github.com/topcoder-platform/taas-apis/blob/feature/notifications-scheduler/src/common/helper.js#L889-L894. Not sure if this is the reason of this issue or it would work without a special M2M token for this particular endpoint. -
If
cc: []is empty array, got validation error https://monosnap.com/file/qE76o0DCzE87FIr4V2MW6aeN1kof5bExample message:
{ "topic": "notification.action.create", "originator": "tc-direct", "timestamp": "2018-02-16T00:00:00", "mime-type": "application/json", "payload": { "notifications": [ { "serviceId": "email", "type": "taas.notification.request-submitted", "details": { "from": "[email protected]", "recipients": [ { "userId": 40035291 }, { "email": "[email protected]" }, { "handle": "nkumar1" } ], "cc": [], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
-
If some
ccare provided, then resulting message toexternal.action.emailhas objects insidecc, while it suppose to be the list of emails only, same like forrecepients.Example input messages:
{ "topic": "notification.action.create", "originator": "tc-direct", "timestamp": "2018-02-16T00:00:00", "mime-type": "application/json", "payload": { "notifications": [ { "serviceId": "email", "type": "taas.notification.request-submitted", "details": { "from": "[email protected]", "recipients": [ { "userId": 40035291 }, { "email": "[email protected]" }, { "handle": "nkumar1" } ], "cc": [{ "handle": "maxceem" }], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
The output message
cchas incorrect format:{ "topic": "external.action.email", "originator": "tc-notifications", "timestamp": "2021-08-05T10:10:43.231Z", "mime-type": "application/json", "payload": { "from": "[email protected]", "recipients": ["[email protected]"], // this is correct - only emails "cc": [ { "handle": "maxceem", "email": "[email protected]", "userId": 40035291 } // this an object, but should be email string only ], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgrid_template_id": "...", "version": "v3" } }
-
Can we please not adding new env var
TC_API_V5_USERS_URL, and reuse existent oneTC_API_V5_BASE_URLwhich suppose to behttps://api.topcoder-dev.com/v5. -
Inside
searchUsersByEmailQuerymethod, let's alsotry catchtheconst res = yield requestto catch issues during network request like it's done in other methods.
|
Thanks, @maxceem for the feedbacks,
caller method has try catch I've fixed the other issues and ready to push, If above issue is okay. while searching by userIds and handles, I combined them into one query and make the search call at once. But for emails and uuids, it doesn't allow to make multiple searches with one call. And we have to make 2 seperate calls for each person while trying to find email by uuid |
@eisbilir got it, then the second try/catch is not required, please push the update. |
maxceem
left a comment
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.
@eisbilir now I've got why you created a separate ENV var for the User API 😁
Works good, so I'm mering this PR.
It looks like we never get email by userUUID I guess we would have to find another endpoint for this, though we would do it separately as it's not urgent now.
No description provided.