Skip to content

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

Merged

Conversation

eisbilir
Copy link
Member

@eisbilir eisbilir commented Aug 5, 2021

No description provided.

Copy link
Contributor

@maxceem maxceem left a 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:

  1. When using userUUID at least in one place, there is always an error https://monosnap.com/file/Sfz1mahb8V1HjGdRLFqactcVll6i2z

    Example 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.

  2. If cc: [] is empty array, got validation error https://monosnap.com/file/qE76o0DCzE87FIr4V2MW6aeN1kof5b

    Example 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" } } ] } }
  3. If some cc are provided, then resulting message to external.action.email has objects inside cc, while it suppose to be the list of emails only, same like for recepients.

    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"
     }
    }
  4. Can we please not adding new env var TC_API_V5_USERS_URL, and reuse existent one TC_API_V5_BASE_URL which suppose to be https://api.topcoder-dev.com/v5.

  5. Inside searchUsersByEmailQuery method, let's also try catch the const res = yield request to catch issues during network request like it's done in other methods.

@eisbilir
Copy link
Member Author

eisbilir commented Aug 5, 2021

Thanks, @maxceem for the feedbacks,

Inside searchUsersByEmailQuery method, let's also try catch the const res = yield request to catch issues during network request like it's done in other methods.

caller method has try catch
https://github.com/eisbilir/tc-notifications/blob/feature/universal-notifications/src/common/tcApiHelper.js#L146-L153

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

@maxceem
Copy link
Contributor

maxceem commented Aug 5, 2021

If above issue is okay.

@eisbilir got it, then the second try/catch is not required, please push the update.

Copy link
Contributor

@maxceem maxceem left a 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.

@maxceem maxceem merged commit 9b78ba7 into topcoder-platform:dev Aug 5, 2021
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.

2 participants