Finish integration tests for officer module (#102)

* Fix profile_picture_url not updating on patch

* add tests for POST /officers/term & insert site_user

* add PATCH /officers/info tests & is_active checks for github & discord modules

* finish all integration tests, woo!

* test loading data locally & add a script for migrating

---------

Co-authored-by: Micah Baker <[email protected]>

src/alembic/versions/166f3772fce7_auth_officer_init.py

@@ -45,17 +45,17 @@ def upgrade() -> None:
         sa.Column("start_date", sa.Date(), nullable=False),
         sa.Column("end_date", sa.Date(), nullable=True),
         sa.Column("nickname", sa.String(length=128), nullable=True),
-        sa.Column("favourite_course_0", sa.String(length=32), nullable=True),
-        sa.Column("favourite_course_1", sa.String(length=32), nullable=True),
-        sa.Column("favourite_pl_0", sa.String(length=32), nullable=True),
-        sa.Column("favourite_pl_1", sa.String(length=32), nullable=True),
+        sa.Column("favourite_course_0", sa.String(length=64), nullable=True),
+        sa.Column("favourite_course_1", sa.String(length=64), nullable=True),
+        sa.Column("favourite_pl_0", sa.String(length=64), nullable=True),
+        sa.Column("favourite_pl_1", sa.String(length=64), nullable=True),
         sa.Column("biography", sa.Text(), nullable=True),
         sa.Column("photo_url", sa.Text(), nullable=True),
     )
     op.create_table(
         "officer_info",
         sa.Column("legal_name", sa.String(length=128), nullable=False),
-        sa.Column("discord_id", sa.String(length=18), nullable=True),
+        sa.Column("discord_id", sa.String(length=32), nullable=True),
         sa.Column("discord_name", sa.String(length=32), nullable=True),
         sa.Column("discord_nickname", sa.String(length=32), nullable=True),
         sa.Column("computing_id", sa.String(length=32), sa.ForeignKey("site_user.computing_id"), primary_key=True),

src/auth/crud.py

@@ -101,35 +101,36 @@ async def get_site_user(db_session: AsyncSession, session_id: str) -> None | Sit
     )
 
 
+async def site_user_exists(db_session: AsyncSession, computing_id: str) -> bool:
+    user = await db_session.scalar(
+        sqlalchemy
+        .select(SiteUser)
+        .where(SiteUser.computing_id == computing_id)
+    )
+    return user is not None
+
+
 # update the optional user info for a given site user (e.g., display name, profile picture, ...)
 async def update_site_user(
     db_session: AsyncSession,
     session_id: str,
     profile_picture_url: str
-) -> None | SiteUserData:
+) -> bool:
     query = (
         sqlalchemy
         .select(UserSession)
         .where(UserSession.session_id == session_id)
     )
     user_session = await db_session.scalar(query)
     if user_session is None:
-        return None
+        return False
 
     query = (
         sqlalchemy
         .update(SiteUser)
         .where(SiteUser.computing_id == user_session.computing_id)
         .values(profile_picture_url = profile_picture_url)
-        .returning(SiteUser) # returns all columns of SiteUser
     )
-    user = await db_session.scalar(query)
-    if user is None:
-        return None
+    await db_session.execute(query)
 
-    return SiteUserData(
-        user_session.computing_id,
-        user.first_logged_in.isoformat(),
-        user.last_logged_in.isoformat(),
-        user.profile_picture_url
-    )
+    return True

src/auth/urls.py

@@ -6,7 +6,7 @@
 import requests  # TODO: make this async
 import xmltodict
 from fastapi import APIRouter, BackgroundTasks, HTTPException, Request
-from fastapi.responses import JSONResponse, RedirectResponse
+from fastapi.responses import JSONResponse, PlainTextResponse, RedirectResponse
 
 import database
 from auth import crud
