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

feat(apollo): Create removeExercise resolver #2657

Conversation

flacial
Copy link
Member

@flacial flacial commented Dec 29, 2022

Motivation

Currently, there is no way to set the removed flag on an exercise when an admin wants to remove it. The removed flag is a field in the database that indicates whether an exercise has been removed or not.

The PR is adding a resolver that will allow an admin to set the removed flag to true when they want to remove an exercise. This will mark the exercise as removed in the database, and it will no longer be visible to users.

Steps taken

  1. Created the resolver removeExercise
  2. Ensure everything is working as intended by adding tests for the resolver
  3. Created the query file for removeExercise resolver so it can be easily used in the client-side
  4. Update GraphQL index.ts with the new resolver and hooks.

Unrelated steps

  • Fixes some typos

Resolver logic

  1. It receives one argument, the user id.
  2. It fetches the exercise to be removed from the database using the provided id.
  3. It retrieves the id of the user making the request (authorId) from the request context (req).
  4. If the user is not an admin and the authorId does not match the authorId of the exercise being removed, the resolver throws an error, indicating that the user is not authorized to remove the exercise.
  5. If the exercise is already removed, the resolver throws an error, indicating that exercise is already flagged.
  6. If the user is authorized to remove the exercise, the resolver updates the exercise in the database, setting the removedAt and removedById fields to new Date & the user ID. It also includes the author and module relations in the update.
  7. The resolver returns the updated exercise.

Testing results

Executes removeExercise:

mutation {
  removeExercise(id: 1) {
    id
  }
}

Result:

{
  "data": {
    "removeExercise": {
      "id": 1,
      "removedAt": "1672591865633",
      "removedById": 1
    }
  }
}

Result if it's already removed:

{
  "errors": [
    {
      "message": "Exercise is already removed",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
  "data": null
}

Result if the exercise is not found:

{
  "errors": [
    {
      "message": "Exercise is not found",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ]
  ],
  "data": null
}

Testing instructions

  1. Go to /api/graphql
  2. Run the queries from Testing result section
  3. Make sure they return the correct results

Related issues

@vercel
Copy link

vercel bot commented Dec 29, 2022

@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 Dec 29, 2022

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

Name Status Preview Updated
c0d3-app ✅ Ready (Inspect) Visit Preview Jan 1, 2023 at 5:40PM (UTC)

@codecov
Copy link

codecov bot commented Dec 29, 2022

Codecov Report

Merging #2657 (7ec7c36) into master (cd8d8a5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2657   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          189       189           
  Lines         3491      3504   +13     
  Branches       966       969    +3     
=========================================
+ Hits          3491      3504   +13     
Impacted Files Coverage Δ
graphql/resolvers/exerciseCrud.ts 100.00% <100.00%> (ø)

@flacial flacial self-assigned this Dec 29, 2022
@JasirZaeem
Copy link
Member

JasirZaeem commented Dec 31, 2022

This is out of the scope of this particular PR but related, maybe removed should be removedAt/removed_at as well like some other booleans we have changed to date | null before.
Like flagged at and the user who flagged it, it would make sense for removed to be like this too with removed at and removed by data.
And even if removed remains a boolean it should not be an optional field, it should either be required with false as default so only two values are possible, or it should be a date field with default value being date. Currently removed can hold three values null, false, true.
My suggestion would be to refactor the schema for exercise and improve it with regards to remove before this PR.

@flacial
Copy link
Member Author

flacial commented Dec 31, 2022

This is out of the scope of this particular PR but related, maybe removed should be removedAt/removed_at as well like some other booleans we have changed to date | null before. Like flagged at and the user who flagged it, it would make sense for removed to be like this too with removed at and removed by data. And even if removed remains a boolean it should not be an optional field, it should either be required with false as default so only two values are possible, or it should be a date field with default value being date. Currently removed can hold three values null, false, true. My suggestion would be to refactor the schema for exercise and improve it with regards to remove before this PR.

I agree. It makes sense.

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