ref: migrate transfer ownership flow to TypeScript #718

karrui · 2020-11-24T03:44:18Z

Problem

This PR migrates the transfer ownership flow to TypeScript.

Solution

Features:

feat(AdminFormSvc): add transferFormOwnership fn
feat(AdminFormCtl): add handleTransferFormOwnership handler fn
feat(AdminFormRoutes): use new controller fn for transferring owner
- Old transferOwner Javascript function is removed

Improvements:

add tighter pre-validate hook for form schema
update Form.transferOwner instance method to return .save()
update parameters for Form.transferOwner to accept both a currentOwner and newOwner IUserSchema object, forcing the caller to retrieve those before calling the function

Tests

Add tests for all new functions in the service and controller layer.
Note that the route layer has not been migrated and thus do not have integration tests yet.

add missing tests for findAdminById

karrui · 2020-11-24T03:44:48Z

@awhdesmond this touches your code, so please review it if you want to! Will greatly appreciate it :)

src/app/models/form.server.model.ts

src/app/modules/form/admin-form/admin-form.service.ts

liangyuanruo

minor changes requested! let me know your thoughts

karrui · 2020-11-26T11:35:20Z

minor changes requested! let me know your thoughts

Updated the responsibility into the service layer. Please rereview and let me know your thoughts. A con of this change is that the service function now looks a little intimidating, even though it's still (imho) fairly readable.

src/app/modules/form/admin-form/admin-form.service.ts

tests/unit/backend/models/form.server.model.spec.ts

liangyuanruo

Approving first as it's all pretty much there

liangyuanruo

to refactor error handling in transferFormOwnership function

Not needed, since the form the instance method is acting on should already have the required information, and should only have a single source of truth. The issue where the current logged in user calling this method to transfer ownership of the form even when the logged in user is not the given form's owner should be mitigated in the controller layer, where checks for whether the current user is the form admin should (and currently is) undertaken.

See #718 (comment)

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

src/app/modules/form/admin-form/admin-form.service.ts

tests/unit/backend/models/form.server.model.spec.ts

src/app/modules/form/admin-form/admin-form.service.ts

src/app/modules/form/admin-form/admin-form.utils.ts

improve clarity

prevents extra db call

karrui · 2020-12-07T08:50:12Z

Ready for rereview.

# Conflicts: # src/app/models/form.server.model.ts # src/app/modules/form/admin-form/__tests__/admin-form.controller.spec.ts # src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts # src/app/modules/form/admin-form/admin-form.controller.ts # src/app/modules/form/admin-form/admin-form.service.ts # src/app/modules/form/admin-form/admin-form.utils.ts # src/types/form.ts # tests/unit/backend/controllers/admin-forms.server.controller.spec.js # tests/unit/backend/models/form.server.model.spec.ts

liangyuanruo · 2020-12-08T03:51:22Z

src/app/models/form.server.model.ts

+    this.permissionList =
+      this.permissionList?.filter((item) => item.email !== newOwner.email) ?? []


did we ever figure out if a default value would be provided by mongoose on read, if it wasn't present in the DB?

yea, will automatically have an empty array: https://mongoosejs.com/docs/schematypes.html#arrays

so this should be safe?

Suggested change

this.permissionList =

this.permissionList?.filter((item) => item.email !== newOwner.email) ?? []

this.permissionList =

this.permissionList.filter((item) => item.email !== newOwner.email)

TypeScript will complain, but that is because it is optional in the schema. Will probably make a IFormDocument type that has all the default optionals populated as required to avoid this then.

I'll do the retyping in a separate PR to avoid polluting this even more

liangyuanruo · 2020-12-08T03:56:26Z

src/app/models/form.server.model.ts

+        const validationError = this.invalidate(
+          'admin',
+          'Admin for this form is not found.',
+        ) as mongoose.Error.ValidationError
+        return next(validationError)


nice use of invalidate(), this is what we should be doing

tests/unit/backend/models/form.server.model.spec.ts

liangyuanruo

minor nitpicks, feel free to leave aside. lgtm otherwise

# Conflicts: # src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts # src/app/modules/form/admin-form/admin-form.controller.ts # src/app/modules/form/admin-form/admin-form.service.ts

karrui requested review from awhdesmond, mantariksh and tshuli November 24, 2020 03:44

liangyuanruo reviewed Nov 26, 2020

View reviewed changes

src/app/models/form.server.model.ts Outdated Show resolved Hide resolved

liangyuanruo reviewed Nov 26, 2020

View reviewed changes

src/app/models/form.server.model.ts Outdated Show resolved Hide resolved

liangyuanruo reviewed Nov 26, 2020

View reviewed changes

src/app/models/form.server.model.ts Outdated Show resolved Hide resolved

liangyuanruo reviewed Nov 26, 2020

View reviewed changes

src/app/modules/form/admin-form/admin-form.service.ts Outdated Show resolved Hide resolved

liangyuanruo suggested changes Nov 26, 2020

View reviewed changes

karrui requested a review from liangyuanruo November 26, 2020 11:35

liangyuanruo reviewed Nov 30, 2020

View reviewed changes

src/app/modules/form/admin-form/admin-form.service.ts Outdated Show resolved Hide resolved

liangyuanruo reviewed Nov 30, 2020

View reviewed changes

tests/unit/backend/models/form.server.model.spec.ts Show resolved Hide resolved

liangyuanruo approved these changes Nov 30, 2020

