clean final endpoint, PATCH /term

Author: EarthenSky

src/officers/crud.py

@@ -184,7 +184,10 @@ async def create_new_officer_term(
         )
     db_session.add(new_officer_term)
 
-async def update_officer_info(db_session: database.DBSession, new_officer_info: OfficerInfo) -> bool:
+async def update_officer_info(
+    db_session: database.DBSession,
+    new_officer_info: OfficerInfo
+) -> bool:
     """
     Return False if the officer doesn't exist yet
     """
@@ -209,28 +212,25 @@ async def update_officer_info(db_session: database.DBSession, new_officer_info:
 async def update_officer_term(
     db_session: database.DBSession,
     new_officer_term: OfficerTerm,
-):
+) -> bool:
     """
     Update based on the term id.
-
     Returns false if the above entry does not exist.
     """
-    query = (
+    officer_term = await db_session.scalar(
         sqlalchemy
         .select(OfficerTerm)
         .where(OfficerTerm.id == new_officer_term.id)
     )
-    officer_term = await db_session.scalar(query)
     if officer_term is None:
         return False
 
-    query = (
+    await db_session.execute(
         sqlalchemy
         .update(OfficerTerm)
         .where(OfficerTerm.id == new_officer_term.id)
         .values(new_officer_term.to_update_dict())
     )
-    await db_session.execute(query)
     return True
 
 async def remove_officer_term():

src/officers/tables.py

@@ -28,6 +28,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?
     id = Column(Integer, primary_key=True, autoincrement=True)
 
     computing_id = Column(

src/officers/types.py

@@ -22,6 +22,7 @@ class OfficerInfoUpload:
     github_username: None | str = None
     google_drive_email: None | str = None
 
+    # TODO: why are valid_or_raise and validate separate?
     def valid_or_raise(self):
         # TODO: more checks
         if self.legal_name is not None and self.legal_name == "":
@@ -119,8 +120,6 @@ def valid_or_raise(self):
             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:
-        # TODO: many positions have a length; if the length is defined, fill it in right here
-        # (end date is 1st of month, 12 months after start date's month).
         return OfficerTerm(
             id = term_id,
             computing_id = computing_id,

src/officers/urls.py

@@ -276,38 +276,33 @@ async def update_term(
     officer_term_upload: OfficerTermUpload = Body(), # noqa: B008
 ):
     officer_term_upload.valid_or_raise()
-
-    # Refactor all of these gets & raises into small functions
-    session_id = request.cookies.get("session_id", None)
-    if session_id is None:
-        raise HTTPException(status_code=401, detail="must be logged in")
-
-    session_computing_id = await auth.crud.get_computing_id(db_session, session_id)
-    if session_computing_id is None:
-        raise HTTPException(status_code=401)
+    _, session_computing_id = logged_in_or_raise(request, db_session)
 
     old_officer_term = await officers.crud.get_officer_term_by_id(db_session, term_id)
+    if old_officer_term.computing_id != session_computing_id:
+        await WebsiteAdmin.has_permission_or_raise(
+            db_session, session_computing_id,
+            errmsg="must have website admin permissions to update another user"
+        )
+    elif utils.is_past_term(old_officer_term):
+        await WebsiteAdmin.has_permission_or_raise(
+            db_session, session_computing_id,
+            errmsg="only website admins can update past terms"
+        )
 
-    if (
-        old_officer_term.computing_id != session_computing_id
-        and not await WebsiteAdmin.has_permission(db_session, session_computing_id)
-    ):
-        # the current user can only input the info for another user if they have permissions
-        raise HTTPException(status_code=401, detail="must have website admin permissions to update another user")
-
-    if (
-        utils.is_past_term(old_officer_term)
-        and not await WebsiteAdmin.has_permission(db_session, session_computing_id)
-    ):
-        raise HTTPException(status_code=401, detail="only website admins can update past terms")
-
-    # NOTE: Only admins can write new versions of position, start_date, and end_date.
     if (
         officer_term_upload.position != old_officer_term.position
         or officer_term_upload.start_date != old_officer_term.start_date.date()
         or officer_term_upload.end_date != old_officer_term.end_date.date()
-    ) and not await WebsiteAdmin.has_permission(db_session, session_computing_id):
-        raise HTTPException(status_code=401, detail="Non-admins cannot modify position, start_date, or end_date.")
+    ):
+        await WebsiteAdmin.has_permission_or_raise(
+            db_session, session_computing_id,
+            errmsg="only admins can write new versions of position, start_date, and end_date"
+        )
+
+    if officer_term_upload.position != old_officer_term.position:
+        # TODO: update the end_date here
+        pass
 
     # TODO: log all important changes to a .log file
     success = await officers.crud.update_officer_term(