feat: rework instance role tables

[gikf]

• 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 main branch of Chapter.

---

• Easiest from the three roles (as instance roles are not utilized anywhere), but I still want to take it slow.
• Adds required tables for instance roles, per https://dbdiagram.io/d/617ae3f8fa17df5ea673e3ea
• Removes old table.
• Seeds administrator and member instance roles.
• Users registering from UI are getting member instance role.

[gitpod-io]

[ghost]

CodeSee Review Map:

Review in an interactive map

View more CodeSee Maps

Legend

[allella]

Thanks for moving things along.

@ojeytonwilliams What do we think about getting the tables and models stubbed out so our schema is fairly up to date with the proposed ER diagram?

I ask because it's sort of confusing that the docs link to a schema that's using different table names and doesn't represent a lot of the decisions that went into the proposed direction.

[allella]

By "stubbing out", I mean the most basic things we can do to get the tables into code, as opposed to going deep into implementing all the logic one table or issue at a time.

Basically, build the skeleton and add the meat to the bones in subsequent PRs?

[gikf]

@allella with role tables rework my plan is to kinda do exactly that. Only change tables, and whichever parts get broken in the process, and also need updates.

[ojeytonwilliams]

@ojeytonwilliams What do we think about getting the tables and models stubbed out so our schema is fairly up to date with the proposed ER diagram?

@allella I like that plan.

Only change tables, and whichever parts get broken in the process, and also need updates.

@gikf that makes sense and this seems like the best place to start.

[ojeytonwilliams]

I was just going over the db diagram https://dbdiagram.io/d/617ae3f8fa17df5ea673e3ea and it seems we're not consistent about which tables should have foreign keys.

As a rule of thumb, how about if it makes most sense to say "Table A has a table B" then B should have the foreign key. e.g. chapters has chapter_users and so chapter_users should be the table with the foreign key.

In this case it makes more sense to say "a user has an instance role" than "an instance role has a user", so how about we move the fk into instance_roles

cc: @moT01

[moT01]

I don't think that rule of thumb makes sense to me.

it makes more sense to say "a user has an instance role" than "an instance role has a user", so how about we move the fk into instance_roles

If a user has an instance role, I think we would want to put the instance_role on the user table with the user. Here's an example using javascript that sort of illustrates what I think is the difference...

a:

const roles = {
  admin: [
    user1,
    user2,
    user3
  ],
  moderator: [
    user3,
    user4,
    user5  
  ]
}

const users = [
  {
    username: 'user1'
  },
  {
    username: 'user2'
  },
  {
    username: 'user3,
  }
  {
    username: 'user4'
  },
  {
    username: 'user5'
  },
  {
    username: 'user6'
  }
]

b:

const roles = {
  admin: 'admin',
  moderator: 'moderator'
};

const users = [
  {
    username: 'user1'
    role: admin
  },
  {
    username: 'user2'
    role: admin
  },
  {
    username: 'user3,
    role: admin
  }
  {
    username: 'user4'
    role: moderator
  },
  {
    username: 'user5'
    role: moderator
  },
  {
    username: 'user6,
    role: moderator
  }
]

Maybe it's not a good idea to try and explain database stuff with javascript, but I think that sort of demonstrates the difference. I guess either way technically works, but I would go with b. I dunno, maybe I'm wrong on it, but that's where my opinion is at the moment.

[ojeytonwilliams]

Thanks @moT01 I think my rule of thumb probably isn't great.

Okay, I'll stop derailing this. Tom's argument makes sense and moving the fk out of the user just seems to make the db more convoluted.

[ojeytonwilliams]

Nice work, @gikf. I only spotted the one thing:

[allella]

@ojeytonwilliams I think having an instance_users table would make sense if a user is going to have more than 1 instance role. However, as it's designed a user can have just one role for the entire instance as set in users.instance_role_id.

Based on our terminology, the roles for an instance may be: owner, administrator, or user

I think your point was to make the instance tables work the same as the event_users and and chapter_users pattern, but those are obviously setup to allow

• event_users - 0 or more event roles per user
• chapter_users - 0 or more chapter roles per user

[allella]

I don't think we need to revisit the finer points, but I'm linking this PR to earlier conversations since it's easy to lose track. Plus, these links tend to help those who are new to the conversation or to help reload things into the brain when they are not fresh.

Earlier Conversations About Roles and Separation of Authorization vs User Details
#898 (comment)
#735 (comment)
#270 (comment)

Database table dump - #967
Terminology - https://github.com/freeCodeCamp/chapter#terminology
Target ER Diagram - https://dbdiagram.io/d/617ae3f8fa17df5ea673e3ea
Database Schema - https://opensource.freecodecamp.org/chapter/ (may be out of date until PRs are committed and SchemaSpy auto-generates)

[ojeytonwilliams]

Thanks for the context, @allella, it is hard to keep track of everything.

The only thing I can add is that each user, event_user and chapter_user should have exactly one role each. Since the higher roles should grant all the permissions of lower roles, there should not be a need for a single user to have more than one.

This is enforced in https://dbdiagram.io/d/617ae3f8fa17df5ea673e3ea by having the user be unique by email and both chapter_user and event_user have unique pairs of ids. That and the fact that all the foreign keys (instance_role_id, chapter_role_id and event_role_id) cannot be null.

[ojeytonwilliams]

LGTM 👍

Are you happy with this, @gikf? If so, I'll merge it.

[ojeytonwilliams]

Oh, sorry, I missed you making it ready to review...

[gikf]

:D That was close.

[ojeytonwilliams]

I may have accidentally clicked the revert button while aiming for the laugh emoji...

Ahem.