View reviewed changes

karrui force-pushed the ref/transfer-ownership branch from f271810 to 4fe3124 Compare November 30, 2020 07:16

liangyuanruo suggested changes Dec 1, 2020

View reviewed changes

karrui added 12 commits December 2, 2020 10:44

feat(FormModel): update transferOwner instance method to return itself

49fb60d

feat(FormModel): remove autogenerated id on permissionList objects

0765cd9

test(FormModel): move getDashboardForms into correct static desc block

2500701

test(FormModel): add tests for transferOwner instance method

46a5372

feat(AdminFormSvc): add transferFormOwnership fn

0384444

test(AdminFormSvc): add tests for transferFormOwnership

ffea92c

feat(AdminFormCtl): add handleTransferFormOwnership handler fn

40027e6

feat(AdminFormRoutes): use new controller fn for transferring owner

b1799df

test(AdminFormCtl): add tests for handleTransferFormOwnership

59d54c4

feat: remove old transferOwner function

bb7b8b4

feat(FormModel): add tighter pre-validate hook for form schema

42446cd

karrui added 8 commits December 2, 2020 10:57

test(FormModel): add tests for increased validation impl

7d8a7a0

feat(FormModel): reduce responsibilities of transferOwner method

e9e26cb

test(UserService): add missing tests for findAdminById

e5ebe51

feat(UserSvc): add findAdminByEmail fn (and tests)

baf06f9

feat(AdminFormSvc): move transfer ownership checks into service fn

847df8d

test(AdminFormSvc): update tests for transferFormOwnership

a86349a

feat(FormModel): restore _id generation for permissionList

dd8a336

See #718 (comment)

feat(AdminFormSvc): return TransferOwnershipError on missing new owner

9c11b20

karrui force-pushed the ref/transfer-ownership branch from 4fe3124 to 9c11b20 Compare December 2, 2020 03:16

karrui requested a review from liangyuanruo December 2, 2020 03:17

mantariksh approved these changes Dec 2, 2020

View reviewed changes

karrui added 4 commits December 7, 2020 13:06

ref: rename findAdminBy* to findUserBy*

b7742c4

improve clarity

Merge branch 'develop' into ref/transfer-ownership

be3c617

feat(AdminFormSvc): move ownerEmail equality check up one step

5beffd9

prevents extra db call

feat: change transfer ownership error return 400

3ffc43f

karrui requested a review from mantariksh December 7, 2020 08:50

mantariksh approved these changes Dec 8, 2020

View reviewed changes

liangyuanruo reviewed Dec 8, 2020

View reviewed changes

tests/unit/backend/models/form.server.model.spec.ts Outdated Show resolved Hide resolved

liangyuanruo approved these changes Dec 8, 2020

View reviewed changes

karrui added 3 commits December 8, 2020 16:32

Merge branch 'develop' into ref/transfer-ownership

f52126e

# Conflicts: # src/app/modules/form/admin-form/__tests__/admin-form.service.spec.ts # src/app/modules/form/admin-form/admin-form.controller.ts # src/app/modules/form/admin-form/admin-form.service.ts

ref(FormModel): rename getDashboardForms to getAllByUserIdOrEmail

361bb58

ref(FormModel): re-renaming to getMetaByUserIdOrEmail

281b542

karrui merged commit b6593cf into develop Dec 8, 2020

karrui deleted the ref/transfer-ownership branch December 8, 2020 10:23

mantariksh mentioned this pull request Dec 10, 2020

build: release 4.49.0 #842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: migrate transfer ownership flow to TypeScript #718

ref: migrate transfer ownership flow to TypeScript #718

karrui commented Nov 24, 2020 •

edited

Loading

karrui commented Nov 24, 2020

liangyuanruo left a comment

karrui commented Nov 26, 2020

liangyuanruo left a comment

liangyuanruo left a comment

karrui commented Dec 7, 2020

liangyuanruo Dec 8, 2020

karrui Dec 8, 2020

liangyuanruo Dec 8, 2020

karrui Dec 8, 2020

karrui Dec 8, 2020

liangyuanruo Dec 8, 2020

liangyuanruo left a comment •

edited

Loading

		this.permissionList =
		this.permissionList?.filter((item) => item.email !== newOwner.email) ?? []

ref: migrate transfer ownership flow to TypeScript #718

ref: migrate transfer ownership flow to TypeScript #718

Conversation

karrui commented Nov 24, 2020 • edited Loading

Problem

Solution

Tests

karrui commented Nov 24, 2020

liangyuanruo left a comment

Choose a reason for hiding this comment

karrui commented Nov 26, 2020

liangyuanruo left a comment

Choose a reason for hiding this comment

liangyuanruo left a comment

Choose a reason for hiding this comment

karrui commented Dec 7, 2020

liangyuanruo Dec 8, 2020

Choose a reason for hiding this comment

karrui Dec 8, 2020

Choose a reason for hiding this comment

liangyuanruo Dec 8, 2020

Choose a reason for hiding this comment

karrui Dec 8, 2020

Choose a reason for hiding this comment

karrui Dec 8, 2020

Choose a reason for hiding this comment

liangyuanruo Dec 8, 2020

Choose a reason for hiding this comment

liangyuanruo left a comment • edited Loading

Choose a reason for hiding this comment

karrui commented Nov 24, 2020 •

edited

Loading

liangyuanruo left a comment •

edited

Loading