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

Add CRUD functions for models via Prisma #228

Merged
merged 1 commit into from
Feb 17, 2023

Conversation

cychu42
Copy link
Contributor

@cychu42 cychu42 commented Feb 15, 2023

Relates to #97

Changes:

  • This PR adds CRUD functions for models via Prisma, for model: User, Record, Certificate, Challenge.
  • deleteUserByEmail() changes to deleteUserByUsername()

Test:

  1. You can try to use the functions to see if they function correctly as CRUD operations.
  • Option 1: You can use them in your code.
  • Option 2: You can make a query file to import and execute these functions.
    NOTE: npx ts-node needs to have --require tsconfig-paths/register or it won't understand ~ for paths( to db.bserver and logger.server) in our project.
  1. npm run db:studio can let you view data in database after an operation.

@cychu42 cychu42 added this to the Milestone 0.3 milestone Feb 15, 2023
@cychu42 cychu42 added the category: data Anything related to data management and structure label Feb 15, 2023
app/models/certificate.server.ts Outdated Show resolved Hide resolved
app/models/certificate.server.ts Show resolved Hide resolved
@cychu42
Copy link
Contributor Author

cychu42 commented Feb 15, 2023

I tried to rebase my topic branch because there was some issue with history not matching, when I attempted to make changes from feedback.

@Eakam1007
Copy link
Contributor

Eakam1007 commented Feb 15, 2023

I tried to rebase my topic branch because there was some issue with history not matching, when I attempted to make changes from feedback.

I think a bunch of other (not your) changes are showing up after the rebase. This is most likely because you did not force push after the rebase.
You could try rebasing with main, and then force pushing

* For model: User, Record, Certificate, Challenge
* deleteUserByEmail() changes to deleteUserByUsername
@cychu42
Copy link
Contributor Author

cychu42 commented Feb 15, 2023

Yeah, I accidently synced the wrong branch to upstream's main, which caused the history mismatch.
Then I tried to rebase without realizing that, and it caused the messy rebase.
I reverted the history and redid it properly with force push, so it's fine now.

@cychu42 cychu42 requested review from Ririio and Myrfion February 17, 2023 13:56
Copy link
Contributor

@Ririio Ririio left a comment

Choose a reason for hiding this comment

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

I only have a nitpick which is the use of id, but otherwise the pr looks great!

@cychu42
Copy link
Contributor Author

cychu42 commented Feb 17, 2023

I hear you. I still think it's more useful to use username, and we can talk about potentially making it the PK maybe in a follow up issue.

@Eakam1007 Eakam1007 self-requested a review February 17, 2023 17:38
Copy link
Contributor

@Eakam1007 Eakam1007 left a comment

Choose a reason for hiding this comment

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

The comment for createCertificate might also apply on createChallenge and createRecord though I have only tested it for createCertificate

app/models/certificate.server.ts Show resolved Hide resolved
@cychu42 cychu42 requested a review from Eakam1007 February 17, 2023 19:16
Copy link
Contributor

@Eakam1007 Eakam1007 left a comment

Choose a reason for hiding this comment

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

I tested all these methods locally; looks great!

Can you file an issue to add test for this code?

@cychu42
Copy link
Contributor Author

cychu42 commented Feb 17, 2023

Sure.
Thank you all for the review.
@humphd Can you help with the ESLint issue?

@humphd humphd merged commit cfb442e into DevelopingSpace:main Feb 17, 2023
@humphd
Copy link
Contributor

humphd commented Feb 17, 2023

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: data Anything related to data management and structure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants