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

Replace deleteExercise with removeExercise resolver in the client #2668

Merged
merged 9 commits into from
Jan 5, 2023
Merged

Replace deleteExercise with removeExercise resolver in the client #2668

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2cb3744
Merge remote-tracking branch 'upstream/master' into 2630-remove-flagg…
flacial Jan 4, 2023
01b1279
Replace deleteExercise with removeExercise resolver in AdminLessonExe…
flacial Jan 4, 2023
1dd05ce
Replace deleteExercise with removeExercise resolver in mentor page
flacial Jan 4, 2023
ad8bb88
admin/lessons page
flacial Jan 4, 2023
5a52746
Replace deleteExercise with removeExercise resolver in Exercises page
flacial Jan 4, 2023
3cc4a58
Revert "refactor: Use consistent rendering conditions"
flacial Jan 4, 2023
144384a
Revert "feat: Display a message for when Discord is already used"
flacial Jan 4, 2023
53501b3
Merge branch 'master' into 2630-remove-flagged-exercises-in-accordanc…
flacial Jan 5, 2023
cb6f83f
Merge branch 'master' into 2630-remove-flagged-exercises-in-accordanc…
flacial Jan 5, 2023
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
10 changes: 5 additions & 5 deletions __tests__/pages/admin/lessons/[lessonSlug]/[pageName]/lessons.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { CHALLENGES } from '../../../../../../graphql/queries/challenges'
import ADD_CHALLENGE from '../../../../../../graphql/queries/createChallenge'
import GET_EXERCISES from '../../../../../../graphql/queries/getExercises'
import REMOVE_EXERCISE_FLAG from '../../../../../../graphql/queries/removeExerciseFlag'
import DELETE_EXERCISE from '../../../../../../graphql/queries/deleteExercise'
import REMOVE_EXERCISE from '../../../../../../graphql/queries/removeExercise'
import { MockedProvider } from '@apollo/client/testing'
import userEvent from '@testing-library/user-event'
import UPDATE_LESSON from '../../../../../../graphql/queries/updateLesson'
Expand Down Expand Up @@ -194,14 +194,14 @@ const getExerciseWithRefetchMock = {
}
}

