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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Adds module crud and module crud tests #1377

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0803b6
adds mod crud
anso3 Jan 31, 2022
c08c7cd
adds modulecrud
anso3 Jan 31, 2022
42299ee
modulecrud
anso3 Jan 31, 2022
a0f5ab0
takes module crud comments out
anso3 Feb 1, 2022
a4c31a3
fixes module red and module crud, puts crud into resolvers
anso3 Feb 2, 2022
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
34 changes: 0 additions & 34 deletions .env.example
View file
Open in desktop

This file was deleted.

78 changes: 78 additions & 0 deletions graphql/queryResolvers/moduleCrud.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* @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 delete module', () => {
prismaMock.module.delete.mockResolvedValue({ success: true })
expect(deleteModule({}, { id: 1 })).resolves.toEqual({ success: true })
})
})
29 changes: 29 additions & 0 deletions graphql/queryResolvers/moduleCrud.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import type { MutationDeleteModuleArgs, MutationAddModuleArgs } from '..'
import prisma from '../../prisma'

export const modules = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm findMany should already return a promise, right? If that's the case, then it is redundant to make this function async

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also we should add error handling

try {
  return prisma.module.findMany(...)
} catch(err) {
  // SOME LOGGING
  // error handling logic
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@anthonykhoa good point about `async.

also your sample code is wrong, try/catch does not work for promises. You would have to do:

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 [])

Copy link
Collaborator

Choose a reason for hiding this comment

The 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) => {
const { authorId, content, lessonId, name } = args
Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't have authorId be passed in, it should be retrieved from the cookie.

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is authorId the same as userId?
And is it a requirement that the user has to be logged in before making a call to this resolver?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If authorId=userId, then we set in in req.session already

c0d3-app/helpers/controllers/authController.ts

Line 54 in 4b140cb

session.userId = user.id

if (!authorId || !content || !name || !lessonId) {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 content value as empty string(''), is that considered a missing parameter?

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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({
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 catch @anthonykhoa ?

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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.
Something that tells us creation of module failed, for example MODULE_CREATION_FAILED_ERROR:${JSON.stringify(error)}

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The 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 Module Creation Failed. IMO having an error notification popup would be ideal user experience incase module creation fails.

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 create button didn't register, so the user clicks on create again. Having an error notification in this scenario would create potential for less confusion for users who enter a situation where the create module feature is not working.

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Apollo already provides an onError option with their useMutation and useQuery hooks.
https://www.apollographql.com/docs/react/data/error-handling/

data: { authorId, content, lessonId, name }
})
}

export const deleteModule = async (
_parent: void,
args: MutationDeleteModuleArgs
) => {
const { id } = args
await prisma.module.delete({ where: { id } })
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should wrap this in a try/catch block

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you suggest we do in the catch block @anthonykhoa ?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

@anthonykhoa anthonykhoa Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@mino323 See comment thread on the addModule function for the discussion about error handling

return { success: true }
}