feat: using `neverthrow` to explicitly handle errors in UserController #371

karrui · 2020-09-24T05:39:11Z

Problem

Similar to #332, this is an extension PR for #142 to implement explicit error handling to the endpoints.

Solution

Improvements:

Add explicit handling using neverthrow's FP-like Result class to return either success oks or error tracks for entire User module endpoints

Note that sms related files should technically be updated to be in their own module, and the other sms-related functions should also be converted to use Result. However, that is outside the scope of this PR and should be done in their respective PRs.

Add lint ordering rule for test root import so test imports are less messy
Inline celebrate rules in user.routes like auth routes
Remove MalformedOtpError, it has now been replaced by ApplicationError
Add utils/hash containing the neverthrown versions of bcrypt.hash and bcrypt.compare

Tests

Add express-auth helper function to get authed session for integration tests that requires the user to be logged in.
User tests are moved to modules/user/ __test__ directory
Tests are refactored to use new FPfied methods for UserService and SmsService
Add integration tests for entire /user route
Rewrite SmsService tests using Jest and TypeScript

karrui · 2020-09-29T03:02:27Z

Drafting this PR to change how controllers call services; they should be called step by step for additional clarity, checking errors every step of the way as discussed with Yuan

src/app/modules/auth/auth.service.ts

liangyuanruo · 2020-09-29T03:11:27Z

src/app/modules/user/user.routes.ts

 import { Router } from 'express'

 import * as UserController from './user.controller'
-import * as UserRules from './user.rules'


is there a new preference for inlining the rules?

Tbh, they are just used in this place, and having to refer to yet another *.rules file when in *.routes files seems a little annoying when developing or reviewing

It has now been replaced by ApplicationError

karrui · 2020-09-29T07:34:26Z

Sorry, sticky fingers. Not refactored to check for errors every step of the way yet.

karrui · 2020-09-29T07:52:43Z

Add error handling every step of the way.

src/app/modules/auth/__tests__/auth.routes.spec.ts

src/app/utils/hash.ts

src/app/modules/auth/auth.service.ts

src/app/modules/user/user.service.ts

src/app/modules/user/user.utils.ts

tests/integration/helpers/express-auth.ts

tests/unit/backend/services/sms.service.spec.ts

# Conflicts: # src/app/factories/sms.factory.js # src/app/modules/auth/__tests__/auth.controller.spec.ts # src/app/modules/auth/__tests__/auth.routes.spec.ts # src/app/modules/auth/auth.utils.ts # src/app/modules/user/__tests__/user.controller.spec.ts # src/app/modules/user/user.controller.ts # src/app/services/sms/sms.service.ts

mantariksh

some remaining questions about backwards compatibility

src/app/modules/user/user.controller.ts

tests/integration/helpers/express-auth.ts

mantariksh

èl géê tëē ėm

karrui requested review from liangyuanruo, arshadali172, mantariksh and tshuli September 24, 2020 05:39

karrui changed the base branch from develop to feat/track-user-login September 24, 2020 05:40

karrui marked this pull request as draft September 29, 2020 03:01

liangyuanruo reviewed Sep 29, 2020

View reviewed changes

src/app/modules/auth/auth.service.ts Outdated Show resolved Hide resolved

liangyuanruo reviewed Sep 29, 2020

View reviewed changes

Base automatically changed from feat/track-user-login to develop September 29, 2020 03:18

karrui added 20 commits September 29, 2020 11:20

lint: add lint ordering rule for test root import

716381b

refactor(User): move user tests to __test__ directory

9a2674b

refactor: move generateOtp to utils/hash and rename to hashData

9203e2c

refactor(UserService): write neverthrown variant for createContactOtp

a49ae02

test(SmsService): rewrite tests with TypeScript

69c2ef6

refactor(SmsService): write neverthrown sendAdminContactOtp

e71a3db

feat: add standard typing for mapRouteError controller utils

86aeb91

refactor(UserCtl): use new ResultAsync fns in handleContactSendOtp

2c1a01a

feat(SmsService): update JsDoc comments

a05e1b7

refactor(UserRoutes): inline celebrate rules

9306541

refactor(hashUtil): add neverthrown compareData function

b1259ad

