-
Notifications
You must be signed in to change notification settings - Fork 6
Organization HABTM Workshops #171
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
base: main
Are you sure you want to change the base?
Conversation
|
Question: Why is this changing the id column type from If we're going to juggle ID column types ... that probably should happen in it's own PR, with a justification that's going to be findable looking through history on it's own (since everything is a squash, standalone ideas get buried in much larger changesets, which makes history spelunking harder) |
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.
Why was this controller deleted?
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.
It's an empty controller not serving any purpose, so I actually never meant to include it in the initial PR, and thought we could add it back in if we need it in the future.
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.
We don't want the organizations controller anymore?
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 didn't mean to include an empty controller in the last PR, so my thinking was that I'd remove it until we actually need it.
@cflipse We can't have foreign key constraints with mixed types, so in order for the But actually... ideally I suspect we'd rather want all the resources to have What do you think? Thanks for calling this out, btw. |
|
@maebeale Oops, didn't mean to request another review since I didn't change anything. I did respond to comments tho. |
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.
Looks great, @dalmaboros ! Please rebase main and merge whenever you're ready.
Okay, yeah -- that makes sense, as for the reasoning for the change. Ah, foreign keys. Rails has been using bigint by default since ... 5.x it seems. Which certainly suggests the tables that still have I'd say that it's very likely that we want to have a single PR dedicated to migrating all ID columns to bigint, one big sweep. 🤔 If you skip adding the FK constraint, that should avoid your need to change the existing tables, to unblock here. Then we can do a follow-up to tighten up the in-db constraints after keys are consistent. How does that sound? |
What is the goal of this PR and why is this important?
We created the Organization model, and Organizations have and belong to many Workshops. This PR implements that association.
How did you approach the change?
Just made a join table.
Anything else to add?
ids on Organization and Facilitator had to be changed frombiginttointeger.