Feat 144 grouped election endpoints#148
Conversation
jbriones1
left a comment
There was a problem hiding this comment.
Thank you for the PR and for writing tests! Lmk if you have any questions about the changes requests.
| @router.get( | ||
| "", | ||
| description="Returns a list of all election & their status", | ||
| description="Returns a list of all election, their status and nominees (if requested)", |
There was a problem hiding this comment.
Pluralize. "Return a list of all elections,..."
| election_list = await elections.crud.get_all_elections(db_session) | ||
| if election_list is None or len(election_list) == 0: | ||
| raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="no election found") | ||
|
|
||
| current_time = datetime.datetime.now(datetime.UTC) | ||
| if await is_user_election_admin(computing_id, db_session): | ||
| election_metadata_list = [election.private_details(current_time) for election in election_list] | ||
| election_metadata_list = [] | ||
| has_permission = await is_user_election_admin(computing_id, db_session) | ||
|
|
||
| if has_permission: | ||
| for election in election_list: | ||
| election_data = election.private_details(current_time) | ||
| # Get the nominees for the election | ||
| if with_nominees: | ||
| election_data["candidates"] = await _get_election_nominees(db_session, election, has_permission) | ||
| election_metadata_list.append(election_data) | ||
| else: | ||
| election_metadata_list = [election.public_details(current_time) for election in election_list] | ||
| for election in election_list: | ||
| election_data = election.public_details(current_time) | ||
| # Get the nominees for the election | ||
| if with_nominees: | ||
| election_data["candidates"] = await _get_election_nominees(db_session, election, has_permission) | ||
| election_metadata_list.append(election_data) |
There was a problem hiding this comment.
This will query the database once for each election, which is expensive. It'd be better to create a new CRUD operation that gets all the election and nominee information in one query (if it's requested). Something like:
SELECT *
FROM election
LEFT JOIN election_nominee_application
ON election.slug=election_nominee_info.nominee_election
So either the query on line 123 is executed or the query I suggested is executed. Make sure to use LEFT JOIN so elections with no candidates still appear.
You could even go one step further and define the fields to get from the database in the query i.e. don't fetch private details if the user does not have permission, but you don't have to.
If you do decide to remove the private details at the query level you can add response_model_exclude_none=True in the decorator, under response_model on line 114 and that will automatically strip away all fields that have None as their value.
| Retrieves the election data for an election by name. | ||
| Returns private details when the time is allowed. | ||
| If user is an admin or election officer, returns computing ids for each candidate as well. | ||
| Returns private details when user is admin or election officer. |
There was a problem hiding this comment.
You can remove this line, since the next one explains this as well.
| async def _get_election_nominees( | ||
| db_session: database.DBSession, | ||
| election_row: ElectionDB, | ||
| has_permission: bool, | ||
| ) -> list[dict]: | ||
| candidates_list = [] | ||
| all_nominations = await candidates.crud.get_all_candidates_in_election(db_session, election_row.slug) | ||
| if not all_nominations: | ||
| return [] | ||
| for nomination in all_nominations: | ||
| # NOTE: if a nominee does not input their legal name, they are not considered a nominee | ||
| nominee_info = await nominees.crud.get_nominee_info(db_session, nomination.computing_id) | ||
| if nominee_info is None: | ||
| continue | ||
|
|
||
| if has_permission: | ||
| candidate_entry = { | ||
| "full_name": nominee_info.full_name, | ||
| "position": nomination.position, | ||
| "speech": ("No speech provided by this candidate" if nomination.speech is None else nomination.speech), | ||
| "computing_id": nomination.computing_id, | ||
| "linked_in": nominee_info.linked_in, | ||
| "instagram": nominee_info.instagram, | ||
| "email": nominee_info.email, | ||
| "discord_username": nominee_info.discord_username, | ||
| } | ||
| else: | ||
| candidate_entry = { | ||
| "full_name": nominee_info.full_name, | ||
| "position": nomination.position, | ||
| "speech": ("No speech provided by this candidate" if nomination.speech is None else nomination.speech), | ||
| } | ||
| candidates_list.append(candidate_entry) | ||
| return candidates_list |
There was a problem hiding this comment.
Move this to elections/crud.py.
| db_session: database.DBSession, | ||
| election_row: ElectionDB, | ||
| has_permission: bool, | ||
| ) -> list[dict]: |
There was a problem hiding this comment.
Change the return type to list[ElectionNomineeSummary] and modify candidate_entry to use the Pydantic model.
list[dict] is too ambiguous as a type and could lead to bugs later.
| if with_nominees: | ||
| election_json["candidates"] = await _get_election_nominees(db_session, election, has_permission) |
There was a problem hiding this comment.
You can move these statements outside the if has_permission block to prevent the duplicate on line 181-182.
| for candidate in election_2_response["candidates"]: | ||
| assert "full_name" in candidate | ||
| assert "position" in candidate | ||
| assert "speech" in candidate | ||
| assert "computing_id" not in candidate | ||
| assert "linked_in" not in candidate | ||
| assert "instagram" not in candidate | ||
| assert "email" not in candidate | ||
| assert "discord_username" not in candidate |
There was a problem hiding this comment.
Write a helper function for this. If the shape of the response changes in the future we would need to update it on line 105 as well.
Also, you don't need to test the fields that should always appear (full_name, position, speech).
| for candidate in election_2_response["candidates"]: | ||
| assert "full_name" in candidate | ||
| assert "position" in candidate | ||
| assert "speech" in candidate | ||
| assert "computing_id" not in candidate | ||
| assert "linked_in" not in candidate | ||
| assert "instagram" not in candidate | ||
| assert "email" not in candidate | ||
| assert "discord_username" not in candidate |
| assert "full_name" in candidate | ||
| assert "position" in candidate | ||
| assert "speech" in candidate |
There was a problem hiding this comment.
You don't need to test the fields that should always appear.
| assert "full_name" in candidate | ||
| assert "position" in candidate | ||
| assert "speech" in candidate |
Feature: Grouped Election Endpoints
closes #144
Description:
Adds optional nominees to
GET /electionandGET /election/{name}via with_nominees=true.Guests sees:
Admins also get:
Features:
with_nomineesquery param on list and single election GET endpoints (default false)