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

Get exercises from backend on DOJO exercises page #2335

Merged
merged 4 commits into from
Sep 21, 2022
Merged

Get exercises from backend on DOJO exercises page #2335

merged 4 commits into from
Sep 21, 2022

Conversation

bryanjenningz
Copy link
Collaborator

@bryanjenningz bryanjenningz commented Sep 20, 2022

This pull request doesn't affect any user-facing pages since there are no direct links to the DOJO exercises page.

This pull request makes it so the DOJO exercises page gets the exercises from the backend instead of relying on mock data on the frontend.

How to test:

  • Create a module at /admin/lessons/js0/modules
  • Create exercises at /curriculum/js0/mentor
  • Go to /exercises/js0 and click on the "SOLVE EXERCISES" button on the right and verify the exercises still work

The exercise page currently doesn't send any mutations to the backend. We will add mutations to update the user's answers to the exercises on the backend in a subsequent pull request.

This pull request is a part of #2253

@vercel
Copy link

vercel bot commented Sep 20, 2022

@bryanjenningz is attempting to deploy a commit to the c0d3-prod Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Sep 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
c0d3-app ✅ Ready (Inspect) Visit Preview Sep 20, 2022 at 11:34PM (UTC)

challengeName: exercise?.module.name!,
problem: exercise?.description!,
answer: exercise?.answer!,
explanation: exercise?.explanation!
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using the ?. and ! operators here isn't ideal.

The reason why we need to do this is because the type definition for the exercises is exercises: [Exercise]! instead of exercises: [Exercise!]!: https://github.com/garageScript/c0d3-app/blob/master/graphql/typeDefs.ts#L9

Would it be better to change the schema to exercises: [Exercise!]! so that we don't have to worry about null values?

Copy link
Member

Choose a reason for hiding this comment

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

I thought we should do that once there are some exercises. Wouldn't it throw an error when getting the exercises if there are none?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we changed the type definition to exercises: [Exercise!]! and if there are no exercises (i.e. it returns an empty array []), then it shouldn't throw an error because [] is the type [Exercise!]!.

It would only throw an error if there were null values in the array (e.g. [null, null]).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a pull request for if we want to make this change: #2336

@codecov
Copy link

codecov bot commented Sep 20, 2022

Codecov Report

Merging #2335 (3be8b47) into master (bd96a63) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2335   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          180       181    +1     
  Lines         3177      3179    +2     
  Branches       850       850           
=========================================
+ Hits          3177      3179    +2     
Impacted Files Coverage Δ
__dummy__/getExercisesData.ts 100.00% <100.00%> (ø)
pages/exercises/[lessonSlug].tsx 100.00% <100.00%> (ø)

@SlyBouhafs
Copy link
Member

SlyBouhafs commented Sep 20, 2022

The graphql/index.tsx file has the same diff in this PR: https://github.com/garageScript/c0d3-app/pull/2330/files

@bryanjenningz
Copy link
Collaborator Author

@SlyBouhafs

This file graphql/index.tsx has the same diff as the file in this PR: https://github.com/garageScript/c0d3-app/pull/2330/files

Do you want me to close that pull request since it's overlapping this one or do you want me to merge that pull request so that this pull request has a smaller diff?

@@ -0,0 +1,31 @@
import { gql } from '@apollo/client'

const GET_EXERCISES = gql`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure what to call this query. It does more than just get the exercises - it gets the lessons and alerts too which are used on the DOJO exercises page.

Copy link
Member

Choose a reason for hiding this comment

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

GET_DOJO_EXERCISES maybe, but it's longer.

Copy link
Collaborator Author

@bryanjenningz bryanjenningz Sep 21, 2022

Choose a reason for hiding this comment

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

Yeah, or maybe even something like GET_DOJO_EXERCISES_PAGE_DATA since we're getting all the data we need for the DOJO exercises page. Maybe GET_DOJO_EXERCISES is fine. Maybe we should also change GET_FLAGGED_EXERCISES to be GET_FLAGGED_DOJO_EXERCISES so that the naming convention is consistent.

@SlyBouhafs
Copy link
Member

SlyBouhafs commented Sep 20, 2022

Do you want me to close that pull request since it's overlapping this one or do you want me to merge that pull request so that this pull request has a smaller diff?

I think it's better to keep this file out of this PR.

@bryanjenningz
Copy link
Collaborator Author

Do you want me to close that pull request since it's overlapping this one or do you want me to merge that pull request so that this pull request has a smaller diff?

I think it's better to keep this file out of this PR.

I also added a new query in this pull request which changed this file as well. Should I move the new query I added to a separate pull request so that this file diff is not in this pull request?

@SlyBouhafs
Copy link
Member

I also added a new query in this pull request which changed this file as well. Should I move the new query I added to a separate pull request so that this file diff is not in this pull request?

It's better if we merge the other PR, then have a new diff with that change only.

@bryanjenningz
Copy link
Collaborator Author

It's better if we merge the other PR, then have a new diff with that change only.

Sounds good.

@flacial
Copy link
Member

flacial commented Sep 21, 2022

This is unrelated to this PR; I just noticed it.

Doesn't it make sense for the input title to be Answer or Your answer rather than the module name?

image

Copy link
Member

@flacial flacial left a comment

Choose a reason for hiding this comment

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

Reapproving.

Super nice, I like it

image

@bryanjenningz
Copy link
Collaborator Author

This is unrelated to this PR; I just noticed it.

Doesn't it make sense for the input title to be Answer or Your answer rather than the module name?

image

Yeah, I actually agree with you on this. I was following the design, but maybe we should change the design so it says "Your answer" or something similar instead.

@bryanjenningz
Copy link
Collaborator Author

bryanjenningz commented Sep 21, 2022

I'll merge this pull request since it's mostly good and everything works. We can fix the exercises type definition to be [Exercise!]! in a separate pull request which will make things more simple and we can also possibly handle changing the naming convention of the DOJO exercises queries in a separate pull request too. We can talk about the design and see if we want to show "Your answer" or something similar instead of the module name on each exercise.

@bryanjenningz bryanjenningz merged commit 27b9d46 into garageScript:master Sep 21, 2022
@bryanjenningz bryanjenningz deleted the get-dojo-exercises-from-backend branch September 21, 2022 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants