topcoder-platform / tc-notifications

4 stars 15 forks source link

Improve Universal Notifications Payload #215

Closed maxceem closed 3 years ago

maxceem commented 3 years ago

Background

At the moment to send email or web notification we require to userId and/or email address as per the Kafka Message payload format https://gist.github.com/maxceem/bb2ad68d2f8a790f01e3326f9f8015eb#file-notification-action-create-js-L9-L48

Though sometimes service which sends notification doesn't know userId/email and have just a handle for example. We don't want to ask each service to implement functionality to retrieve userId/email. Instead of this, we would allow passing only of the user identifier like userId, userUUID, email or handle and tc-notificatoin would have to get any details about the user necessary for sending notifications by itself.

Task

  1. For email the recipients array should allow passing the next object:

    { // only ONE of these fields is required (any of them can be passed)
      userId: 1212748193,
      userUUID: "12341234-1234-sdfsa-123fa-sadf",
      email: "email@test.com",
      handle: "maxceem"
    }
    • to send email tc-notification needs to know email so we have to get email if it's not passed by userId or userUUID or handle. If we could not get email - log error as we cannot send notification in such a case.
    • we also should getuserId which is needed to check user settings. So we have to use handle, email or userUUID to get userId. If user is not found, then still send notification to the email, this would be needed in case if user is not registered yet. But if there is some other error while getting userId (not NOT FOUND), then don't send email and log error as we failed to get userId.
    • The same logic should be supported for cc of the email. Thoug as we don't need to check setting for the user, then userId for cc is not need to retrieve.
  2. For web notification:

    • replace userId field with recipients wich would also be an array. And we would should send this web notification to each member in the recipients array.
      • recipients array should allow the same objects as for email:
        {  // only ONE of these fields is required (any of them can be passed)
        userId: 1212748193,
        userUUID: "12341234-1234-sdfsa-123fa-sadf",
        email: "email@test.com",
        handle: "maxceem"
        }
      • to send web notification we need userId, so we have to get it by userUUID or email of handle depend on what is provided.
      • if we could not get userId for any reason - log error as we cannot create web notification without userId.

API Endpoints

We have 2 kind of user ids in Topcoder which are provided by different services, though we keeping user in sync between them.

For example, TC-Notifications services use only userId, while TaaS API always uses userUUID.

Get userId

  1. Get userId by userUUID - https://github.com/topcoder-platform/taas-apis/blob/feature/notifications-scheduler/src/common/helper.js#L1065-L1095 ( enrich can be false )
  2. Get userId by email - https://github.com/topcoder-platform/tc-project-service/blob/feature/new-milestone-concept/src/util.js#L909-L947
  3. Get userId by handle - https://github.com/topcoder-platform/tc-project-service/blob/feature/new-milestone-concept/src/util.js#L584-L608

Get email

  1. Get email by userUUID - https://github.com/topcoder-platform/taas-apis/blob/feature/notifications-scheduler/src/common/helper.js#L1065-L1095 ( enrich must be true to return emails )
  2. Get email by userId - https://github.com/topcoder-platform/tc-project-service/blob/feature/new-milestone-concept/src/util.js#L559-L579
  3. Get email by handle - https://github.com/topcoder-platform/tc-project-service/blob/feature/new-milestone-concept/src/util.js#L584-L608

API Notes

maxceem commented 3 years ago

@eisbilir one more small change in the kafka message payload format. It appears that Bus API doesn't allow sending Kafka Events with payload: [] which is an array, it only allows the payload to be an object.

Can we please update supported payload from payload: [] to payload: { notifications: [] }. I've already updated Event Example here https://gist.github.com/maxceem/bb2ad68d2f8a790f01e3326f9f8015eb

eisbilir commented 3 years ago

@maxceem https://api.topcoder-dev.com/v5/users/{uuid}?enrich=true is not returning email address. I tried with m2mToken, m2mTokenForUbahn, admin token and searched several uuids including yours. I made all necessary updates, but it won't work when we are not able to get emails by UUID When the only provided field is UUID, should I get the handle by UUID after that, get email by handle?

eisbilir commented 3 years ago

PR https://github.com/topcoder-platform/tc-notifications/pull/216 I tried the workaround UUID => userId => email

maxceem commented 3 years ago

@eisbilir your solution UUID => userId => email works great, and I don't see a better way.

During testing of the PR it failed, but it only fails for some users who most likely don't have V3 user profile because of data inconsistency on DEV.

Thanks!

maxceem commented 3 years ago

This was deployed to DEV and tested locally, closing.

Payment processed by https://www.topcoder.com/challenges/7e694e52-5d85-4c63-845b-b561c7d661ec