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

Remove removed, adds removedAt and removedBy fields to Exercise model #2660

Conversation

flacial
Copy link
Member

@flacial flacial commented Jan 1, 2023

Motivation

In a recent comment, it was addressed that the fields for handling removing an exercise could be improved, and this PR reflects the suggested changes.

Changes

This PR updates the Exercise model to improve how the removed field is handled.

The changes include:

  1. Renaming removed to removedAt to match the naming conventions of other date fields in the model.
  2. Making removedAt a required field with a default value of null. This ensures that there are only two possible values for the field: null or a date.
  3. Adding a new field called removedBy to track the user who removed the exercise.
  4. Removing the removed field.

These changes will make it easier to track when an exercise is removed and who removed it, and will improve the overall consistency of the model.

Related issues

@vercel
Copy link

vercel bot commented Jan 1, 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 1, 2023

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 4:21PM (UTC)

@codecov
Copy link

codecov bot commented Jan 1, 2023

Codecov Report

Merging #2660 (c81384b) into master (fa4614b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #2660   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          189       189           
  Lines         3491      3491           
  Branches       966       966           
=========================================
  Hits          3491      3491           
Impacted Files Coverage Δ
__dummy__/getExercisesData.ts 100.00% <ø> (ø)

@@ -187,7 +188,9 @@ model Exercise {
answer String
testStr String?
explanation String?
removed Boolean? @default(false)
removedAt DateTime?
Copy link
Member Author

Choose a reason for hiding this comment

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

It's only possible to set removedAt default value to a DateTime, and in the client, we will use it like the following:

if (exercise.removedAt) // do something

If it has a default, value, the above condition will be true, and we'd get an opposite behavior for what we want.

Copy link
Member

Choose a reason for hiding this comment

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

If you don't set a default value postgresql will set it to null so we don't need to explicitly set the default to null in prisma, postgresql will do it and we'll get the desired behaviour. Being limited to datetime as a default value is due to prisma, prisma doesn't let you set null as the default for an optional value but that is the default behaviour.
Just adding removedAt DateTime? like you have done is enough.

removed Boolean? @default(false)
removedAt DateTime?
removedById Int?
removedBy User? @relation("removedExercises", fields: [removedById], references: [id])
Copy link
Member Author

Choose a reason for hiding this comment

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

Something I discovered during testing the changes with Apollo playground: removedBy and flaggedBy returns null because they refer to a different model. In order to include them in the result of querying the db for exercises, we'd have to explicitly tell Prisma to include them:

export const exercises = () => {
  return prisma.exercise.findMany({
    include: {
      author: true,
+     flaggedBy: true,
+     removedBy: true,
      module: {
        include: { lesson: true }
      }
    }
  })
}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, getting the related fields requires a sql JOIN so prisma will not do the extra work unless needed and requested by us.

@flacial flacial changed the title Add removedAt, removedBy, and removedAt fields to Exercise model Remove removed, adds removedAt, removedBy, and removedAt fields to Exercise model Jan 1, 2023
@flacial flacial merged commit cd8d8a5 into garageScript:master Jan 1, 2023
@flacial flacial changed the title Remove removed, adds removedAt, removedBy, and removedAt fields to Exercise model Remove removed, adds removedAt and removedBy fields to Exercise model Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🦄 Done
Development

Successfully merging this pull request may close these issues.

2 participants