allow changing computing_id by admins and add all TODOs

Author: EarthenSky

src/officers/constants.py

@@ -28,8 +28,7 @@ def position_list() -> list[str]:
 
     @staticmethod
     def length_in_semesters(position: str) -> int | None:
-        # TODO: ask the committee to maintain a json file with all the important details from the constitution
-        # (I can create the version version of the file)
+        # 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]
 
@@ -71,6 +70,7 @@ def is_signer(position: str) -> bool:
 
     @staticmethod
     def expected_positions() -> list[str]:
+        # TODO (#93): use this function in the daily cronjobs
         return [
             OfficerPosition.PRESIDENT,
             OfficerPosition.VICE_PRESIDENT,
@@ -81,11 +81,11 @@ def expected_positions() -> list[str]:
             OfficerPosition.DIRECTOR_OF_EDUCATIONAL_EVENTS,
             OfficerPosition.ASSISTANT_DIRECTOR_OF_EVENTS,
             OfficerPosition.DIRECTOR_OF_COMMUNICATIONS,
-            #OfficerPosition.DIRECTOR_OF_OUTREACH, # TODO: when https://github.com/CSSS/documents/pull/9/files merged
+            #OfficerPosition.DIRECTOR_OF_OUTREACH, # TODO (#101): when https://github.com/CSSS/documents/pull/9/files merged
             OfficerPosition.DIRECTOR_OF_MULTIMEDIA,
             OfficerPosition.DIRECTOR_OF_ARCHIVES,
             OfficerPosition.EXECUTIVE_AT_LARGE,
-            # TODO: expect these only during fall & spring semesters. Also, TODO: this todo is correct...
+            # TODO (#101): expect these only during fall & spring semesters.
             #OfficerPosition.FIRST_YEAR_REPRESENTATIVE,
 
             #ElectionsOfficer,

src/officers/crud.py

@@ -196,8 +196,8 @@ async def update_officer_info(
     if officer_info is None:
         return False
 
-    # TODO: how to detect an entry insert error? For example, what happens if
-    # we try to set our discord id to be the same as another executive's?
+    # NOTE: if there's ever an insert entry error, it will raise SQLAlchemyError
+    # see: https://stackoverflow.com/questions/2136739/how-to-check-and-handle-errors-in-sqlalchemy
     await db_session.execute(
         sqlalchemy
         .update(OfficerInfo)
@@ -211,7 +211,7 @@ async def update_officer_term(
     new_officer_term: OfficerTerm,
 ) -> bool:
     """
-    Update based on the term id.
+    Update all officer term data in `new_officer_term` based on the term id.
     Returns false if the above entry does not exist.
     """
     officer_term = await db_session.scalar(
@@ -231,7 +231,6 @@ async def update_officer_term(
     return True
 
 async def delete_officer_term_by_id(db_session: database.DBSession, term_id: int):
-    # TODO: detect error and return the response
     await db_session.execute(
         sqlalchemy
         .delete(OfficerTerm)

src/officers/tables.py

@@ -25,8 +25,7 @@
 class OfficerTerm(Base):
     __tablename__ = "officer_term"
 
-    # TODO: change primary key to computing_id, position, start_date?
-    # nah, I like having a term-id -> just do a check when inserting?
+    # TODO (#98): create a unique constraint for (computing_id, position, start_date).
     id = Column(Integer, primary_key=True, autoincrement=True)
 
     computing_id = Column(
@@ -83,8 +82,7 @@ def is_filled_in(self):
 
     def to_update_dict(self) -> dict:
         return {
-            # TODO: do we want computing_id to be changeable?
-            # "computing_id": self.computing_id,
+            "computing_id": self.computing_id,
 
             "position": self.position,
             "start_date": self.start_date,
@@ -114,24 +112,21 @@ class OfficerInfo(Base):
     legal_name = Column(String(128), nullable=False) # some people have long names, you never know
     phone_number = Column(String(24), nullable=True)
 
-    # a null discord id would mean you don't have discord
-    # TODO: add unique constraints to these (stops users from stealing the username of someone else)
+    # TODO (#99): add unique constraints to discord_id (stops users from stealing the username of someone else)
     discord_id = Column(String(DISCORD_ID_LEN), nullable=True)
     discord_name = Column(String(DISCORD_NAME_LEN), nullable=True)
     # this is their nickname in the csss server
     discord_nickname = Column(String(DISCORD_NICKNAME_LEN), nullable=True)
 
     # Technically 320 is the most common max-size for emails, but we'll use 256 instead,
     # since it's reasonably large (input validate this too)
-    # TODO: add unique constraint to this (stops users from stealing the username of someone else)
+    # TODO (#99): add unique constraint to this (stops users from stealing the username of someone else)
     google_drive_email = Column(String(256), nullable=True)
 
-    # TODO: add unique constraint to this (stops users from stealing the username of someone else)
+    # TODO (#99): add unique constraint to this (stops users from stealing the username of someone else)
     github_username = Column(String(GITHUB_USERNAME_LEN), nullable=True)
 
-    # NOTE: not sure if we'll need this, depending on implementation
-    # TODO: get this data on the fly when requested, but rate limit users
-    # to something like 1/s 100/hour
+    # TODO (#22): add support for giving executives bitwarden access automagically
     # has_signed_into_bitwarden = Column(Boolean)
 
     def serializable_dict(self) -> dict:

src/officers/types.py

@@ -91,7 +91,7 @@ async def validate(self, computing_id: str, old_officer_info: OfficerInfo) -> tu
                 corrected_officer_info.discord_id = discord_user.id
                 corrected_officer_info.discord_nickname = discord_user.global_name
 
-        # TODO: validate google-email using google module, by trying to assign the user to a permission or something
+        # 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
@@ -101,15 +101,16 @@ async def validate(self, computing_id: str, old_officer_info: OfficerInfo) -> tu
             validation_failures += [f"invalid github username {self.github_username}"]
             corrected_officer_info.github_username = old_officer_info.github_username
 
-        # TODO: if github user exists, invite the github user to the org (or can we simply add them directly?)
-        # -> do so outside this function
-        # TODO: detect if changing github username & uninvite old user
+        # 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?)
+        # -> do so outside this function. Also, detect if the github username is being changed & uninvite the old user
 
         return validation_failures, corrected_officer_info
 
 @dataclass
 class OfficerTermUpload:
     # only admins can change:
+    computing_id: str
     position: str
     start_date: date
     end_date: None | date = None
@@ -122,22 +123,20 @@ class OfficerTermUpload:
     favourite_pl_1: None | str = None
     biography: None | str = None
 
-    # TODO: we're going to need an API call to upload images
-    # NOTE: changing the name of this variable without changing all instances is breaking
+    # TODO (#39): we're going to need an endpoint for uploading images
     photo_url: None | str = None
 
     def valid_or_raise(self):
-        # NOTE: An officer can change their own data for terms that are ongoing.
         if self.position not in OfficerPosition.position_list():
             raise HTTPException(status_code=400, detail=f"invalid new position={self.position}")
         elif self.end_date is not None and self.start_date > self.end_date:
             raise HTTPException(status_code=400, detail="end_date must be after start_date")
 
-    def to_officer_term(self, term_id: str, computing_id:str) -> OfficerTerm:
+    def to_officer_term(self, term_id: str) -> OfficerTerm:
         return OfficerTerm(
             id = term_id,
-            computing_id = computing_id,
 
+            computing_id = self.computing_id,
             position = self.position,
             start_date = self.start_date,
             end_date = self.end_date,

src/officers/urls.py

@@ -262,7 +262,8 @@ async def update_term(
         )
 
     if (
-        officer_term_upload.position != old_officer_term.position
+        officer_term_upload.computing_id != old_officer_term.computing_id
+        or officer_term_upload.position != old_officer_term.position
         or officer_term_upload.start_date != old_officer_term.start_date
         or officer_term_upload.end_date != old_officer_term.end_date
     ):
@@ -274,7 +275,7 @@ async def update_term(
     # TODO (#27): log all important changes to a .log file
     success = await officers.crud.update_officer_term(
         db_session,
-        officer_term_upload.to_officer_term(term_id, old_officer_term.computing_id)
+        officer_term_upload.to_officer_term(term_id)
     )
     if not success:
         raise HTTPException(status_code=400, detail="the associated officer_term does not exist yet, please create it first")
@@ -306,6 +307,8 @@ async def remove_officer(
     deleted_officer_term = await officers.crud.get_officer_term_by_id(db_session, term_id)
 
     # TODO (#27): log all important changes to a .log file
+
+    # TODO (#100): return whether the deletion succeeded or not
     await officers.crud.delete_officer_term_by_id(db_session, term_id)
     await db_session.commit()