Feat 144 grouped election endpoints#148
Conversation
jbriones1
left a comment
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.
jbriones1
left a comment
jbriones1
left a comment
There was a problem hiding this comment.
Thanks for changing things based on my feedback.
The tests look good. If you're not too busy and you're willing to, we can discuss how to populate the database with test data and we could make more comprehensive tests.
| NomineeInfoDB.instagram, | ||
| NomineeInfoDB.email, | ||
| NomineeInfoDB.discord_username, | ||
| ).join(NomineeInfoDB, CandidateDB.computing_id == NomineeInfoDB.computing_id) |
There was a problem hiding this comment.
Needs to be outerjoin or else elections without a candidate don't get fetched.
| CandidateDB.position, | ||
| CandidateDB.speech, | ||
| NomineeInfoDB.full_name, | ||
| ).join(NomineeInfoDB, CandidateDB.computing_id == NomineeInfoDB.computing_id) |
There was a problem hiding this comment.
Need to be outerjoin or else elections without a candidate don't get fetched.
| 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) |
There was a problem hiding this comment.
I think this could also be changed so that you can batch all the requests into one DB query instead of having to hit the database for every single nominee, similar to what you did in get_all_nominees_by_election.
| db_session: database.DBSession, | ||
| with_nominees: bool = Query(False), | ||
| ): | ||
| election_list = await elections.crud.get_all_elections(db_session) |
There was a problem hiding this comment.
If you change get_all_nominees_by_election to use (left) outerjoin, then you can change this to election_list = await (elections.crud.get_all_nominees_by_election(...) if has_permission else elections.crud.get_all_elections(...)), and then shuffle the logic around a bit.
| if with_nominees: | ||
| election_json["candidates"] = await _get_election_nominees(db_session, election, has_permission) | ||
| if with_nominees: | ||
| nominees = await elections.crud._get_election_nominees(db_session, election, has_permission) |
There was a problem hiding this comment.
The underscore at the front of this function indicates it should only be used internally i.e. not outside of the function's module (file). Its not enforced by the interpreter so this runs fine, but you should remove the underscore so that its clear this is not an internal function.
| dependencies=[Depends(perm_election)], | ||
| ) | ||
| async def delete_nominee_info(db_session: database.tDBSession, computing_id: str): | ||
| async def delete_nominee_info(db_session: database.DBSession, computing_id: str): |
There was a problem hiding this comment.
Thanks for fixing that, I let that through >.<
2 participants
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)