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

Feature/cgd 51 - Add DB models, and some seed data [part 1] #5

Merged
merged 16 commits into from
Sep 20, 2023

Conversation

cherylli
Copy link
Collaborator

@cherylli cherylli commented Sep 16, 2023

did user, voyage, tech stack stuff.
It would take a while to finish everything, so I would like to merge what I have now, so we can use it for other tasks. I will continue to work on it in the next week or two

prisma migrate dev
prisma db seed

might have to do a prisma migrate reset or prisma db push if you have a old schema locally

One problem is I cannot figure out how to insert a record into the project idea, project stack vote tables programmatically, with just the voyageTeamMemberId. However, the tables seems to be working and I can manually add records using prisma studio

Copy link
Contributor

@alcb1310 alcb1310 left a comment

Choose a reason for hiding this comment

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

There is described one major change that will be to change the database type of the id fields to uuid, other than that, in this change you should modify the README.md file to describe how to load the seed data to the database, and since not everyone has the prisma cli installed globally, you should add it npx prisma migrate dev npx prisma seed, basically most of your PR description should be added to the README.md

prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
prisma/schema.prisma Outdated Show resolved Hide resolved
@alcb1310
Copy link
Contributor

Regarding inserting data programmatically, we can still look at it, but in a worst case scenario, we can always write a TS script to do so and run it as a npm run <script>

@cherylli
Copy link
Collaborator Author

Regarding inserting data programmatically, we can still look at it, but in a worst case scenario, we can always write a TS script to do so and run it as a npm run <script>

yes as I said it's not a huge issue now and it will be same syntax as doing from a TS script anyway, so there would be no difference.

@Dan-Y-Ko
Copy link
Collaborator

Regarding inserting data programmatically, we can still look at it, but in a worst case scenario, we can always write a TS script to do so and run it as a npm run <script>

That is not the issue here.

@alcb1310
Copy link
Contributor

Regarding inserting data programmatically, we can still look at it, but in a worst case scenario, we can always write a TS script to do so and run it as a npm run <script>

That is not the issue here.

That is why I just made a comment and didn't made a conversation to be resolved before merging

@Dan-Y-Ko
Copy link
Collaborator

So after seeing this implemented, I've realized some issues that need to be taken care of. There is a big conflict between what the admins need to be able to handle and what our application requires. There are edge cases that require certain fields to be null on the admin side. So the solution to this is to just allow things to be nullable at the db level, and then we can enforce it to be required at the server level.

@cherylli
Copy link
Collaborator Author

updated all tables to use auto increment (except user)
now seeding for all tables work

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@marktlinn marktlinn left a comment

Choose a reason for hiding this comment

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

The code looks good to me, everything that's been dsicussed is there. I think the bash commands for Prisma need to be prefixed with npx or yarn, other than that looks good to merge to me. Nice work! Thanks for putting your time into getting this all set up 😊🙏

cherylli and others added 2 commits September 20, 2023 08:53
Co-authored-by: Mark Linn <[email protected]>
Co-authored-by: Mark Linn <[email protected]>
@marktlinn marktlinn merged commit ac44fbe into dev Sep 20, 2023
1 check passed
@cherylli cherylli deleted the feature/CGD-51 branch October 19, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants