search

appirio-tech / connect-app

Build your next project on Connect with the power of crowdsourcing
https://connect.topcoder.com
44 stars 140 forks source link

Display invite user status #3048

Open maxceem opened 5 years ago

maxceem commented 5 years ago

From @vikasrohit:

I have another requirement (kind of urgent POC). We want some kind of confirmation or status for the users when they invite others. Right now they are not sure if the target user has received the email or not. Our systems works in following way: tc-project-service sends bus event for the particular user action (e.g. xxx.member.invited) and tc-notifications consumes that and generates another notification for tc-email-service to send email (if this is an action which requires immediate email) or makes an entry in ScheduledEvents table in tc-notifications. Now, for every bus event that tc-email-service receives, it saves it into a table Emails in email services's database with pending status and after success or failed attempt, it updates the status. My idea is to read data from ScheduledEvents table from tc-notifications and Emails table from tc-email-service and come up with status of a particular member invite that user has sent. fyi, there are not endpoints as of now for receiving the data from these two mentioned tables.

maxceem commented 5 years ago

Questions from @maxceem :

  1. Do you think we should add ednpoints to both these services and call them directly from Connect App to show the status?

  2. Just an idea, what if we add some empty pixel to the emails to track when users open the emails? We would have to create a new table in tc-email-service, gother with endpoint which would serve an 1 pixel image, and track when such image is loaded.

Answers from @vikasrohit :

  1. Yes
  2. That is something sendgrid is already doing for us. We might get data via sendgrid api. However, i am trying get an status which can give us confident that email is sent to sendgrid successfully.
maxceem commented 5 years ago

@vikasrohit here are few finding:

  1. Looks like tc-notfications service always sends invitation emails immediately and never schedule them no matter what user settings have for notifications.

    1. In this line, we set flag to send invitation emails immediately https://github.com/topcoder-platform/tc-notifications/blob/dev/connect/notificationServices/email.js#L316-L318
    2. And in this line, we check for a flag https://github.com/topcoder-platform/tc-notifications/blob/dev/connect/notificationServices/email.js#L350

    So we only have to add API to tc-email-service.

  2. In the tc-email-service.

    This service get events from the tc-notifcations which look like this for a user who is registered on Topcoder. It has userId and email for the invited user.

    This event is initially inserted to the DB with a pending status:

     INSERT INTO "Emails" ("id","topic_name","data","recipients","status","createdAt","updatedAt") VALUES (DEFAULT,'notifications.action.email.connect.project.notifications.generic','{"subject":"Skip unnecessary notifications for customers update","connectURL":"https://connect.topcoder-dev.com","projects":[{"id":8031,"name":"Skip unnecessary notifications for customers","sections":[{"title":"Team changes","PROJECT_TEAM":true,"notifications":[{"name":"F_NAME L_NAME","handle":"maxceem","date":"2019-05-20T05:06:13.353Z","projectName":"Skip unnecessary notifications for customers","projectId":8031,"authorHandle":"maxceem","authorFullName":"F_NAME L_NAME","photoURL":"https://topcoder-dev-media.s3.amazonaws.com/member/profile/maxceem-1541153638241.jpeg","type":"notifications.connect.project.member.invite.created","emailToAffectedUser":true,"notifications.connect.project.member.invite.created":true,"userId":40035291,"email":"maxceem@gmail.com","role":"customer","initiatorUserId":40152856,"toUserHandle":true,"timestamp":"2019-05-20T05:06:12.199Z","userHandle":"maxceem","userFullName":"F_NAME L_NAME","userEmail":"maxceem@gmail.com","initiatorUser":{"lastName":"Manag1","firstName":"Part12","photoURL":"https://topcoder-dev-media.s3.amazonaws.com/member/profile/pshah_manager-1553143102526.png","handle":"pshah_manager","userId":40152856,"email":"pshah_tcmanager@mailinator.com","status":"ACTIVE"}}]}]}]}','["maxceem@gmail.com"]','PENDING','2019-05-20 05:46:21.485 +00:00','2019-05-20 05:46:21.485 +00:00')

    Later, status could be updated to FAILED or SUCCESS.

    We can add an endpoint GET /email/status which would return the status of a particular email. To get the record we need, we have to know the next params:

    • eventType ("notifications.action.email.connect.project.notifications.generic") Then next is saved inside textual data column as a stringified JSON:
    • projectId (8031)
    • email and/or userId ("maxceem@gmail.com" / 40035291)

    The tricky moment here would be to restrict permissions. If want to restrict permission to only get status of the invitation for the users who are the member of the project, we would need to retrieve project data inside tc-email-service which would bind it to project service and is not that good. If want to get a quick POC, we can implement it without checking permission, as we would retrieve only the status of the email, it feels that security won't suffer if we don't check permissions here.

@vikasrohit let me know what you think about above.

maxceem commented 5 years ago

@vikasrohit just a humble reminder about this. Would be great to get some feedback from you, if it's ok to access status data from tc-email-service without project restriction or no.

vikasrohit commented 5 years ago

Thanks @maxceem for bumping it up.

  1. Agreed, we immediately send the member invite emails, however, in future, we may need to show the sent notification status for any generic event not only for member invite. Hence, I would like it to be generic and get the functionality, of having status of ScheduledEmails, in built in first iteration itself.

  2. I understand that it would be tricky to control the access without coupling the services. However, I think using email in the URL for getting status of the email for that particular email would be more security issue as anyone might use that endpoint to check status of any other user's email. Also, I am bit reluctant to send email in URL, though it would be sent over https. I have a solution for the plain email though, we can send MD5 of the target email and compare it with MD5 of the email in database as well, just like we do for standard password validation.

I have few suggestions for the permissions issue:

  1. Can we use the JSON query operators and functions to filter only project and event specific records from Emails table? I think it is using postgres which has set good JSON operators and functions.
  2. If yes, we can restrict access to the new endpoint for accessing the Emails records just like we do for tc-message-service i.e. by having referenceLookup table and using expression evaluator with it to determine if the requesting user has access to check notification status of other users.
  3. If no, I don't have any idea yet. 😬
maxceem commented 5 years ago

@vikasrohit I'm creating a spec to implement it in tc-email-service. As we would need to add new model, I'm thinking about the way to handle DB migrations in tc-email-service. So far as I see there is no migration functionality and as per code, DB is re-creted on the service run, see https://github.com/topcoder-platform/tc-email-service/blob/dev/connect/connectEmailServer.js#L42-L45

Is it how it works now? So no migration scripts are needed? Or should we introduce a way to run migrations script, like in tc-message-service, see https://github.com/appirio-tech/tc-message-service/tree/dev/migrations?

maxceem commented 5 years ago

@vikasrohit another question. How would you prefer to name the endpoint? I'm thinking to keep it as general and RESTful as possible. I propose this:

GET /email?reference=project&referenceId=1234&emailHash=QWOEIYR12Y4923749

with response which would be limited to status field only:

{
   "status": "success"
}

So in the future, we can add more fields to the response if we need.

maxceem commented 5 years ago

@vikasrohit question number 3.

I have a solution for the plain email though, we can send MD5 of the target email and compare it with MD5 of the email in database as well, just like we do for standard password validation.

You would like to send email as an md5, I guess we can do that tough the way feels little bit hacky and probably has not best performance.

We have an email in two places in DB and of these places are stringified TEXT/STRING, not JSON:

  data: { type: DataTypes.TEXT, allowNull: false },
  recipients: { type: DataTypes.STRING, allowNull: false }

And we keep scringified array and object inside:

So we cannot use JSON operators. What we can do though is to use regexp_matches command to extract the list of emails from recipients and after calculate each email md5 to compare with our search request. Not sure if such way would have a good perfomance if we have many records.

What do you think? Also, I'm not sure if it worth to md5 emails here, as we send emails without md5 for example when we log in.

vikasrohit commented 5 years ago

@maxceem for searching the information from the plain text fields data and recipients, I have some queries (I would ping you in Slack) which can help retrieving the required data relatively efficiently. I tried to find records for a specific project (from the data field, right now I have used first project form the projects array of the data json) and for specific email (from recipients array) and it turned out that it returned in 100-200ms time. I think we are good if we can fetch these details in this range for first release. And we can even add indexes on the specific path of the data text, however, I guess that would tightly couple the email service with the calling service. So, may be for next release (or even for the first release) we can try indexing the required data in ElasticSearch and query against it.

vikasrohit commented 5 years ago

@maxceem lets hold for this for some time until we wrap the major work around redesigning the navigation components in project and work management features and revisit it later. I would discuss it with product team meanwhile.