Skip to content
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.

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

ref: migrate transfer ownership flow to TypeScript #718

Merged
merged 28 commits into from
Dec 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
49fb60d
feat(FormModel): update transferOwner instance method to return itself
karrui Nov 23, 2020
dbfbabb
feat(FormModel): remove currentOwner parameter from Form#transferOwner
karrui Nov 23, 2020
0765cd9
feat(FormModel): remove autogenerated id on permissionList objects
karrui Nov 23, 2020
2500701
test(FormModel): move getDashboardForms into correct static desc block
karrui Nov 23, 2020
46a5372
test(FormModel): add tests for transferOwner instance method
karrui Nov 23, 2020
0384444
feat(AdminFormSvc): add transferFormOwnership fn
karrui Nov 23, 2020
ffea92c
test(AdminFormSvc): add tests for transferFormOwnership
karrui Nov 23, 2020
40027e6
feat(AdminFormCtl): add handleTransferFormOwnership handler fn
karrui Nov 23, 2020
b1799df
feat(AdminFormRoutes): use new controller fn for transferring owner
karrui Nov 23, 2020
59d54c4
test(AdminFormCtl): add tests for handleTransferFormOwnership
karrui Nov 24, 2020
bb7b8b4
feat: remove old transferOwner function
karrui Nov 24, 2020
42446cd
feat(FormModel): add tighter pre-validate hook for form schema
karrui Nov 26, 2020
7d8a7a0
test(FormModel): add tests for increased validation impl
karrui Nov 26, 2020
e9e26cb
feat(FormModel): reduce responsibilities of transferOwner method
karrui Nov 26, 2020
e5ebe51
test(UserService): add missing tests for findAdminById
karrui Nov 26, 2020
baf06f9
feat(UserSvc): add findAdminByEmail fn (and tests)
karrui Nov 26, 2020
847df8d
feat(AdminFormSvc): move transfer ownership checks into service fn
karrui Nov 26, 2020
a86349a
test(AdminFormSvc): update tests for transferFormOwnership
karrui Nov 26, 2020
dd8a336
feat(FormModel): restore _id generation for permissionList
karrui Nov 26, 2020
9c11b20
feat(AdminFormSvc): return TransferOwnershipError on missing new owner
karrui Dec 2, 2020
b7742c4
ref: rename findAdminBy* to findUserBy*
karrui Dec 7, 2020
be3c617
Merge branch 'develop' into ref/transfer-ownership
karrui Dec 7, 2020
5beffd9
feat(AdminFormSvc): move ownerEmail equality check up one step
karrui Dec 7, 2020
3ffc43f
feat: change transfer ownership error return 400
karrui Dec 7, 2020
0cf974e
Merge branch 'develop' into ref/transfer-ownership
karrui Dec 7, 2020
f52126e
Merge branch 'develop' into ref/transfer-ownership
karrui Dec 8, 2020
361bb58
ref(FormModel): rename getDashboardForms to getAllByUserIdOrEmail
karrui Dec 8, 2020
281b542
ref(FormModel): re-renaming to getMetaByUserIdOrEmail
karrui Dec 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 0 additions & 31 deletions src/app/controllers/admin-forms.server.controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,36 +336,5 @@ function makeModule(connection) {
req.submission = submission
return next()
},

/**
* Transfer a form to another user
* @param {Object} req - Express request object
* @param {Object} res - Express response object
*/
transferOwner: async function (req, res) {
const newOwnerEmail = req.body.email

// Transfer owner and Save the form
try {
await req.form.transferOwner(req.session.user, newOwnerEmail)
} catch (err) {
logger.error({
message: err.message,
meta: {
action: 'makeModule.transferOwner',
...createReqMeta(req),
},
err,
})
return res.status(StatusCodes.CONFLICT).json({ message: err.message })
}
req.form.save(function (err, savedForm) {
if (err) return respondOnMongoError(req, res, err)
savedForm.populate('admin', (err) => {
if (err) return respondOnMongoError(req, res, err)
return res.json({ form: savedForm })
})
})
},
}
}
47 changes: 24 additions & 23 deletions src/app/models/form.server.model.ts
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import BSON from 'bson-ext'
import { compact, filter, pick, uniq } from 'lodash'
import { Mongoose, Schema, SchemaOptions } from 'mongoose'
import { compact, pick, uniq } from 'lodash'
import mongoose, { Mongoose, Schema, SchemaOptions } from 'mongoose'
import validator from 'validator'

import {
AuthType,
BasicField,
Colors,
DashboardFormView,
FormLogoState,
FormMetaView,
FormOtpData,
IEmailFormModel,
IEmailFormSchema,
Expand Down Expand Up @@ -443,26 +443,16 @@ const compileFormModel = (db: Mongoose): IFormModel => {
}

// Transfer ownership of the form to another user
FormSchema.methods.transferOwner = async function (
currentOwner: IUserSchema,
newOwnerEmail: string,
) {
// Verify that the new owner exists
const newOwner = await User.findOne({ email: newOwnerEmail })
if (newOwner == null) {
throw new Error(
`${newOwnerEmail} must have logged in once before being added as Owner`,
)
}

// Update form's admin to new owner's id
FormSchema.methods.transferOwner = async function (currentOwner, newOwner) {
// Update form's admin to new owner's id.
this.admin = newOwner._id

// Remove new owner from perm list and include previous owner as an editor
this.permissionList = filter(this.permissionList, (item) => {
return item.email !== newOwnerEmail
})
// Remove new owner from perm list and include previous owner as an editor.
this.permissionList =
this.permissionList?.filter((item) => item.email !== newOwner.email) ?? []
Comment on lines +451 to +452
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this.permissionList.push({ email: currentOwner.email, write: true })

return this.save()
}

// Statics
Expand Down Expand Up @@ -520,11 +510,11 @@ const compileFormModel = (db: Mongoose): IFormModel => {
return form.save()
}

FormSchema.statics.getDashboardForms = async function (
FormSchema.statics.getMetaByUserIdOrEmail = async function (
this: IFormModel,
userId: IUserSchema['_id'],
userEmail: IUserSchema['email'],
): Promise<DashboardFormView[]> {
): Promise<FormMetaView[]> {
return (
this.find()
// List forms when either the user is an admin or collaborator.
Expand Down Expand Up @@ -562,8 +552,19 @@ const compileFormModel = (db: Mongoose): IFormModel => {
// Validate that admin exists before form is created.
return User.findById(this.admin).then((admin) => {
if (!admin) {
throw new Error(`Admin for this form is not found.`)
const validationError = this.invalidate(
'admin',
'Admin for this form is not found.',
) as mongoose.Error.ValidationError
return next(validationError)
Comment on lines +555 to +559
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

// Remove admin from the permission list if they exist.
// This prevents the form owner from being both an admin and another role.
this.permissionList = this.permissionList?.filter(
(item) => item.email !== admin.email,
)

return next()
})
})
Expand Down
Loading