Clean TODOs (#68)

* check next_url & refactor OfficerPosition module

* refactor into the is_active_officer function

* refactor officer data constructor into function

* refactor is_filled_in blocks into methods

* be explicit about the autoincrement constraint

* update many TODOs & clean code

Author: EarthenSky

src/admin/admin.py

@@ -22,7 +22,7 @@
 def upgrade() -> None:
     op.create_table(
         "user_session",
-        sa.Column("id", sa.Integer, primary_key=True),
+        sa.Column("id", sa.Integer, primary_key=True, autoincrement=True),
         sa.Column("issue_time", sa.DateTime, nullable=False),
         sa.Column("session_id", sa.String(512), nullable=False),
         sa.Column("computing_id", sa.String(32), nullable=False),

src/alembic/versions/066f3772fce6_create_user_session_table.py

@@ -22,7 +22,7 @@
 def upgrade() -> None:
     op.create_table(
         "site_user",
-        sa.Column("id", sa.Integer, primary_key=True),
+        sa.Column("id", sa.Integer, primary_key=True, autoincrement=True),
         sa.Column("computing_id", sa.String(32), nullable=False),
     )
 

src/alembic/versions/0db2c57ce969_add_persistent_user_table.py

@@ -78,14 +78,14 @@ def downgrade() -> None:
     op.drop_table("site_user")
     op.create_table(
         "site_user",
-        sa.Column("id", sa.Integer, primary_key=True),
+        sa.Column("id", sa.Integer, primary_key=True, autoincrement=True),
         sa.Column("computing_id", sa.String(32), nullable=False),
     )
 
     op.drop_table("user_session")
     op.create_table(
         "user_session",
-        sa.Column("id", sa.Integer, primary_key=True),
+        sa.Column("id", sa.Integer, primary_key=True, autoincrement=True),
         sa.Column("issue_time", sa.DateTime, nullable=False),
         sa.Column("session_id", sa.String(512), nullable=False),
         sa.Column("computing_id", sa.String(32), nullable=False),

src/alembic/versions/75857bf0c826_introduce_officer_tables.py

@@ -57,16 +57,14 @@ async def create_user_session(db_session: AsyncSession, session_id: str, computi
 async def remove_user_session(db_session: AsyncSession, session_id: str) -> dict:
     query = sqlalchemy.select(UserSession).where(UserSession.session_id == session_id)
     user_session = await db_session.scalars(query)
-    await db_session.delete(user_session.first())  # TODO: what to do with this result that we're awaiting?
+    await db_session.delete(user_session.first())
 
 
 async def check_user_session(db_session: AsyncSession, session_id: str) -> dict:
     query = sqlalchemy.select(UserSession).where(UserSession.session_id == session_id)
     existing_user_session = (await db_session.scalars(query)).first()
 
     if existing_user_session:
-        # TODO: replace this select with an sqlalchemy relationship access
-        # see: https://docs.sqlalchemy.org/en/20/orm/basic_relationships.html
         query = sqlalchemy.select(SiteUser).where(SiteUser.computing_id == existing_user_session.computing_id)
         existing_user = (await db_session.scalars(query)).first()
         return {

src/auth/crud.py

@@ -37,11 +37,15 @@ def generate_session_id_b64(num_bytes: int) -> str:
     description="Login to the sfucsss.org. Must redirect to this endpoint from SFU's cas authentication service for correct parameters",
 )
 async def login_user(
-    next_url: str,  # TODO: ensure next_url is a valid url? or local to our site or something...
+    next_url: str,
     ticket: str,
     db_session: database.DBSession,
     background_tasks: BackgroundTasks,
 ):
+    # TODO: test this
+    if not next_url.startswith(root_ip_address):
+        raise HTTPException(status_code=400, detail=f"invalid next_url={next_url}, must be a page of our site")
+
     # verify the ticket is valid
     url = (
         f"https://cas.sfu.ca/cas/serviceValidate?service={urllib.parse.quote(root_ip_address)}"

src/auth/urls.py

@@ -48,7 +48,6 @@ def current_semester(the_date: date) -> Semester:
     elif the_date.month >= 1:
         return Semester.Spring
     else:
-        # TODO: apparently static typecheckers exist in python!?
         assert_never()
 
 

src/data/semesters.py

@@ -108,7 +108,7 @@ async def load_test_officers_data(db_session: AsyncSession):
     await create_new_officer_term(db_session, OfficerTermData(
         computing_id="abc11",
 
-        position=OfficerPosition.VicePresident.value,
+        position=OfficerPosition.VICE_PRESIDENT,
         start_date=date.today() - timedelta(days=365),
         end_date=date.today() - timedelta(days=1),
 
@@ -125,7 +125,7 @@ async def load_test_officers_data(db_session: AsyncSession):
     await create_new_officer_term(db_session, OfficerTermData(
         computing_id="abc11",
 
-        position=OfficerPosition.ExecutiveAtLarge.value,
+        position=OfficerPosition.EXECUTIVE_AT_LARGE,
         start_date=date.today(),
         end_date=None,
 
@@ -142,7 +142,7 @@ async def load_test_officers_data(db_session: AsyncSession):
     await create_new_officer_term(db_session, OfficerTermData(
         computing_id="abc33",
 
-        position=OfficerPosition.President.value,
+        position=OfficerPosition.PRESIDENT,
         start_date=date.today(),
         end_date=date.today() + timedelta(days=365),
 
@@ -160,7 +160,7 @@ async def load_test_officers_data(db_session: AsyncSession):
     await create_new_officer_term(db_session, OfficerTermData(
         computing_id="abc22",
 
-        position=OfficerPosition.DirectorOfArchives.value,
+        position=OfficerPosition.DIRECTOR_OF_ARCHIVES,
         start_date=date.today(),
         end_date=date.today() + timedelta(days=365),
 
@@ -191,7 +191,7 @@ async def load_test_officers_data(db_session: AsyncSession):
     await update_officer_term(db_session, OfficerTermData(
         computing_id="abc33",
 
-        position=OfficerPosition.President.value,
+        position=OfficerPosition.PRESIDENT,
         start_date=date.today(),
         end_date=date.today() + timedelta(days=365),
 
@@ -226,7 +226,7 @@ async def load_sysadmin(db_session: AsyncSession):
     await create_new_officer_term(db_session, OfficerTermData(
         computing_id=SYSADMIN_COMPUTING_ID,
 
-        position=OfficerPosition.SystemAdministrator.value,
+        position=OfficerPosition.SYSTEM_ADMINISTRATOR,
         start_date=date.today() - timedelta(days=365),
         end_date=None,
 

src/load_test_db.py

@@ -4,122 +4,72 @@
 
 _logger = logging.getLogger(__name__)
 
-# TODO: remove enum, b/d python enums suck
-class OfficerPosition(Enum):
-    President = "president"
-    VicePresident = "vice-president"
-    Treasurer = "treasurer"
-
-    DirectorOfResources = "director of resources"
-    DirectorOfEvents = "director of events"
-    DirectorOfEducationalEvents = "director of educational events"
-    AssistantDirectorOfEvents = "assistant director of events"
-    DirectorOfCommunications = "director of communications"
-    #DirectorOfOutreach = "director of outreach"
-    DirectorOfMultimedia = "director of multimedia"
-    DirectorOfArchives = "director of archives"
-    ExecutiveAtLarge = "executive at large"
-    FirstYearRepresentative = "first year representative"
-
-    ElectionsOfficer = "elections officer"
-    SFSSCouncilRepresentative = "sfss council representative"
-    FroshWeekChair = "frosh week chair"
-
-    SystemAdministrator = "system administrator"
-    Webmaster = "webmaster"
-    SocialMediaManager = "social media manager"
+class OfficerPosition:
+    PRESIDENT = "president"
+    VICE_PRESIDENT = "vice-president"
+    TREASURER = "treasurer"
+
+    DIRECTOR_OF_RESOURCES = "director of resources"
+    DIRECTOR_OF_EVENTS = "director of events"
+    DIRECTOR_OF_EDUCATIONAL_EVENTS = "director of educational events"
+    ASSISTANT_DIRECTOR_OF_EVENTS = "assistant director of events"
+    DIRECTOR_OF_COMMUNICATIONS = "director of communications"
+    #DIRECTOR_OF_OUTREACH = "director of outreach"
+    DIRECTOR_OF_MULTIMEDIA = "director of multimedia"
+    DIRECTOR_OF_ARCHIVES = "director of archives"
+    EXECUTIVE_AT_LARGE = "executive at large"
+    FIRST_YEAR_REPRESENTATIVE = "first year representative"
+
+    ELECTIONS_OFFICER = "elections officer"
+    SFSS_COUNCIL_REPRESENTATIVE = "sfss council representative"
+    FROSH_WEEK_CHAIR = "frosh week chair"
+
+    SYSTEM_ADMINISTRATOR = "system administrator"
+    WEBMASTER = "webmaster"
+    SOCIAL_MEDIA_MANAGER = "social media manager"
 
     @staticmethod
-    def position_values() -> list[str]:
-        return _OFFICER_POSITION_VALUES
+    def position_list() -> list[str]:
+        return _OFFICER_POSITION_LIST
 
     @staticmethod
-    def from_string(position: str) -> Self | None:
-        for item in OfficerPosition:
-            if position == item.value:
-                return item
-
-        _logger.warning(f"Unknown OfficerPosition position = {position}. reporting N/A.")
-        return None
-
-    def to_string(self) -> str:
-        return self.value
-
-    def to_email(self) -> str:
-        match self:
-            case OfficerPosition.President:
-                return "[email protected]"
-            case OfficerPosition.VicePresident:
-                return "[email protected]"
-            case OfficerPosition.Treasurer:
-                return "[email protected]"
-
-            case OfficerPosition.DirectorOfResources:
-                return "[email protected]"
-            case OfficerPosition.DirectorOfEvents:
-                return "[email protected]"
-            case OfficerPosition.DirectorOfEducationalEvents:
-                return "[email protected]"
-            case OfficerPosition.AssistantDirectorOfEvents:
-                return "[email protected]"
-            case OfficerPosition.DirectorOfCommunications:
-                return "[email protected]"
-            case OfficerPosition.DirectorOfMultimedia:
-                return "[email protected]"
-            case OfficerPosition.DirectorOfArchives:
-                return "[email protected]"
-            case OfficerPosition.ExecutiveAtLarge:
-                return "[email protected]"
-            case OfficerPosition.FirstYearRepresentative:
-                return "[email protected]"
-
-            case OfficerPosition.ElectionsOfficer:
-                return "[email protected]"
-            case OfficerPosition.SFSSCouncilRepresentative:
-                return "[email protected]"
-            case OfficerPosition.FroshWeekChair:
-                return "[email protected]"
-
-            case OfficerPosition.SystemAdministrator:
-                return "[email protected]"
-            case OfficerPosition.Webmaster:
-                return "[email protected]"
-            case OfficerPosition.SocialMediaManager:
-                return "N/A"
-
-    def num_active(self) -> int | None:
+    def to_email(position: str) -> str | None:
+        return _EMAIL_MAP.get(position, None)
+
+    @staticmethod
+    def num_active(position: str) -> int | None:
         """
         The number of executive positions active at a given time
         """
         # None means there can be any number active
         if (
-            self == OfficerPosition.ExecutiveAtLarge
-            or self == OfficerPosition.FirstYearRepresentative
+            position == OfficerPosition.ExecutiveAtLarge
+            or position == OfficerPosition.FirstYearRepresentative
         ):
             return 2
         elif (
-            self == OfficerPosition.FroshWeekChair
-            or self == OfficerPosition.SocialMediaManager
+            position == OfficerPosition.FroshWeekChair
+            or position == OfficerPosition.SocialMediaManager
         ):
-            # TODO: configure this value in a database table somewhere?
             return None
         else:
             return 1
 
-    def is_signer(self) -> bool:
+    @staticmethod
+    def is_signer(position: str) -> bool:
         """
         If the officer is a signing authority of the CSSS
         """
         return (
-            self == OfficerPosition.President
-            or self == OfficerPosition.VicePresident
-            or self == OfficerPosition.Treasurer
-            or self == OfficerPosition.DirectorOfResources
-            or self == OfficerPosition.DirectorOfEvents
+            position == OfficerPosition.President
+            or position == OfficerPosition.VicePresident
+            or position == OfficerPosition.Treasurer
+            or position == OfficerPosition.DirectorOfResources
+            or position == OfficerPosition.DirectorOfEvents
         )
 
     @staticmethod
-    def expected_positions() -> list[Self]:
+    def expected_positions() -> list[str]:
         return [
             OfficerPosition.President,
             OfficerPosition.VicePresident,
@@ -145,6 +95,52 @@ def expected_positions() -> list[Self]:
             OfficerPosition.Webmaster,
         ]
 
-_OFFICER_POSITION_VALUES = [
-    pos.value for pos in OfficerPosition.__members__.values()
+_EMAIL_MAP = {
+    OfficerPosition.PRESIDENT: "[email protected]",
+    OfficerPosition.VICE_PRESIDENT: "[email protected]",
+    OfficerPosition.TREASURER: "[email protected]",
+
+    OfficerPosition.DIRECTOR_OF_RESOURCES: "[email protected]",
+    OfficerPosition.DIRECTOR_OF_EVENTS: "[email protected]",
+    OfficerPosition.DIRECTOR_OF_EDUCATIONAL_EVENTS: "[email protected]",
+    OfficerPosition.ASSISTANT_DIRECTOR_OF_EVENTS: "[email protected]",
+    OfficerPosition.DIRECTOR_OF_COMMUNICATIONS: "[email protected]",
+    #OfficerPosition.DIRECTOR_OF_OUTREACH,
+    OfficerPosition.DIRECTOR_OF_MULTIMEDIA: "[email protected]",
+    OfficerPosition.DIRECTOR_OF_ARCHIVES: "[email protected]",
+    OfficerPosition.EXECUTIVE_AT_LARGE: "[email protected]",
+    OfficerPosition.FIRST_YEAR_REPRESENTATIVE: "[email protected]",
+
+    OfficerPosition.ELECTIONS_OFFICER: "[email protected]",
+    OfficerPosition.SFSS_COUNCIL_REPRESENTATIVE: "[email protected]",
+    OfficerPosition.FROSH_WEEK_CHAIR: "[email protected]",
+
+    OfficerPosition.SYSTEM_ADMINISTRATOR: "[email protected]",
+    OfficerPosition.WEBMASTER: "[email protected]",
+    OfficerPosition.SOCIAL_MEDIA_MANAGER: "N/A",
+}
+
+_OFFICER_POSITION_LIST = [
+    OfficerPosition.PRESIDENT,
+    OfficerPosition.VICE_PRESIDENT,
+    OfficerPosition.TREASURER,
+
+    OfficerPosition.DIRECTOR_OF_RESOURCES,
+    OfficerPosition.DIRECTOR_OF_EVENTS,
+    OfficerPosition.DIRECTOR_OF_EDUCATIONAL_EVENTS,
+    OfficerPosition.ASSISTANT_DIRECTOR_OF_EVENTS,
+    OfficerPosition.DIRECTOR_OF_COMMUNICATIONS,
+    #OfficerPosition.DIRECTOR_OF_OUTREACH,
+    OfficerPosition.DIRECTOR_OF_MULTIMEDIA,
+    OfficerPosition.DIRECTOR_OF_ARCHIVES,
+    OfficerPosition.EXECUTIVE_AT_LARGE,
+    OfficerPosition.FIRST_YEAR_REPRESENTATIVE,
+
+    OfficerPosition.ELECTIONS_OFFICER,
+    OfficerPosition.SFSS_COUNCIL_REPRESENTATIVE,
+    OfficerPosition.FROSH_WEEK_CHAIR,
+
+    OfficerPosition.SYSTEM_ADMINISTRATOR,
+    OfficerPosition.WEBMASTER,
+    OfficerPosition.SOCIAL_MEDIA_MANAGER,
 ]