@@ -130,8 +130,9 @@ async def update_user(
     if session_id is None:
         raise HTTPException(status_code=401, detail="User must be authenticated to get their info")
 
-    user_info = await crud.update_site_user(db_session, session_id, profile_picture_url)
-    if user_info is None:
+    ok = await crud.update_site_user(db_session, session_id, profile_picture_url)
+    await db_session.commit()
+    if not ok:
         raise HTTPException(status_code=401, detail="Could not find user with session_id, please log in")
 
-    return JSONResponse(user_info.serializable_dict())
+    return PlainTextResponse("ok")

src/constants.py

@@ -15,7 +15,9 @@
 COMPUTING_ID_MAX = 8
 
 # see https://support.discord.com/hc/en-us/articles/4407571667351-How-to-Find-User-IDs-for-Law-Enforcement#:~:text=Each%20Discord%20user%20is%20assigned,user%20and%20cannot%20be%20changed.
-DISCORD_ID_LEN = 18
+# NOTE: the length got updated to 19 in july 2024. See https://www.reddit.com/r/discordapp/comments/ucrp1r/only_3_months_until_discord_ids_hit_19_digits/
+# I set us to 32 just in case...
+DISCORD_ID_LEN = 32
 
 # https://github.com/discord/discord-api-docs/blob/main/docs/resources/User.md
 DISCORD_NAME_LEN = 32

src/discord/discord.py

@@ -43,6 +43,10 @@ class Channel:
     name: str
     permission_overwrites: list[str] | None = None
 
+def is_active() -> bool:
+    # if there is no discord token, then consider the module inactive; calling functions may fail without warning!
+    return os.environ.get("DISCORD_TOKEN") is not None
+
 async def _discord_request(
     url: str,
     token: str

src/github/__init__.py

@@ -1,5 +1,6 @@
 # TODO: does this allow importing anything from the module?
 import logging
+import os
 
 from github.internals import add_user_to_team, list_members, list_team_members, list_teams, remove_user_from_team
 from github.types import GithubUserPermissions
@@ -32,6 +33,10 @@
     if kind == "auto"
 ]
 
+def is_active() -> bool:
+    # if there is no github token, then consider the module inactive; calling functions may fail without warning!
+    return os.environ.get("GITHUB_TOKEN") is not None
+
 def officer_teams(position: str) -> list[str]:
     if position == OfficerPosition.DIRECTOR_OF_ARCHIVES:
         return ["doa", "officers"]

src/officers/constants.py

@@ -30,7 +30,11 @@ def position_list() -> list[str]:
     def length_in_semesters(position: str) -> int | None:
         # TODO (#101): ask the committee to maintain a json file with all the important details from the constitution
         """How many semester position is active for, according to the CSSS Constitution"""
-        return _LENGTH_MAP[position]
+        if position not in _LENGTH_MAP:
+            # this can occur for legacy positions
+            return None
+        else:
+            return _LENGTH_MAP[position]
 
     @staticmethod
     def to_email(position: str) -> str | None:

src/officers/crud.py

@@ -1,8 +1,11 @@
 import logging
+from datetime import datetime
 
 import sqlalchemy
 from fastapi import HTTPException
 
+import auth.crud
+import auth.tables
 import database
 import utils
 from data import semesters
@@ -157,6 +160,14 @@ async def create_new_officer_info(
     new_officer_info: OfficerInfo
 ) -> bool:
     """Return False if the officer already exists & don't do anything."""
+    if not await auth.crud.site_user_exists(db_session, new_officer_info.computing_id):
+        # if computing_id has not been created as a site_user yet, add them
+        db_session.add(auth.tables.SiteUser(
+            computing_id=new_officer_info.computing_id,
+            first_logged_in=datetime.now(),
+            last_logged_in=datetime.now()
+        ))
+
     existing_officer_info = await db_session.scalar(
         sqlalchemy
         .select(OfficerInfo)

src/officers/tables.py

@@ -40,11 +40,11 @@ class OfficerTerm(Base):
     end_date = Column(Date, nullable=True)
 
     nickname = Column(String(128), nullable=True)
-    favourite_course_0 = Column(String(32), nullable=True)
-    favourite_course_1 = Column(String(32), nullable=True)
+    favourite_course_0 = Column(String(64), nullable=True)
+    favourite_course_1 = Column(String(64), nullable=True)
     # programming language
-    favourite_pl_0 = Column(String(32), nullable=True)
-    favourite_pl_1 = Column(String(32), nullable=True)
+    favourite_pl_0 = Column(String(64), nullable=True)
+    favourite_pl_1 = Column(String(64), nullable=True)
     biography = Column(Text, nullable=True)
     photo_url = Column(Text, nullable=True) # some urls get big, best to let it be a string
 

src/officers/types.py

@@ -70,36 +70,52 @@ async def validate(self, computing_id: str, old_officer_info: OfficerInfo) -> tu
             validation_failures += [f"invalid phone number {self.phone_number}"]
             corrected_officer_info.phone_number = old_officer_info.phone_number
 
-        if self.discord_name is None or self.discord_name == "":
-            corrected_officer_info.discord_name = None
-            corrected_officer_info.discord_id = None
-            corrected_officer_info.discord_nickname = None
-        else:
-            discord_user_list = await discord.search_username(self.discord_name)
-            if len(discord_user_list) != 1:
-                validation_failures += [
-                    f"unable to find discord user with the name {self.discord_name}"
-                    if len(discord_user_list) == 0
-                    else f"too many discord users start with {self.discord_name}"
-                ]
-                corrected_officer_info.discord_name = old_officer_info.discord_name
-                corrected_officer_info.discord_id = old_officer_info.discord_id
-                corrected_officer_info.discord_nickname = old_officer_info.discord_nickname
+        if discord.is_active():
+            if self.discord_name is None or self.discord_name == "":
+                corrected_officer_info.discord_name = None
+                corrected_officer_info.discord_id = None
+                corrected_officer_info.discord_nickname = None
             else:
-                discord_user = discord_user_list[0]
-                corrected_officer_info.discord_name = discord_user.username
-                corrected_officer_info.discord_id = discord_user.id
-                corrected_officer_info.discord_nickname = discord_user.global_name
+                discord_user_list = await discord.search_username(self.discord_name)
+                if len(discord_user_list) != 1:
+                    validation_failures += [
+                        f"unable to find discord user with the name {self.discord_name}"
+                        if len(discord_user_list) == 0
+                        else f"too many discord users start with {self.discord_name}"
+                    ]
+                    corrected_officer_info.discord_name = old_officer_info.discord_name
+                    corrected_officer_info.discord_id = old_officer_info.discord_id
+                    corrected_officer_info.discord_nickname = old_officer_info.discord_nickname
+                else:
+                    discord_user = discord_user_list[0]
+                    corrected_officer_info.discord_name = discord_user.username
+                    corrected_officer_info.discord_id = discord_user.id
+                    corrected_officer_info.discord_nickname = discord_user.global_name
+        else:
+            # TODO (#27): log that the module is inactive & send an email to [email protected]
+            # (if local is false & we have the email permissions or smth)
+
+            # if module is inactive, don't allow updates to discord username
+            corrected_officer_info.discord_name = old_officer_info.discord_name
+            corrected_officer_info.discord_id = old_officer_info.discord_id
+            corrected_officer_info.discord_nickname = old_officer_info.discord_nickname
+            validation_failures += ["discord module inactive"]
 
         # TODO (#82): validate google-email using google module, by trying to assign the user to a permission or something
         if not utils.is_valid_email(self.google_drive_email):
             validation_failures += [f"invalid email format {self.google_drive_email}"]
             corrected_officer_info.google_drive_email = old_officer_info.google_drive_email
 
         # validate that github user is real
-        if await github.internals.get_user_by_username(self.github_username) is None:
-            validation_failures += [f"invalid github username {self.github_username}"]
+        if github.is_active():
+            if await github.internals.get_user_by_username(self.github_username) is None:
+                validation_failures += [f"invalid github username {self.github_username}"]
+                corrected_officer_info.github_username = old_officer_info.github_username
+        else:
+            # TODO (#27): log that the module is inactive & send an email to [email protected]
+            # (if local is false & we have the email permissions or smth)
             corrected_officer_info.github_username = old_officer_info.github_username
+            validation_failures += ["github module inactive"]
 
         # TODO (#93): add the following to the daily cronjob
         # TODO (#97): if github user exists, invite the github user to the org (or can we simply add them directly?)

src/officers/urls.py

@@ -166,6 +166,8 @@ async def new_officer_term(
     await WebsiteAdmin.has_permission_or_raise(db_session, session_computing_id)
 
     for officer_info in officer_info_list:
+        # if user with officer_info.computing_id has never logged into the website before,
+        # a site_user tuple will be created for them.
         await officers.crud.create_new_officer_info(db_session, OfficerInfo(
             computing_id = officer_info.computing_id,
             # TODO (#71): use sfu api to get legal name from officer_info.computing_id
@@ -186,7 +188,7 @@ async def new_officer_term(
         ))
 
     await db_session.commit()
-    return PlainTextResponse("success")
+    return PlainTextResponse("ok")
 
 @router.patch(
     "/info/{computing_id}",