-
-
Notifications
You must be signed in to change notification settings - Fork 359
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.
Typescript nitpicking 😝
@JoinColumn({ name: 'user_id' }) | ||
user!: User; | ||
|
||
constructor(params: { userId: number; roleName: InstanceRoles }) { |
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 shouldn't be required if you have if (params)
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.
The if (params)
is required for the migration; errors without them (apparently the constructor is called with no arguments?). Ideally we keep it so that the params is not optional so people writing actual TS code don't think it's optional when it's not.
@JoinColumn({ name: 'chapter_id' }) | ||
chapter!: Chapter; | ||
|
||
constructor(params: { |
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.
Same as the other one, make params?:
@timmyichen do we also need to generate a data/ddl.sql and the associated ER Diagram? I installed DBeaver and it looks like you need a real database for it to create an ERD. That is, it doesn't look like you can paste in the DDL and get a diagram. I haven't ventured into installing Docker and I suspect it would take me awhile to get DBeaver talking to Postgres inside of the container, so I figured to ask. We previously used an online tool for the ERD, but people liked DBeaver better and there was a bug in the tool we were using which didn't support one of the psql date types. So, that's how we ended up on DBeaver. |
@allella we don't need to, but we can if you think it'll be helpful. Either way I think it's best if not done in this PR as we don't want that to be blocking other people developing the other API routes. |
@timmyichen that's fine. I was somewhat wondering if the ddl.sql was generated automatically. I think that would be nice and then someone can update the E-R Diagram since it's been a manual process. |
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 not giving this a technical review, but the table names and roles look good.
@timmyichen Thanks! Merging 🎉 @allella IMO What do you say? |
I found the ddl.sql useful and it was helpful to create an ER-Diagram on dbdiagram.io but we did move to DBeaver since there was a timestamptz issue with dbdiagram.io. I've re-opened #54 because for documentation we need to at least update it. If the ddl.sql can be updated automatically then I'd say to keep it around too. I've pinged the original creators of both files on #54 |
Update README.md
).master
branch of Chapter.Closes #296
Adds the role tables for chapter roles (member, organizer) and instance roles (admin, owner).