Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

[WIP] fix(db, api): update database model & API schemas #253

Merged
merged 2 commits into from
Dec 15, 2019

Conversation

vkWeb
Copy link
Member

@vkWeb vkWeb commented Nov 18, 2019

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the master branch of Chapter.

This is not ready for review. It's a work in progress. I am updating the database & API schemas to reflect the recent decisions we made:

  • Allow multiple tags per event.
  • Adding admin as a user role. So now we have three user roles: participants (normal visitors), organizers & admin(s).

Closes #254.

@vkWeb vkWeb mentioned this pull request Nov 18, 2019
@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

So here are the architectural updates we will make:

  • add user_roles table for organizers and admins
  • add routes for tags and events

cc @timmyichen @Zeko369 Is it fine?

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

Remove typeORM from here, this should be DB schema only

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

for user roles, how did you want to model this? i can imagine a world where a single user could be both a participant and an organizer, or both an organizer and an admin, etc.

@timmyichen we can create a user_roles table with id and type as string and reference can be made via users table?

@timmyichen
Copy link
Contributor

👍 makes sense to me, just be sure to add a column for the chapter_id for user_roles since each of these roles are specific to a particular chapter/org

type might work better as an enum anyway depending on how typeORM handles those migrations cc @Zeko369

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

We can try using enums, but strings might be simpler and easier when it comes to migrations, not sure what you meant by how typeORM handles those migrations

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

@timmyichen Admin as a user role isn't related to any chapter Tim.

Let's discuss the API endpoints we should create.

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

@timmyichen Should we consider this:

GET /events
GET /events/{eventId}

Wouldn't the 2nd one be redundant as we already have chapters/chapterId/events/eventId?

My point is in the events page we need to show all the events irrespective of a chapter, right? Then how we will do that via the current api routes?

@timmyichen
Copy link
Contributor

I think the only routes that are actually necessary are:

chapters/{chapterId}/events/{eventId}
chapters/{chapterId}/events

Anything else (like /events/{eventId}) would just be shorthand for the above and a bonus.

Regarding Chapter roles, I think an Admin user role is still necessary for deletion perms. You wouldn't want any organizer to also have the ability to delete/edit your Chapter (that power should be given to the creator by default only); organizers should only be able to manage events for the Chapter. Let me know your thoughts on this.

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

Agree, for now I think that c/e/:eID is enough

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

@Zeko369 @timmyichen

My point is in the events page we need to show all the events irrespective of a chapter, right? Then how we will do that via the current api routes?

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

Do we though? They're namespaced to chapters or am I wrong?

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

@Zeko369 They are namespaced to chapters. So in the events page we will loop over all the chapterIds and their respective event(s)?

@timmyichen
Copy link
Contributor

@vkWeb That can be done via a route like /events, but I would not group it in with the same as /events/{eventId} since they would do very different things

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

We could then have
/events -> get all event across chapters (homepage)
/chapter/:id/events -> get chapter events (chapter homepage)?

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

@timmyichen Agreed.

So /events with just GET as a method? We won't be updating anything via this route, right?

For that we have chapters/... route.

@vkWeb
Copy link
Member Author

vkWeb commented Nov 18, 2019

We could then have
/events -> get all event across chapters (homepage)
/chapter/:id/events -> get chapter events (chapter homepage)
?

Perfect!

@vaibhavsingh97
Copy link
Member

@vkWeb can you please update the PR with necessary changes as suggested in comments

@allella
Copy link
Contributor

allella commented Dec 4, 2019

@vkWeb commented on Discord last week that he'd be unavailable for awhile. It may be best to reassign this one.

@vkWeb vkWeb marked this pull request as ready for review December 5, 2019 20:58
@vkWeb
Copy link
Member Author

vkWeb commented Dec 5, 2019

@vaibhavsingh97 as @allella mentioned I'll be inactive for a while :(

I have marked this PR ready for review so you all can push changes and merge them at your pace.

@ceciliaconsta3 would you like to tackle this?

@allella
Copy link
Contributor

allella commented Dec 6, 2019

@Zeko369 @timmyichen @ceciliaconsta3 would you all be okay with reviewing and approving the earlier commits and then opening more specific PRs for the user_roles and API endpoint parts of this conversation.

This has been open for awhile and I could see it sitting and becoming a blocker if we expand the scope to include user roles and API endpoints.

I've reviewed the ddl.sql and the event_tags and tag schema changes look like a good start to me.

I'm not sure what the Swagger documentation is supposed to look like, so I can't comment on that one.

@allella
Copy link
Contributor

allella commented Dec 8, 2019

@nik-john could you look at the Swagger file on this PR and see if it needs anything.

I'm thinking we commit the event tags and move the API routes and user roles comments to new or existing issues so we can close this and not have it lingering.

I've reviewed the schema changes and it looks simple enough to merge.

@QuincyLarson
Copy link
Contributor

@nik-john Could you look at the changes to the Swagger file real quick before we merge this?

@Zeko369 Zeko369 merged commit 2199b81 into freeCodeCamp:master Dec 15, 2019
@vkWeb vkWeb deleted the fix/db-api-schema branch December 15, 2019 21:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DB Schema] User types
6 participants