refactor(UserService): rewrite neverthrown verifyContactOtp fn

6046d42

feat(User): remove MalformedOtpError

ad2cf60

It has now been replaced by ApplicationError

refactor(UserService): rewrite neverthrown updateUserContact fn

9159f11

refactor(UserService): rewrite neverthrown getPopulatedUserById fn

973e72b

refactor(UserCtl): use new UserService neverthrow fns

a3a8b21

feat(UserUtils): return 422 for MissingUserError

796dab7

feat(UserRoutes): add route documentation

035669d

chore: add supertest-session package and typing

4e6531b

test: add express-auth helper function to get authed session

ac7c9c1

karrui marked this pull request as ready for review September 29, 2020 07:33

karrui marked this pull request as draft September 29, 2020 07:34

refactor(UserCtl): check errors in each step instead of at the end

9acb1b5

karrui marked this pull request as ready for review September 29, 2020 07:50

refactor(hashUtils): rename compareData to compareHash

b3a28fc

karrui requested a review from liangyuanruo September 29, 2020 07:52

This was referenced Oct 1, 2020

feat: add billing module to handle /billing endpoints #398

Merged

feat: add analytics module to handle /analytics endpoints #403

Merged

mantariksh suggested changes Oct 5, 2020

View reviewed changes

karrui added 8 commits October 5, 2020 16:34

feat(User): return 401 status when mapping InvalidOtpError

8b3a9fc

test(UserCtl): add more test cases for error handling

38d5ca8

test(SmsService): rename smsCountSpy to camelcase from UPPER_CASE

1ca7a04

test(AuthRoute): add note to say that request is not logged in yet

9a4b049

feat(SmsService): now returns better err msg for invalid number err

6a9a5dc

fix: fix broken tests due to resolving merge conflicts

9e804f7

feat(UserCtl): convert all res.send calls to res.json calls

8e9a4f4

mantariksh reviewed Oct 12, 2020

View reviewed changes

src/app/modules/user/user.controller.ts Outdated Show resolved Hide resolved

src/app/modules/user/user.controller.ts Outdated Show resolved Hide resolved

LoneRifle reviewed Oct 12, 2020

View reviewed changes

tests/integration/helpers/express-auth.ts Outdated Show resolved Hide resolved

karrui added 2 commits October 12, 2020 13:18

test(expressAuth): add documentation and rename to createAuthedSession

5496aa4

feat(UserCtl): return error as string instead of message object

bc95e6c

karrui requested a review from mantariksh October 12, 2020 05:38

test(expressAuth): update grammar on JSDoc

7be177e

mantariksh approved these changes Oct 12, 2020

View reviewed changes

karrui merged commit 293e843 into develop Oct 12, 2020

karrui deleted the feat/neverthrow-user branch October 12, 2020 06:03

This was referenced Oct 12, 2020

Add integration tests for /billing and /analytics endpoints #456

Closed

build: release v4.40.0 #460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: using `neverthrow` to explicitly handle errors in UserController #371

feat: using `neverthrow` to explicitly handle errors in UserController #371

karrui commented Sep 24, 2020 •

edited

Loading

karrui commented Sep 29, 2020 •

edited

Loading

liangyuanruo Sep 29, 2020

karrui Sep 29, 2020

karrui commented Sep 29, 2020

karrui commented Sep 29, 2020

mantariksh left a comment

mantariksh left a comment

feat: using neverthrow to explicitly handle errors in UserController #371

feat: using neverthrow to explicitly handle errors in UserController #371

Conversation

karrui commented Sep 24, 2020 • edited Loading

Problem

Solution

Tests

karrui commented Sep 29, 2020 • edited Loading

liangyuanruo Sep 29, 2020

Choose a reason for hiding this comment

karrui Sep 29, 2020

Choose a reason for hiding this comment

karrui commented Sep 29, 2020

karrui commented Sep 29, 2020

mantariksh left a comment

Choose a reason for hiding this comment

mantariksh left a comment

Choose a reason for hiding this comment

feat: using `neverthrow` to explicitly handle errors in UserController #371

feat: using `neverthrow` to explicitly handle errors in UserController #371

karrui commented Sep 24, 2020 •

edited

Loading

karrui commented Sep 29, 2020 •

edited

Loading