[LLSC-16] Add basic user model #2

mslwang · 2024-10-03T05:19:18Z

Notion ticket link

Implementation description

Adding initial User and Role models, so that other services aren't blocked on this model. First time running migration script as well.

Steps to test

Run docker-compose up

What should reviewers focus on?

User, Role model
migrations

Checklist

My PR name is descriptive and in imperative tense
My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
I have run the appropriate linter(s)
I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

mmiqball · 2024-10-16T21:39:15Z

backend/app/models/User.py

+
+class User(Base):
+    __tablename__ = "users"
+    id = Column(String, primary_key=True)  # UUID


from sqlalchemy.dialects.postgresql import UUID import uuid id = Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)

How about this to auto-generate UUIDs when a new user is inserted into the db?

This was the migration I added for this:

"""Update users id to UUID with default Revision ID: c9bc2b4d1036 Revises: 79de0b981dd8 Create Date: 2024-10-16 17:13:53.820521 """ from typing import Sequence, Union from alembic import op import sqlalchemy as sa # revision identifiers, used by Alembic. revision: str = 'c9bc2b4d1036' down_revision: Union[str, None] = '79de0b981dd8' branch_labels: Union[str, Sequence[str], None] = None depends_on: Union[str, Sequence[str], None] = None def upgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### op.alter_column('users', 'id', existing_type=sa.VARCHAR(), type_=sa.UUID(), postgresql_using="id::uuid", server_default=sa.text("gen_random_uuid()"), existing_nullable=False) # ### end Alembic commands ### def downgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### op.alter_column('users', 'id', existing_type=sa.UUID(), type_=sa.VARCHAR(), postgresql_using="id::text", server_default=None, existing_nullable=False) # ### end Alembic commands ###

Yep that's a stronger guarantee, let's do that instead.

mmiqball · 2024-10-16T21:40:31Z

backend/app/models/Role.py

+from .Base import Base
+
+
+class Role(Base):


Should we add in roles right now?

I added a migration with this in my ticket since we need the roles in the db to assign them to users. I can keep it there, or if you think it belongs in this PR that would work too.

op.bulk_insert( sa.table('roles', sa.Column('id', sa.Integer(), nullable=False), sa.Column('name', sa.String(length=80), nullable=False), ), [ {'id': 1, 'name': 'participant'}, {'id': 2, 'name': 'volunteer'}, {'id': 3, 'name': 'admin'}, ] )

I think I'll move it to this PR, given that @emma-x1 can just work off this branch.

In that case, can you please add in an auth_id field as well? It's needed for referencing the Firebase UID associated with a user in our db. I was keeping it in my ticket since that's where I initialized Firebase auth, but it's probably good to add it in here if @emma-x1 is working off this branch.

FIeld:

auth_id = Column(String, nullable=True) # Firebase Auth ID

Migration:

def upgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### op.add_column('users', sa.Column('auth_id', sa.String(), nullable=True)) # ### end Alembic commands ### def downgrade() -> None: # ### commands auto generated by Alembic - please adjust! ### op.drop_column('users', 'auth_id') # ### end Alembic commands ###

Should we make nullable=False when Firebase auth is implemented?

Yep, since every user should have a Firebase UID. We can just add it in as nullable=False right now too, should be fine.

I think with the new edge cases you mentioned, we should keep this. From what I understand, a participant can have different types (caregiver, patient) so we can maybe assign multiple roles to that user to handle those cases. It also seems safer since stuff does seem to change around a bit whenever we talk to the npo

Mayank808 · 2024-10-17T00:35:11Z

backend/app/migrations/versions/4ba3479cb8df_create_user_table_and_roles.py

