-
Notifications
You must be signed in to change notification settings - Fork 69
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
Adds module crud and module crud tests #1377
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/** | ||
* @jest-environment node | ||
*/ | ||
import prismaMock from '../../__tests__/utils/prismaMock' | ||
import { modules, addModule, deleteModule } from './moduleCrud' | ||
|
||
const mockModules = [ | ||
{ | ||
name: 'Quality omlet', | ||
id: 1 | ||
}, | ||
{ | ||
name: 'Super booster', | ||
id: 2 | ||
} | ||
] | ||
const ctx = { | ||
req: { | ||
user: { isAdmin: true } | ||
} | ||
} | ||
describe('It should return modules', () => { | ||
test('Should return modules', () => { | ||
prismaMock.module.findMany.mockResolvedValue(mockModules) | ||
expect(modules()).resolves.toEqual(mockModules) | ||
}) | ||
}) | ||
|
||
describe('It should add a modules', () => { | ||
test('Should create module', async () => { | ||
prismaMock.module.create.mockResolvedValue({ | ||
authorId: 1, | ||
content: 'testing', | ||
name: 'Using functions to make pizza', | ||
lessonId: 1 | ||
}) | ||
expect( | ||
await addModule( | ||
{}, | ||
{ | ||
authorId: 1, | ||
content: 'testing', | ||
name: 'Using functions to make pizza', | ||
lessonId: 1 | ||
} | ||
) | ||
).toEqual({ | ||
authorId: 1, | ||
content: 'testing', | ||
name: 'Using functions to make pizza', | ||
lessonId: 1 | ||
}) | ||
}) | ||
test('Create modules should have all parameters', async () => { | ||
expect( | ||
addModule({}, { authorId: 1, name: 'Hi all', content: 'This module' }) | ||
).rejects.toThrowError('Missing parameters'), | ||
expect( | ||
addModule({}, { authorId: 1, lessonId: 1, content: 'This module' }) | ||
).rejects.toThrowError('Missing parameters'), | ||
expect( | ||
addModule( | ||
{}, | ||
{ content: 'This Module', name: 'Hi all', content: 'This module' } | ||
) | ||
).rejects.toThrowError('Missing parameters'), | ||
expect( | ||
addModule({}, { authorId: 1, lessonId: 1, name: 'Hi all' }) | ||
).rejects.toThrowError('Missing parameters') | ||
}) | ||
}) | ||
|
||
describe('It should test delete', () => { | ||
test('It should have an id', async () => { | ||
expect(deleteModule({}, {})).rejects.toThrowError('Missing parameter') | ||
}) | ||
test('it should delete module', () => { | ||
prismaMock.module.delete.mockResolvedValue({ success: true }) | ||
expect(deleteModule({}, { id: 1 })).resolves.toEqual({ success: true }) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,35 @@ | ||||
// import { Context } from '../../@types/helpers' | ||||
import type { MutationDeleteModuleArgs, MutationAddModuleArgs } from '..' | ||||
import prisma from '../../prisma' | ||||
// import { isAdminOrThrow } from '../../helpers/isAdmin' | ||||
|
||||
export const modules = async () => { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should use typescript for all functions you declare so that you can define what they return |
||||
return prisma.module.findMany({ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also we should add error handling
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anthonykhoa good point about `async. also your sample code is wrong, return prisma.module.findMany(...).catch(...) We could return empty array on errors so we always guarantee returning an array: return prisma.module.findMany(...).catch( _e => return []) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah good idea. returning empty array is appropriate for this scenario. |
||||
include: { | ||||
author: true | ||||
} | ||||
}) | ||||
} | ||||
|
||||
export const addModule = async (_parent: void, args: MutationAddModuleArgs) => { | ||||
//isAdminOrThrow(req) will add after, easier for testing | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not understanding this. Why are we not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @anthonykhoa is right, dont put testing code into production! |
||||
// const authorId = ctx.req.user?.id | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this commented line if its unneeded |
||||
const { authorId, content, lessonId, name } = args | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we shouldn't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is authorId the same as userId? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If authorId=userId, then we set in in
|
||||
if (!authorId || !content || !name || !lessonId) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we only trying to check if all arguments are present, or instead are we checking if the arguments are not the correct value? For example, if I passed the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are only trying to check if all required arguments are present, you don't have to do that. Graphql takes care of that for you already -- if you marked in the typedefs that arguments are required for this resolver, then the graphql request will already return an error if a person does not include required arguments in their request. |
||||
throw new Error('Missing parameters') | ||||
} | ||||
return await prisma.module.create({ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if this results in error? We should wrap this in a try/catch block There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this returns an error the front end will get an error, which could render a message. What do you suggest we do in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah i see that makes sense. In this case I think we should log the error then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we don't want to return an error to the frontend, I guess we can return some default value like But I think we should throw the error. And then we handle the error from the frontend too. If there is an error thrown in this resolver, we can have error notification popup on the screen saying something like I can imagine a scenario wherein a user tries to create a module, and it fails; the user thinks it could be their internet, or maybe the user thinks their click on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apollo already provides an |
||||
data: { authorId, content, lessonId, name } | ||||
}) | ||||
} | ||||
|
||||
export const deleteModule = async ( | ||||
_parent: void, | ||||
args: MutationDeleteModuleArgs | ||||
) => { | ||||
// isAdminOrThrow(req) | ||||
const { id } = args | ||||
if (!id) throw new Error('Missing parameter') | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question here, if you have marked Do you still need that check? I would think graphql would handle it for you |
||||
await prisma.module.delete({ where: { id } }) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should wrap this in a try/catch block There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do you suggest we do in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log the error. Incase anything goes wrong, logs would be helpful in issue triaging. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mino323 See comment thread on the |
||||
return { success: true } | ||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this commented out line