clean up naming of functions

Author: EarthenSky

src/cron/daily.py

@@ -7,7 +7,7 @@
 import google_api
 import utils
 from database import _db_session
-from officers.crud import all_officer_terms, get_user_by_username, officer_terms
+from officers.crud import all_officer_data, get_user_by_username
 
 _logger = logging.getLogger(__name__)
 
@@ -18,7 +18,7 @@ async def update_google_permissions(db_session):
 
     # TODO: for performance, only include officers with recent end-date (1 yr)
     # but measure performance first
-    for term in await all_officer_terms(db_session):
+    for term in await all_officer_data(db_session):
         if utils.is_active(term):
             # TODO: if google drive permission is not active, update them
             pass
@@ -31,7 +31,7 @@ async def update_google_permissions(db_session):
 async def update_github_permissions(db_session):
     github_permissions, team_id_map = github.all_permissions()
 
-    for term in await all_officer_terms(db_session):
+    for term in await all_officer_data(db_session):
         new_teams = (
             # move all active officers to their respective teams
             github.officer_teams(term.position)

src/officers/constants.py

@@ -1,6 +1,3 @@
-from typing import Self
-
-
 class OfficerPosition:
     PRESIDENT = "president"
     VICE_PRESIDENT = "vice-president"

src/officers/crud.py

@@ -5,7 +5,6 @@
 
 import database
 import utils
-from auth.tables import SiteUser
 from data import semesters
 from officers.constants import OfficerPosition
 from officers.tables import OfficerInfo, OfficerTerm
@@ -69,24 +68,23 @@ async def officer_term(db_session: database.DBSession, term_id: int) -> OfficerT
         raise HTTPException(status_code=400, detail=f"Could not find officer_term with id={term_id}")
     return officer_term
 
-# TODO: change to "get_officer_terms" naming convention (& all functions in this module)
-async def officer_terms(
+async def get_officer_terms(
     db_session: database.DBSession,
     computing_id: str,
     max_terms: None | int,
-    # will not include officer term info that has not been filled in yet.
-    view_only_filled_in: bool,
+    # will include term info for officers that are not active
+    # or have not yet been filled out
+    include_inactive: bool,
 ) -> list[OfficerTerm]:
     query = (
         sqlalchemy
         .select(OfficerTerm)
         .where(OfficerTerm.computing_id == computing_id)
+        .order_by(OfficerTerm.start_date.desc())
     )
-    # TODO: does active == filled_in ?
-    if view_only_filled_in:
+    if not include_inactive:
         query = utils.is_active_officer(query)
 
-    query = query.order_by(OfficerTerm.start_date.desc())
     if max_terms is not None:
         query.limit(max_terms)
 
@@ -156,21 +154,20 @@ async def current_executive_team(db_session: database.DBSession, include_private
 
     return officer_data
 
-async def all_officer_terms(
+async def all_officer_data(
     db_session: database.DBSession,
     include_private: bool,
     view_only_filled_in: bool,
 ) -> list[OfficerData]:
     """
-    Orders officers recent first.
-
     This could be a lot of data, so be careful.
 
     TODO: optionally paginate data, so it's not so bad.
     """
     query = sqlalchemy.select(OfficerTerm)
     if view_only_filled_in:
         query = OfficerTerm.sql_is_filled_in(query)
+    # Ordered recent first
     query = query.order_by(OfficerTerm.start_date.desc())
     officer_terms = (await db_session.scalars(query)).all()
 

src/officers/tables.py

@@ -194,4 +194,3 @@ def to_update_dict(self) -> dict:
             "github_username": self.github_username,
             "google_drive_email": self.google_drive_email,
         }
-

src/officers/urls.py

@@ -78,7 +78,7 @@ async def has_access(db_session: database.DBSession, request: Request) -> bool:
 
     has_private_access = await has_access(db_session, request)
 
-    all_officer_data = await officers.crud.all_officer_terms(db_session, has_private_access, view_only_filled_in)
+    all_officer_data = await officers.crud.all_officer_data(db_session, has_private_access, view_only_filled_in)
     all_officer_data = [officer_data.serializable_dict() for officer_data in all_officer_data]
     return JSONResponse(all_officer_data)
 
@@ -92,7 +92,7 @@ async def get_officer_terms(
     computing_id: str,
     # the maximum number of terms to return, in chronological order
     max_terms: int | None = None,
-    view_only_filled_in: bool = True,
+    include_inactive: bool = False,
 ):
     # TODO: put these into a function
     session_id = request.cookies.get("session_id", None)
@@ -105,18 +105,18 @@ async def get_officer_terms(
         raise HTTPException(status_code=401)
 
     if (
-        not view_only_filled_in
-        and computing_id != session_computing_id
+        computing_id != session_computing_id
+        and include_inactive
         and not await WebsiteAdmin.has_permission(db_session, session_computing_id)
     ):
         raise HTTPException(status_code=401)
 
     # all term info is public, so anyone can get any of it
-    officer_terms = await officers.crud.officer_terms(
+    officer_terms = await officers.crud.get_officer_terms(
         db_session,
         computing_id,
         max_terms,
-        view_only_filled_in=view_only_filled_in
+        include_inactive=include_inactive
     )
     return JSONResponse([term.serializable_dict() for term in officer_terms])
 
@@ -323,13 +323,11 @@ async def update_term(
         # 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")
 
-    # TODO: turn is_active into a utils function
-    is_active = (old_officer_term.end_date is None) or (datetime.today() <= old_officer_term.end_date)
     if (
-        not is_active
+        not utils.is_active_term(old_officer_term)
         and not await WebsiteAdmin.has_permission(db_session, session_computing_id)
     ):
-        raise HTTPException(status_code=401, detail="only website admin can update a past (non-active) term")
+        raise HTTPException(status_code=401, detail="only website admin can update a non-active term")
 
     # NOTE: Only admins can write new versions of position, start_date, and end_date.
     if (

src/permission/types.py

@@ -46,8 +46,7 @@ async def has_permission(db_session: database.DBSession, computing_id: str) -> b
         """
         A website admin has to be an active officer who has one of the above positions
         """
-        position_list = await officers.crud.current_officer_positions(db_session, computing_id)
-        for position in position_list:
+        for position in await officers.crud.current_officer_positions(db_session, computing_id):
             if position in WebsiteAdmin.WEBSITE_ADMIN_POSITIONS:
                 return True
         return False

tests/integration/test_officers.py

@@ -5,7 +5,7 @@
 import load_test_db
 from database import SQLALCHEMY_TEST_DATABASE_URL, DatabaseSessionManager
 from officers.constants import OfficerPosition
-from officers.crud import all_officer_terms, current_executive_team, most_recent_officer_term
+from officers.crud import all_officer_data, current_executive_team, most_recent_officer_term
 
 # TODO: setup a database on the CI machine & run this as a unit test then (since
 # this isn't really an integration test)
@@ -56,7 +56,7 @@ async def test__read_execs(database_setup):
         assert next(iter(current_exec_team.values()))[0].private_data is not None
         assert next(iter(current_exec_team.values()))[0].private_data.computing_id == "abc33"
 
-        all_terms = await all_officer_terms(db_session, include_private=True)
+        all_terms = await all_officer_data(db_session, include_private=True)
         assert len(all_terms) == 3