Adds module crud and module crud tests #1377

anso3 · 2022-01-31T20:11:45Z

This pr adds module crud and module crud tests.
This continues pr #1346

vercel · 2022-01-31T20:11:51Z

@mino323 is attempting to deploy a commit to a Personal Account owned by @garageScript on Vercel.

@garageScript first needs to authorize it.

codecov · 2022-01-31T20:16:31Z

Codecov Report

Merging #1377 (a4c31a3) into master (c498cab) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1377   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          146       147    +1     
  Lines         2450      2462   +12     
  Branches       638       626   -12     
=========================================
+ Hits          2450      2462   +12

Impacted Files	Coverage Δ
graphql/queryResolvers/moduleCrud.ts	`100.00% <100.00%> (ø)`

vercel · 2022-01-31T20:43:59Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/c0d3/c0d3-app/3QAsjtPGNg5tgC772YUzMrpH6T3u
✅ Preview: https://c0d3-app-git-fork-mino323-ago-c0d3.vercel.app

anthonykhoa · 2022-01-31T20:55:16Z

graphql/queryResolvers/moduleCrud.ts

+}
+
+export const addModule = async (_parent: void, args: MutationAddModuleArgs) => {
+  //isAdminOrThrow(req) will add after, easier for testing


I am not understanding this. Why are we not using isAdminOrThrow, and instead have it commented out?

@anthonykhoa is right, dont put testing code into production!

anthonykhoa · 2022-01-31T21:00:29Z

graphql/queryResolvers/moduleCrud.ts

@@ -0,0 +1,35 @@
+// import { Context } from '../../@types/helpers'


Remove this commented out line

anthonykhoa · 2022-01-31T21:01:29Z

graphql/queryResolvers/moduleCrud.ts

+
+export const addModule = async (_parent: void, args: MutationAddModuleArgs) => {
+  //isAdminOrThrow(req) will add after, easier for testing
+  // const authorId = ctx.req.user?.id


Remove this commented line if its unneeded

songz · 2022-02-01T01:07:08Z

graphql/queryResolvers/moduleCrud.ts

+) => {
+  // isAdminOrThrow(req)
+  const { id } = args
+  if (!id) throw new Error('Missing parameter')


question here, if you have marked id as required: https://github.com/garageScript/c0d3-app/pull/1354/files#diff-0776d3ad89186de849e067b2a7b9a9701408e7b983a6387c50819581ad6f8f9aR63

Do you still need that check? I would think graphql would handle it for you

anthonykhoa · 2022-02-01T20:06:23Z

What are these changes for? What do we need module crud for? I am confused about the context of this PR and why these changes are needed. Is there some other feature you are working on, and you need to make module crud as a step for that feature? Please add context by description, or link some issue

anthonykhoa · 2022-02-01T20:19:04Z

graphql/queryResolvers/moduleCrud.ts

+import prisma from '../../prisma'
+
+export const modules = async () => {
+  return prisma.module.findMany({


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

Also we should add error handling

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

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

Ah good idea. returning empty array is appropriate for this scenario.

anthonykhoa · 2022-02-01T20:25:07Z

graphql/queryResolvers/moduleCrud.ts

+  if (!authorId || !content || !name || !lessonId) {
+    throw new Error('Missing parameters')
+  }
+  return await prisma.module.create({


What if this results in error? We should wrap this in a try/catch block

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 ?

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)}

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.

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

anthonykhoa · 2022-02-01T20:26:04Z

graphql/queryResolvers/moduleCrud.ts

+  args: MutationDeleteModuleArgs
+) => {
+  const { id } = args
+  await prisma.module.delete({ where: { id } })


We should wrap this in a try/catch block

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

Log the error. Incase anything goes wrong, logs would be helpful in issue triaging.

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

anthonykhoa · 2022-02-01T20:30:12Z

graphql/queryResolvers/moduleCrud.ts

+
+export const addModule = async (_parent: void, args: MutationAddModuleArgs) => {
+  const { authorId, content, lessonId, name } = args
+  if (!authorId || !content || !name || !lessonId) {


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?

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.

anthonykhoa · 2022-02-01T23:06:50Z

graphql/queryResolvers/moduleCrud.ts

+import type { MutationDeleteModuleArgs, MutationAddModuleArgs } from '..'
+import prisma from '../../prisma'
+
+export const modules = async () => {


You should use typescript for all functions you declare so that you can define what they return

songz · 2022-02-02T20:17:47Z

.env.example

@@ -1,34 +0,0 @@
-#


please undo the deletion of this file. We don't want this file to be deleted

songz · 2022-02-02T20:22:50Z

graphql/queryResolvers/moduleCrud.ts

+}
+
+export const addModule = async (_parent: void, args: MutationAddModuleArgs) => {
+  const { authorId, content, lessonId, name } = args


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

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?

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

anthonykhoa · 2022-02-03T07:38:21Z

Is it a requirement that user MUST be logged in order to do these crud operations for modules? If so, then we should include a check to make sure users are logged in

anso3 added 3 commits January 31, 2022 18:34

adds mod crud

d0803b6

adds modulecrud

c08c7cd

modulecrud

42299ee

vercel bot deployed to Preview January 31, 2022 20:44 View deployment

anthonykhoa reviewed Jan 31, 2022

View reviewed changes

songz reviewed Feb 1, 2022

View reviewed changes

takes module crud comments out

a0f5ab0

vercel bot deployed to Preview February 1, 2022 12:47 View deployment

anthonykhoa reviewed Feb 1, 2022

View reviewed changes

fixes module red and module crud, puts crud into resolvers

a4c31a3

vercel bot deployed to Preview February 2, 2022 13:28 View deployment

songz reviewed Feb 2, 2022

View reviewed changes

.env.example

@@ -1,34 +0,0 @@

#

Copy link

Contributor

songz Feb 2, 2022

Choose a reason for hiding this comment

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

please undo the deletion of this file. We don't want this file to be deleted

songz reviewed Feb 2, 2022

View reviewed changes

anso3 closed this Feb 11, 2022

anso3 deleted the ago branch February 16, 2022 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds module crud and module crud tests #1377

Adds module crud and module crud tests #1377

anso3 commented Jan 31, 2022 •

edited

Loading

vercel bot commented Jan 31, 2022

codecov bot commented Jan 31, 2022 •

edited

Loading

vercel bot commented Jan 31, 2022 •

edited

Loading

anthonykhoa Jan 31, 2022

songz Feb 1, 2022

anthonykhoa Jan 31, 2022

anthonykhoa Jan 31, 2022

songz Feb 1, 2022 •

edited

Loading

anthonykhoa commented Feb 1, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022

anthonykhoa Feb 1, 2022 •

edited

Loading

songz Feb 2, 2022

anthonykhoa Feb 3, 2022

anthonykhoa Feb 1, 2022

songz Feb 2, 2022

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 4, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022

songz Feb 2, 2022

anthonykhoa Feb 3, 2022

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022

anthonykhoa Feb 1, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022

songz Feb 2, 2022

songz Feb 2, 2022

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022

anthonykhoa commented Feb 3, 2022 •

edited

Loading

		@@ -0,0 +1,35 @@
		// import { Context } from '../../@types/helpers'

Adds module crud and module crud tests #1377

Adds module crud and module crud tests #1377

Conversation

anso3 commented Jan 31, 2022 • edited Loading

vercel bot commented Jan 31, 2022

codecov bot commented Jan 31, 2022 • edited Loading

Codecov Report

vercel bot commented Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

songz Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

anthonykhoa commented Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

anthonykhoa Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa Feb 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonykhoa commented Feb 3, 2022 • edited Loading

anso3 commented Jan 31, 2022 •

edited

Loading

codecov bot commented Jan 31, 2022 •

edited

Loading

vercel bot commented Jan 31, 2022 •

edited

Loading

songz Feb 1, 2022 •

edited

Loading

anthonykhoa commented Feb 1, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 4, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa Feb 1, 2022 •

edited

Loading

anthonykhoa Feb 3, 2022 •

edited

Loading

anthonykhoa commented Feb 3, 2022 •

edited

Loading