@@ -0,0 +1,50 @@
+"""create user table and roles


just wanted to confirm all files under migration folder are autogenerated from defined model/schemas correct

Yes for the most part. The only exception would be seeding for constants or roles in this case.

Mayank808

As starter table definitions, i think this is fine for now. Good to merge once the comments above are added in. We should probably add in a github action/precommit hook to ensure the migration scripts are autogenerated every time.

Mayank808 · 2024-10-17T00:36:08Z

backend/alembic.ini

@@ -3,7 +3,7 @@
 [alembic]
 # path to migration scripts
 # Use forward slashes (/) also on windows to provide an os agnostic path
-script_location = migrations
+script_location = app/migrations


is there a reason we needed to move migration scripts into the app?

I moved it a while back to figure out the migration issue, but it's good now. Moved it back to migrations folder

Mayank808 · 2024-10-17T00:42:14Z

backend/app/models/Role.py

+
+from .Base import Base
+
+


Can we add just a few comments above table definitions, giving a brief description of what the table is used for. Ie role tables will be like entitlements table housing all possible user roles on the system. Documentation on variables/relations if we think that's important could also be added but might be overkill.

Mayank808 · 2024-10-17T00:43:05Z

backend/app/models/__init__.py


+from .Base import Base


``

Suggested change

from .Base import Base

We need that for Base.metadata.create_all(bind=engine) and in backend/app/migrations/env.py

Note: removing Base.metadata.create_all(bind=engine) so that we rely only on migrations to create tables.

Converted all nullable columns to be non-nullable Migrations added: - adding auth id to user model - updated user id to uuid - Seeded roles directly through migration

mmiqball · 2024-10-28T05:36:12Z

backend/migrations/versions/59bb2488a76b_insert_roles.py

+
+def downgrade() -> None:
+    # ### commands auto generated by Alembic - please adjust! ###
+    pass


Should we have a command to remove the roles we added in downgrade?

yep let's do that.

mslwang added 4 commits October 7, 2024 21:14

Add psycog2

dd69b49

Add User and Role models and update db init

f47b1f7

Move migrations folder and add first migration

4273653

Update linting of migrations for alembic

669319e

mslwang force-pushed the mslwang/LLSC-16-add-user branch from 470b85e to 669319e Compare October 8, 2024 01:16

mslwang added 2 commits October 7, 2024 21:58

Lint and format files

7085f05

Backend content to README

865f46c

mslwang requested review from Mayank808 and mmiqball October 16, 2024 21:21

Update alembic ini

c5ad272

mmiqball reviewed Oct 16, 2024

View reviewed changes

Mayank808 reviewed Oct 17, 2024

View reviewed changes

Mayank808 approved these changes Oct 17, 2024

View reviewed changes

Mayank808 reviewed Oct 17, 2024

View reviewed changes

mslwang added 7 commits October 26, 2024 19:33

Update User model and add migrations

4e42a8a

Converted all nullable columns to be non-nullable Migrations added: - adding auth id to user model - updated user id to uuid - Seeded roles directly through migration

Update server to do migration upgrade head on startup

b768b49

Update User model and migrations

b855d86

Update backend README

391686f

Remove dotenv

25360c2

Reformat

aa30eb1

Move migrations folder back to backend

686e041

mslwang requested a review from mmiqball October 28, 2024 00:51

mmiqball reviewed Oct 28, 2024

View reviewed changes

mmiqball approved these changes Oct 28, 2024

View reviewed changes

Add downgrade for role insertion

781a5e9

mslwang merged commit ff1b4f7 into main Oct 28, 2024
1 check passed

mslwang deleted the mslwang/LLSC-16-add-user branch October 28, 2024 07:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LLSC-16] Add basic user model #2

[LLSC-16] Add basic user model #2

mslwang commented Oct 3, 2024 •

edited

Loading

mmiqball Oct 16, 2024 •

edited

Loading

mslwang Oct 16, 2024

mmiqball Oct 16, 2024

mslwang Oct 16, 2024

mmiqball Oct 16, 2024

mslwang Oct 16, 2024

mmiqball Oct 16, 2024

mmiqball Oct 26, 2024

Mayank808 Oct 17, 2024

mslwang Oct 17, 2024

Mayank808 left a comment •

edited

Loading

Mayank808 Oct 17, 2024

mslwang Oct 28, 2024

Mayank808 Oct 17, 2024 •

edited

Loading

Mayank808 Oct 17, 2024

mslwang Oct 17, 2024 •

edited

Loading

mslwang Oct 28, 2024

mmiqball Oct 28, 2024

mslwang Oct 28, 2024

[LLSC-16] Add basic user model #2

[LLSC-16] Add basic user model #2

Conversation

mslwang commented Oct 3, 2024 • edited Loading

Notion ticket link

Implementation description

Steps to test

What should reviewers focus on?

Checklist

mmiqball Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mayank808 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mayank808 Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mslwang Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mslwang commented Oct 3, 2024 •

edited

Loading

mmiqball Oct 16, 2024 •

edited

Loading

Mayank808 left a comment •

edited

Loading

Mayank808 Oct 17, 2024 •

edited

Loading

mslwang Oct 17, 2024 •

edited

Loading