-
Notifications
You must be signed in to change notification settings - Fork 3
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
Refactor prisma further #1891
Refactor prisma further #1891
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this change. Having a single documents table seems like a win. I put in some notes.
supportingDocumentRevisionID String | ||
contractDocumentContractRevision ContractRevisionTable? @relation(name: "ContractDocumentOnContractRevision", fields: [contractDocumentRevisionID], references: [id]) | ||
supportingDocumentContractRevision ContractRevisionTable? @relation(name: "SupportingDocumentOnContractRevision", fields: [supportingDocumentRevisionID], references: [id]) | ||
display_seq Int? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a bit dodgy to have this be optional, when would we not want to set it? I think all of these things need to have a visual order every time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macrael The reason i didn't set this is we haven't discussed when and how it gets set. I was just adding it in to be used in the future since we discussed it in slack. Is it set in handlers I'm guessing - so on create or update prisma actions? And then we start to orderBy
that in the queries? Let me know if that's right -- I will implement!
sha256 String | ||
contractDocumentRevisionID String | ||
supportingDocumentRevisionID String | ||
contractDocumentContractRevision ContractRevisionTable? @relation(name: "ContractDocumentOnContractRevision", fields: [contractDocumentRevisionID], references: [id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to do it this way, why not have a single documents table?
The reason I suggested we have 4 different tables here was that then the primary key could be required in the database, ensuring that we don't accidentally orphan a document at some point. But I don't think that's that big a deal, it would be nice if we could add a db constraint that made sure that one of the four primary keys were set so you can't make a document that doesn't belong to anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@macrael Gotcha, I am understanding you now. I might be missing something about how relations work let me know. I will keep looking around for a way to implement that type of constraint at the db layer.
@@ -110,32 +109,38 @@ const createRateRevision = ( | |||
supportingDocuments: [ | |||
{ | |||
id: uuidv4(), | |||
rateRevisionID: 'rateRevisionID', | |||
rateDocumentRevisionID: 'rateDocRevisionID', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't a document only ever have one parent ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an artifact of having the named relationship. Here are the docs. What do you think of this? Let me know your thoughts.
Closing this branch since I have changes in flights that will rework these migrations based on feedback - need to start a fresh branch. |
Summary
Two proposed changes to new rates refactor db that have come up via Slack conversations and in person pairing.
display_seq
field.