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: create interface and types for resident #18

Merged
merged 28 commits into from
Nov 18, 2023

Conversation

NandiniMeh
Copy link
Contributor

@NandiniMeh NandiniMeh commented Oct 26, 2023

Notion ticket link

Resident API Backend - CRUD

Implementation description

  • Added graphql/types file for Resident.
  • Created the interface file for Resident.

Steps to test

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@NandiniMeh NandiniMeh marked this pull request as draft October 26, 2023 01:57
@williamtran10
Copy link
Contributor

@ssreekar Can you try testing this from Postman or something.

@ssreekar
Copy link
Contributor

ssreekar commented Nov 6, 2023

So I ran these changes and tested them via Postman. In order to get it to work I made changes to the code base which I migrated. FYI I generally made the simplest change to get the Postman to work rather than the 'smart' solution, because I didn't want to override the design changes you guys made. You can take a look at the latest commit to see the exact change but as an overview:

Made Model Naming Consistent
There are discrepancies between the naming of the GraphQL Schema (in the ResidentsType file) and the Prisma model columns, which gave me errors when running the code. For this PR and testing I changed all to snake case (to avoid touching the predefined model) but personally I think it makes sense to just use CamelCase for everything since that is what the entity service does in the backend. This is not my decision to make so feel free to change the names however you like but the names need to be consistent otherwise the code won't run.

I also changed DateTime and Date to String in the GraphQL Schema, as they support Date or DateTime. I also added a TODO to fix this in the future where we can either use third party handlers or manually handle them in the backend.

Discrepancies between optional types in the Prisma model and the optional types in the Schema Model.
For example date_joined is marked at optional in the createResidentDTO but not in the resident prisma model, meaning you could input an add resident request with null data which will cause an error on the backend (as the date_joined is null) this should ideally be consistent so I changed the residentType to follow the resident model as I assumed that is the priority, but I'm not really sure what your design choices are so feel free to remove mandatory fields that shouldn't be mandatory

ID's from GraphQL Schema are considered strings but on Prisma side they are considered ints so I just added a parseInt()
in the middle (alternatively we could just make the GraphQL schema use int instead of ID, but up to you guys)

Changed output of DeleteResident to ResidentDTO instead of ID (i think this was a typo but not 100% sure)

Changed input of schemaType for residentByID to [ID!] and made both return types for residentsById and allResidents
[ResidentDTO!] (as apposed to just one item) (Again probably just a typo as the code returns arrays, but not 100% sure)

Other than that the PR looks great and works when tested 😄 Most of the changes I made had to do with getting the code simply running as you guys had no way to test your code, but once it started working everything worked great.

@williamtran10 Thoughts? (Specifically wanted your opinion on snake vs camel casing since that impacts the entire data model) and whether we should use third party date time types vs backend string checks)

@williamtran10
Copy link
Contributor

@ssreekar

Re: Case naming consistency
I would prefer to have the DTO fields to use camelCase. The Prisma models use snake_case because of the Postgres convention of naming columns using snake_case. However, I'm okay with breaking convention here to change everything to camelCase. Alternatively, we can map camelCase Prisma fields to snake_case Postgres columns.
Let me know your thoughts on this.

Re: DateTime
I'll defer to you for a decision on using a third party handler or handling them ourself or keeping it as a string.

Re: IDs as ints
I think we should keep using the GraphQL ID, and just parse it to an int as soon as it gets to the backend. See this stack overflow for more details.

@ssreekar
Copy link
Contributor

@williamtran10
Yeah if Postgres convention is snake_case then we should stick with that and DTO's should be camel case to be consistent with the previous blueprint starter code api's. I think using the mapper makes a lot of sense since the old api's manually map between the camel and snake case, so the Prisma mapper you linked is just a cleaner solution to that problem.

@selena1108
Copy link
Contributor

So everything works up till commit 331d0ff. Then I tried to get the changes from dev with a rebase, led to resolving a conflict with the dev branch and I don't think I rebased things correctly, now it doesn't build on my end and I can't figure out how to fix it. @NandiniMeh can you please have a look?

williamtran10 and others added 2 commits November 15, 2023 20:31
* feat: add neon db

* fix: add optional neon setup
@williamtran10 williamtran10 force-pushed the nandini-selena/resident-api branch from 321ee19 to 01940de Compare November 16, 2023 01:32
@selena1108 selena1108 marked this pull request as ready for review November 16, 2023 01:57
Copy link
Contributor

@williamtran10 williamtran10 left a comment

Choose a reason for hiding this comment

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

@NandiniMeh @selena1108 Can you double check that this works? I changed the name mapping and linting. Added some comments too.

package-lock.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@ssreekar ssreekar left a comment

Choose a reason for hiding this comment

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

I left some small comments regarding casing and references

@ssreekar
Copy link
Contributor

I added some changes regarding the types of the resident graph ql schema to be camelCase and some other minor changes to get the apis to run via postman. I also added graphql scalars third party Library so we can now use Datetime and Date time in the graph ql schema file. I'm not 100% sure how exactly the model should work with Date and DateTime, so feel free to update the resdient schema types so that they make more sense.

Copy link
Contributor

@williamtran10 williamtran10 left a comment

Choose a reason for hiding this comment

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

LGTM, squash and merge when you're ready.

@NandiniMeh NandiniMeh merged commit 7841fb1 into dev Nov 18, 2023
1 check passed
@NandiniMeh NandiniMeh deleted the nandini-selena/resident-api branch November 18, 2023 17:13
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.

5 participants