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

Replace deleteExercise with removeExercise resolver in the client #2668

Conversation

flacial
Copy link
Member

@flacial flacial commented Jan 4, 2023

Motivation

After adding the removeExercise resolver that aims at correcting our initial implementation for removing exercises, we need to use it in the client-side now.

Steps taken

  1. Replace deleteExercise with removeExercise in mentor page
  2. Replace deleteExercise with removeExercise in admin/lessons page
  3. Replace deleteExercise with removeExercise in AdminLessonExerciseCard component
  4. Replace deleteExercise with removeExercise in exercises page
  5. Update the tests queries mock to reflect the new changes
  6. Prevent removed exercises from showing to the students

Testing

  1. Go to /exercises/js0
  2. Click SOLVE EXERCISES
  3. Flag the exercise
  4. Go to /admin/lessons/js0/exercises
  5. Remove the exercise by clicking the Remove button
  6. Refresh the page and it shouldn't be displayed
  7. Go to /exercises/js0 again
  8. Notice how the removed exercise isn't appear
  9. Go to /curriculum/js0/mentor
  10. Click any exercise dropdown button and click Delete button to remove the exercise
  11. Go to /exercises/js0 and notice how the exercise isn't showing

Related issues

…ed-exercises-in-accordance-with-design-doc-1
…rciseCard

This commit update the pages to use removeExercise and add conditions to not display removed exercise. In the future, we should let the backend handle this so the app don't consume a lot of the user bandwidth.
This commit update the pages to use removeExercise and add conditions to not display removed exercise. In the future, we should let the backend handle this so the app don't consume a lot of the user bandwidth.
This commit update the pages to use removeExercise and add conditions to not display removed exercise. In the future, we should let the backend handle this so the app don't consume a lot of the user bandwidth.
This commit update the pages to use removeExercise and add conditions to not display removed exercise. In the future, we should let the backend handle this so the app don't consume a lot of the user bandwidth.
@vercel
Copy link

vercel bot commented Jan 4, 2023

@flacial 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 Jan 4, 2023

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

Name Status Preview Updated
c0d3-app ✅ Ready (Inspect) Visit Preview Jan 5, 2023 at 3:26AM (UTC)

@flacial
Copy link
Member Author

flacial commented Jan 4, 2023

I accidentally checked-out from a non-master branch, this is why there are some reverted commits.

@@ -4,7 +4,7 @@ import React, { useState } from 'react'
import { Collapse, Spinner } from 'react-bootstrap'
Copy link
Member Author

Choose a reason for hiding this comment

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

For this component, I noticed there's no error handling if the exercise wasn't removed/unflagged successfully.

@@ -193,7 +193,9 @@ const ExercisesPage = ({ lessonSlug }: ExercisesProps) => {
const mapExercisesToExerciseCard = data?.exercises
.filter(
exercise =>
exercise.flaggedAt && exercise.module.lesson.slug === lessonSlug
exercise.flaggedAt &&
!exercise.removedAt &&
Copy link
Member Author

Choose a reason for hiding this comment

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

I added a condition to prevent the display of any removed exercises. Sending removed exercises to the client would be a waste of bandwidth, as they will not be displayed. Instead, it would be more efficient to have the backend filter out removed exercises and only send the ones that should be displayed to the client.

Copy link
Member

Choose a reason for hiding this comment

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

Not only is it more efficient, it is the correct way. If the user is not supposed to see some data/be able to access it/perform some action, merely hiding it on the frontend is never enough, it should not be available at the client in the first place and backend should ensure that.

@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #2668 (53501b3) into master (805306c) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 53501b3 differs from pull request most recent head cb6f83f. Consider uploading reports for the commit cb6f83f to get more accurate results

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2668   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          189       189           
  Lines         3510      3507    -3     
  Branches       970       973    +3     
=========================================
- Hits          3510      3507    -3     
Impacted Files Coverage Δ
...onents/ExercisePreviewCard/ExercisePreviewCard.tsx 100.00% <100.00%> (ø)
...mponents/admin/lessons/AdminLessonExerciseCard.tsx 100.00% <100.00%> (ø)
...es/admin/lessons/[lessonSlug]/[pageName]/index.tsx 100.00% <100.00%> (ø)
pages/curriculum/[lessonSlug]/mentor/index.tsx 100.00% <100.00%> (ø)
pages/exercises/[lessonSlug].tsx 100.00% <100.00%> (ø)
pages/discord/success.tsx 100.00% <0.00%> (ø)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🦄 Done
Development

Successfully merging this pull request may close these issues.

2 participants