Skip to content

Feature/interview nylas #571

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Oct 19, 2021
Merged

Feature/interview nylas #571

merged 12 commits into from
Oct 19, 2021

Conversation

painterner
Copy link
Collaborator

Final fix PR for challenge:
Topcoder TaaS Interview Scheduler - Notifications

@painterner
Copy link
Collaborator Author

painterner commented Oct 17, 2021 •
edited
Loading

The last 5 commit is about final fixes, please review.
Reference:

  1. Review scorecard
  2. https://docs.google.com/document/d/1x2K-C5OsWGYjvNU4Uvua-TH7pVZLd7qy5Jog73Esv2w/edit

maxceem
maxceem suggested changes Oct 18, 2021
View reviewed changes
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.

Thank you @painterner.

Most of the fixes works great for me.

The only issue I'm still getting, is that App is still crashing sometimes, see screenshots:
https://monosnap.com/file/IDoXLfeD2YXkTKfX1h7LklmgP80OpA
https://monosnap.com/file/hOaENsQ66p8dhEolijg1oYncNge36Q

Steps to reproduce:

  • use my .env file provided by email
  • run npm run local:init
  • run npm run dev
  • run in separate window node scripts/demo-email-notifications/index.js
  • wait around 5 minutes

src/services/NotificationsSchedulerService.js Outdated
const user = await getUserWithId(jobCandidate.userId)
if (!user) { return null }

const interviewLink = `${config.TAAS_APP_URL}/${job.projectId}/positions/${job.id}/candidates/interviews`
const guestName = _.isEmpty(interview.guestNames) ? '' : interview.guestNames[0]
// const guestName = _.isEmpty(interview.guestNames) ? '' : interview.guestNames[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove commented line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@painterner
Copy link
Collaborator Author

painterner commented Oct 19, 2021 •
edited
Loading

Hi, I have created three new commits for app crashing + three additional tasks.
For app crashing tests:
Actually I only get warning message of UnhandledPromiseRejectionWarning, It was fired from sendInterviewComingUpNotifications function,
However I have added all error catcher for schedulers like this now, is it okay ?

[2021-10-19T08:22:08.403Z] NotificationsSchedulerService sendInterviewComingUpNotifications ERROR : cannot GET /v5/users/dfc118c9-cdc6-4716-a8c5-763b5ff2383c?enrich=true (500)
(node:653496) UnhandledPromiseRejectionWarning: Error: Internal Server Error
    at Request.callback (/home/ka/all/work/toc/work103-taas-scheduler/taas-apis/node_modules/superagent/lib/node/index.js:883:15)
    at /home/ka/all/work/toc/work103-taas-scheduler/taas-apis/node_modules/superagent/lib/node/index.js:1127:20
    at IncomingMessage.<anonymous> (/home/ka/all/work/toc/work103-taas-scheduler/taas-apis/node_modules/superagent/lib/node/parsers/json.js:22:7)
    at IncomingMessage.emit (events.js:387:35)
    at IncomingMessage.emit (domain.js:470:12)
    at endReadableNT (internal/streams/readable.js:1317:12)
    at processTicksAndRejections (internal/process/task_queues.js:82:21)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:653496) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:653496) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

I am a little curious why I only got promise warning but you got app crashing, is my warning the case you got error ?
As I have added error catcher for these scheduler, you can test again if app will crash, sorry for this inconvenience.

@maxceem
Copy link
Contributor

maxceem commented Oct 19, 2021

Hi

Thank you for 3 additional tasks. Regarding the 1st task.

The idea is that we don't have to write code inside src/services/InterviewService.js. In that file we fire event here. And this event triggers event handler - method processRequest. So what we need to do is to call a method to send notification inside processRequest method like it was done before https://github.com/topcoder-platform/taas-apis/blob/feature/interview-nylas/src/eventHandlers/InterviewEventHandler.js#L169. The only difference that we would use update method to send notification.

image

I am a little curious why I only got promise warning but you got app crashing, is my warning the case you got error ?
As I have added error catcher for these scheduler, you can test again if app will crash, sorry for this inconvenience.

I guess app crashes for me because I used Node v16, when use Node v12 I also get UnhandledPromiseRejectionWarning. But UnhandledPromiseRejectionWarning means that error catcher doesn't catch these errors, it has to be fixed. Promise rejection should be handled and just error message logged into console.

@maxceem
Copy link
Contributor

maxceem commented Oct 19, 2021

Looks like after you wrapped other method with your catcher there is no crash anymore and no UnhandledPromiseRejectionWarning anymore for me.
Do you still get UnhandledPromiseRejectionWarning after you wrapped other method using your error catcher?

@painterner
Copy link
Collaborator Author

Looks like after you wrapped other method with your catcher there is no crash anymore and no UnhandledPromiseRejectionWarning anymore for me. Do you still get UnhandledPromiseRejectionWarning after you wrapped other method using your error catcher?

No

@painterner
Copy link
Collaborator Author

painterner commented Oct 19, 2021 •
edited
Loading

Updated for send invitation request. test passed

maxceem
maxceem approved these changes Oct 19, 2021
View reviewed changes
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.

Thank you @painterner, all works good now.

@maxceem maxceem merged commit d33f1a1 into topcoder-platform:feature/interview-nylas Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxceem maxceem maxceem approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@painterner @maxceem