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

Add TypeORM models #162

Closed

Conversation

eolculnamo2
Copy link
Contributor

  • 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.

Adds sequelize models as show in the db diagram. Potential improvement would be to use HasMany relationships over simply referencing the Id in some case.

@Zeko369
Copy link
Member

Zeko369 commented Nov 9, 2019

You should defiantly add realtions on the ORM level

@eolculnamo2
Copy link
Contributor Author

That’s what I thought. Will add.

@timmyichen
Copy link
Contributor

thanks for adding these! just a quick note, model fields should be snake_case as is standard for SQL/postgres. additionally, each new model will need a corresponding migration file - you could do all of them in one file, or split them up into many - the latter is probably preferred

@eolculnamo2
Copy link
Contributor Author

Updated relationships and casing.

Tag was missing event_id which is required for the one to many relationship. I've added that to the class.

Also, I'm not 100% sure I understand the one-to-one relationships from sequelize-typescript (If someone could double check) :)

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

@eolculnamo2 can you also update this PR with migration files.

@allella
Copy link
Contributor

allella commented Nov 17, 2019

@eolculnamo2 the schema was showing only one tag per event, which seems like an oversight.

Is Discord, I just asked "The mock-ups already show multiple tags per event and it seems a very safe assumption 1 tag per event is not enough.

That said, do we need an MVP user story that says 'As an Admin, I can apply optional tags to an event'?

Or, do we think this isn't a necessary MVP story. If not, then should we still building an event_tags table now to show the future intention of multiple tags?"

@eolculnamo2
Copy link
Contributor Author

The event tags decision is probably not my call, but it seems simple enough to include with the MVP and its already in our mock ups.

Is there a way to automatically build the migrations file from what I already have or does that need to be manually typed? (I ask because it reminds me of C#'s Entity Framework which build it for you)

@timmyichen
Copy link
Contributor

As far as I know there is no way to automatically generate migrations file. If we were using non-typescript sequelize it would be fairly straightforward as the migration file would look almost identical to the model declaration object, but since we are using typescript-sequelize, the model declarations are classes.

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

@timmyichen @eolculnamo2 If we used TypeORM, which has pretty much identical API to sequelize-typescript we could have auto generated migrations based on model files. We haven't discussed that since sequezlie was in the initial stack

@vkWeb
Copy link
Member

vkWeb commented Nov 18, 2019

I agree with @allella that we need multiple tags per event and it should be in MVP. We can control the maximum number of tags organizers can apply client side (or server-side).

I am updating the database, er diagram and api schemas as well. I'll send a PR soon.


@timmyichen, @Zeko369, @nik-john, @allella I was looking at the api routes and I realized that the routes are revolving around chapters. What if an organization doesn't have a chapter?
Just, for example, assume we have Coders.org as an organization and they don't want any chapters under them, they host events under the organization's name. In that case, our routes will return 404, right? I think we should include /events, /events/eventId and so on as routes. What do you all think?

@vkWeb
Copy link
Member

vkWeb commented Nov 18, 2019

Blocking this till we resolve the architectural issues on #251.

@allella
Copy link
Contributor

allella commented Nov 18, 2019

Unless we're not going to show tags at all in the MVP, then I'd also vote to make the multi-tags changes to the schema, which I believe @vkWeb is going to submit as a PR.

Tags were implied to be an MVP feature based on earlier conversations and as evident by the design mockups, but I haven't seen a user story about tags.

Anybody have an issue with creating a user story like 'As an Organizer, I can apply optional tags to an event' ?

@timmyichen
Copy link
Contributor

@Zeko369 I'm down to use TypeORM but we should loop @QuincyLarson into this conversation as I believe it was he who initially proposed sequelize.

@Zeko369
Copy link
Member

Zeko369 commented Nov 18, 2019

Blocked because of #252, resolved -> USING TYPEORM

@Zeko369
Copy link
Member

Zeko369 commented Nov 21, 2019

We're switching to TypeORM, but since sequelize-ts syntax is pretty much TypeORM syntax, this PR will need little changes to make it work with TypeORM.
DONT DO MIGRATATIONS TILL THEN, beacuse we'll use TypeORM generate migrations, so you dont need to write raw SQL, just check if TypeORM was right 🤓

So this is blocked till #257 is merged

@eolculnamo2
Copy link
Contributor Author

Awesome, will get this done soon! I'll shoot to have it done sometime tomorrow or the next day.

@eolculnamo2
Copy link
Contributor Author

Things have come up lately that have made it hard to finish this in a timely manner :/
I've been working on this issue, but if someone who knows TypeORM already wants to knock it out, let me know.

@Zeko369
Copy link
Member

Zeko369 commented Nov 26, 2019

@eolculnamo2 When I finish TypeORM migration PR #257 I'll take over this

@ankorGH
Copy link
Contributor

ankorGH commented Nov 29, 2019

@Zeko369 , if you are not working on it I would like to take a go at it.

@Zeko369
Copy link
Member

Zeko369 commented Nov 29, 2019

Today I'll finish the dev docs on TypeORM and how we're using it so I'll tag you in the pr, and then you can finish this migration @ankorGH

@ankorGH
Copy link
Contributor

ankorGH commented Nov 29, 2019

sure

@QuincyLarson QuincyLarson changed the title Add sequelize models Add TypeORM models Nov 30, 2019
@Zeko369 Zeko369 mentioned this pull request Dec 6, 2019
4 tasks
@vaibhavsingh97
Copy link
Member

@ankorGH you can now work on this PR, since #257 is merged.

@Zeko369
Copy link
Member

Zeko369 commented Dec 6, 2019

@ankorGH If you don't have enough time, I can do this tomorrow

@ankorGH
Copy link
Contributor

ankorGH commented Dec 7, 2019

@Zeko369 I am on it.

@ankorGH
Copy link
Contributor

ankorGH commented Dec 8, 2019

@Zeko369 @eolculnamo2 can you grant me access to make commits to this PR or I should create a new PR

@Zeko369
Copy link
Member

Zeko369 commented Dec 8, 2019

@ankorGH I think that for the sake of simplicity and to get it done sooner it would be better to just create a new PR from your fork

@Zeko369 Zeko369 closed this Dec 8, 2019
@Zeko369
Copy link
Member

Zeko369 commented Dec 8, 2019

Closing and moving to #287

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.

7 participants