Convert user.controller.js to async/await syntax.

[lindapaiste]

Ref #521

Changes:

• Convert all user endpoint controllers to use async/await syntax.
• Got rid of async.waterfall() chaining.
• Rename function random to generateToken and make it aysnc.
• Make user method comparePassword be async, with a minor change to the passport.js file to support this.
• Changed 404 error messages from 'Document not found' to 'User not found'.

Limitations:

• I am using await mail.send(mailOptions); as this function returns a Promise, but the mail class doesn't handle errors correctly and will resolve instead of reject. I will fix this in an additional PR.
• I'm still unsure of the best way to handle unexpected errors from async functions. Some of these handlers have try/catch blocks, while others will throw the error and reject the promise. I basically kept the same amount of error handling on each function as was there before.
• Not every error is handled perfectly, but it's as good or better than before.
• Some of the code is very repetitive across handlers (particularly for sending emails). This can be cleaned up in the future.

I have verified that this pull request:

• has no linting errors (npm run lint)
• has no test errors (npm run test)
• is from a uniquely-named feature branch and is up to date with the develop branch.
• is descriptively named and links to an issue number, i.e. Fixes #123

[Swarnendu0123]

We can use async await syntax here also.

[lindapaiste]

For sure. I figured I’d convert the passport file in a separate PR. So I made the minimum change to keep it working for now.

[raclim]

Thanks for working on this, I'm going to merge this in!

[raclim]

This is great to note!

[raclim]

Hi @lindapaiste! Just wanted to share some of the server logs I found during the days that peaked in errors. I noticed that during those times, the server would restart more frequently and that these two errors would show up mostly around those moments:

Error 1

Error 2

Based on the first image, I think it's coming from comparePassword in the user.js file? Or maybe from removing the return on line 51 in passport.js?

I also have a two more screenshots of other logs when the server restarts from two different days. I hope this was able to provide some context but let me know if you need more logs or other info here, thanks!!!

Log from 03/22/24

Log from 03/24/24

[lindapaiste]

@raclim thank you for the info! The bcrypt.compare is this line. "Illegal arguments: string, undefined" means that this.password is undefined. So it sounds like the function is not getting the right this context?

The first thing that I would try is to change it back from async function comparePassword to function comparePassword. It does not await anything so technically it does not need the async keyword. It will behave the same with or without it -- both return a Promise that can be await-ed.

I'm not sure how you want to address troubleshooting this because I don't want to break the production server again. I think that it was working fine for me on localhost? I will look at that again but not this second.

This is assuming that the bcypt error is new. The other possibility is that the error would occasionally get thrown before too but was being caught and now it is no longer being caught. That's where the oddly-mixed syntax in the passport.js file comes into play. And now my brain is exploding a little bit thinking about whether the outer .catch on line 56 would catch errors from the promise on line 46. Before we were getting an innerErr variable and we were not using it. Which let me to think that we weren't properly handling errors there before and therefore the new version is no worse. But now I'm 💡 that in the case of an error before it would call the same callback and would have if (isMatch) be false. But the callback is not being called currently because it's in a then which is only fired when the Promise resolves and there's no catch. The more that I think about it I'm more convinced that it's this.

I will fix this when I get a chance. Quick solution is to add an explicit catch. I think even just adding a return at the start of line 48 might do it? By allowing the outer catch to catch it? The longer term is that we should rewrite the passport.js file to be async/await too.

I've also got to think about what situations would have a User object with this.password undefined. Is it their account was created with GitHub or Google linking? We might want to return a more specific error in those situations, to the effect of "The account with this email/username does not have a password. Please log in using your linked GitHub/Google account." Alternatively, if we consider this to be a "expected" error rather than an unexpected error then we might want to only call bcyrpt.compare when this.password is defined and otherwise we can immediately return false from comparePassword but resolve the Promise rather than rejecting it. Lots to ponder. But yes I can def fix this (in too many ways lol).

[raclim]

Thanks so much for breaking this down!!!

I just checked through the database and it seems like that accounts created with Github or Google don't have passwords. There are accounts that do have both Github/Google credentials and a password, but I think those accounts exist because Github/Google was linked after creating their account.

Alternatively, if we consider this to be a "expected" error rather than an unexpected error then we might want to only call bcyrpt.compare when this.password is defined and otherwise we can immediately return false from comparePassword but resolve the Promise rather than rejecting it.

I think this sounds good to me? But also down for other solutions you think would be better as well! Also agree that we want to rewrite the passport.js file at some point down the line 😂