const deleteExerciseMock = {
const removeExerciseMock = {
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: { id: 1 }
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
Expand Down Expand Up @@ -280,7 +280,7 @@ const mocksWithRefetchExercises = [
getAppQueryMock,
getAppQueryMock,
getExerciseWithRefetchMock,
deleteExerciseMock,
removeExerciseMock,
unflagExerciseMock,
challengesQueryMock,
addChallengeQueryMock
Expand Down
18 changes: 9 additions & 9 deletions __tests__/pages/curriculum/[lessonSlug]/mentor/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { useRouter } from 'next/router'
import { MockedProvider } from '@apollo/client/testing'
import getExercisesData from '../../../../../__dummy__/getExercisesData'
import GET_EXERCISES from '../../../../../graphql/queries/getExercises'
import DELETE_EXERCISE from '../../../../../graphql/queries/deleteExercise'
import REMOVE_EXERCISE from '../../../../../graphql/queries/removeExercise'
import GET_SESSION from '../../../../../graphql/queries/getSession'
import dummySessionData from '../../../../../__dummy__/sessionData'
import userEvent from '@testing-library/user-event'
Expand Down Expand Up @@ -81,7 +81,7 @@ describe('Mentor page', () => {
expect(await screen.findByText('500 Error!')).toBeInTheDocument()
})

test('Should delete exercise', async () => {
test('Should remove exercise', async () => {
expect.assertions(1)

const mocks = [
Expand Down Expand Up @@ -109,21 +109,21 @@ describe('Mentor page', () => {
},
{
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: {
id: 1
}
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
},
newData: jest.fn(() => ({
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
Expand All @@ -149,7 +149,7 @@ describe('Mentor page', () => {
expect(mocks.at(-1).newData).toBeCalled()
})

test('Should delete exercise item and its QUERY INFO', async () => {
test('Should remove exercise item and its QUERY INFO', async () => {
expect.assertions(1)

const mocks = [
Expand Down Expand Up @@ -177,21 +177,21 @@ describe('Mentor page', () => {
},
{
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: {
id: 1
}
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
},
newData: jest.fn(() => ({
data: {
deleteExercise: {
removeExercise: {
id: 2
}
}
Expand Down
9 changes: 5 additions & 4 deletions components/ExercisePreviewCard/ExercisePreviewCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { KebabHorizontalIcon } from '@primer/octicons-react'
import Markdown from 'markdown-to-jsx'
import React from 'react'
import { useDeleteExerciseMutation } from '../../graphql'
import { useRemoveExerciseMutation } from '../../graphql'
import { DropdownMenu } from '../DropdownMenu'
import QueryInfo from '../QueryInfo'
import styles from './exercisePreviewCard.module.scss'
Expand All @@ -23,7 +23,8 @@ const ExercisePreviewCard = ({
onDelete,
className = ''
}: ExercisePreviewCardProps) => {
const [deleteExercise, { data, loading, error }] = useDeleteExerciseMutation()
const [removedExercise, { data, loading, error }] =
useRemoveExerciseMutation()

const [topBorderStyle, topMessageStyle] =
state === 'ANSWERED'
Expand All @@ -42,7 +43,7 @@ const ExercisePreviewCard = ({
>
{state && <div className={topBorderStyle} />}
<div className={styles.header__container}>
{data && data.deleteExercise.id !== id ? (
{data && data.removeExercise.id !== id ? (
<></>
) : (
<QueryInfo
Expand All @@ -69,7 +70,7 @@ const ExercisePreviewCard = ({
{
title: 'Delete',
onClick: async () => {
await deleteExercise({
await removedExercise({
variables: {
id
}
Expand Down
14 changes: 7 additions & 7 deletions components/admin/lessons/AdminLessonExerciseCard.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import Component from './AdminLessonExerciseCard'
import userEvent from '@testing-library/user-event'
import { render, screen } from '@testing-library/react'
import { MockedProvider } from '@apollo/client/testing'
import DELETE_EXERCISE from '../../../graphql/queries/deleteExercise'
import REMOVE_EXERCISE from '../../../graphql/queries/removeExercise'
import REMOVE_EXERCISE_FLAG from '../../../graphql/queries/removeExerciseFlag'

// Imported to be able to use expect(...).toBeInTheDocument()
Expand Down Expand Up @@ -31,12 +31,12 @@ const exercise = {
const mocks = [
{
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: { id: 1 }
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
Expand All @@ -62,12 +62,12 @@ const mocks = [
const loadingMocks = [
{
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: { id: 1 }
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: 1
}
}
Expand Down Expand Up @@ -120,12 +120,12 @@ describe('AdminLessonExerciseCard component', () => {
mocks={[
{
request: {
query: DELETE_EXERCISE,
query: REMOVE_EXERCISE,
variables: { id: 1 }
},
result: {
data: {
deleteExercise: {
removeExercise: {
id: null
}
}
Expand Down
18 changes: 9 additions & 9 deletions components/admin/lessons/AdminLessonExerciseCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

import { PROFILE_PATH } from '../../../constants'
import {
useDeleteExerciseMutation,
useRemoveExerciseMutation,
useRemoveExerciseFlagMutation,
Exercise,
User
Expand Down Expand Up @@ -95,23 +95,23 @@ type FooterProps = {
onUnflag?: (id: number) => void
}
const Footer = ({ exercise, onRemove, onUnflag }: FooterProps) => {
const [deleteExercise, { loading: deleteLoading }] =
useDeleteExerciseMutation()
const [removeExercise, { loading: removeLoading }] =
useRemoveExerciseMutation()
const [unflagExercise, { loading: unflagLoading }] =
useRemoveExerciseFlagMutation()

const handleRemove = async () => {
const deletedExercise = await deleteExercise({
const removedExercise = await removeExercise({
variables: {
id: exercise.id
}
})

if (onRemove) {
const deletedExerciseId = deletedExercise.data?.deleteExercise.id
const removedExerciseId = removedExercise.data?.removeExercise.id

if (deletedExerciseId) {
onRemove(deletedExerciseId)
if (removedExerciseId) {
onRemove(removedExerciseId)
}
}
}
Expand Down Expand Up @@ -146,10 +146,10 @@ const Footer = ({ exercise, onRemove, onUnflag }: FooterProps) => {
</button>
<button
className={`${styles.card__footer__btn} ${styles.card__footer__btn__remove}`}
disabled={deleteLoading}
disabled={removeLoading}
onClick={handleRemove}
>
{deleteLoading ? (
{removeLoading ? (
<Spinner size="sm" animation="border" />
) : (
<span>REMOVE EXERCISE</span>
Expand Down
4 changes: 3 additions & 1 deletion pages/admin/lessons/[lessonSlug]/[pageName]/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

exercise.module.lesson.slug === lessonSlug
)
.map(exercise => {
return (
Expand Down
3 changes: 2 additions & 1 deletion pages/curriculum/[lessonSlug]/mentor/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ const MentorPage = () => {
.filter(
exercise =>
exercise.module.lesson.slug === slug &&
exercise.author.id === sessionUser?.id
exercise.author.id === sessionUser?.id &&
!exercise.removedAt
)
.map(exercise => ({
id: exercise.id,
Expand Down
8 changes: 6 additions & 2 deletions pages/exercises/[lessonSlug].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ const Exercises: React.FC<QueryDataProps<GetExercisesQuery>> = ({
return <Error code={StatusCode.NOT_FOUND} message="Lesson not found" />

const currentExercises = exercises
.filter(exercise => exercise?.module.lesson.slug === slug)
.filter(
exercise => exercise?.module.lesson.slug === slug && !exercise.removedAt
)
.map(exercise => {
const userAnswer = userAnswers[exercise.id] ?? null
return {
Expand All @@ -73,7 +75,8 @@ const Exercises: React.FC<QueryDataProps<GetExercisesQuery>> = ({
if (userAnswer === exercise.answer) return 'ANSWERED'
if (userAnswer) return 'INCORRECT'
return 'NOT ANSWERED'
})()
})(),
removedAt: exercise.removedAt
}
})
.filter(
Expand Down Expand Up @@ -227,6 +230,7 @@ type ExerciseListItem = {
userAnswer: string | null
state: ExercisePreviewCardProps['state']
id: number
removedAt?: string | null
}

type ExerciseListProps = {
Expand Down