-
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
userUUID
at 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": "example@example.com", "recipients": [ { "userId": 40035291 }, { "email": "gunasekar.kesavalu@topcoder.com" }, { "userUUID": "6910d2f4-a50a-4494-8f46-6de1f3d032c2" }, { "handle": "nkumar1" } ], "cc": [{ "email": "maxceem@gmail.com" }], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
Btw, when getting something by
userUUID
we 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": "example@example.com", "recipients": [ { "userId": 40035291 }, { "email": "gunasekar.kesavalu@topcoder.com" }, { "handle": "nkumar1" } ], "cc": [], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
-
If some
cc
are provided, then resulting message toexternal.action.email
has 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": "example@example.com", "recipients": [ { "userId": 40035291 }, { "email": "gunasekar.kesavalu@topcoder.com" }, { "handle": "nkumar1" } ], "cc": [{ "handle": "maxceem" }], "data": { "subject": "...", "body": "...", "field1": "...", "field2": "...", "filedN": "..." }, "sendgridTemplateId": "...", "version": "v3" } } ] } }
The output message
cc
has incorrect format:{ "topic": "external.action.email", "originator": "tc-notifications", "timestamp": "2021-08-05T10:10:43.231Z", "mime-type": "application/json", "payload": { "from": "example@example.com", "recipients": ["nkumar.cm+21@gmail.com"], // this is correct - only emails "cc": [ { "handle": "maxceem", "email": "maxceem@gmail.com", "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_URL
which suppose to behttps://api.topcoder-dev.com/v5
. -
Inside
searchUsersByEmailQuery
method, let's alsotry catch
theconst res = yield request
to 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. |
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.