First Elections PR #63
Conversation
…omineeSepech -> NomineeApplication, changed ForeignKeyConstraints to ForeignKey
…r a user is a current elections officer
src/elections/crud.py
Outdated
| election = (await db_session.execute(sqlalchemy.select(Election).filter_by(slug=params["slug"]))).scalar_one() | ||
|
|
||
| if params["date"] is not None: | ||
| election.date = params["date"] |
There was a problem hiding this comment.
I'm not sure if this syntax actually emits an sql statement, but maybe we could use an update function?
There was a problem hiding this comment.
This emits an SQL statement and will just fetch the first matching election (since slugs are supposed to be unique). An update isn't necessary as this isn't actually modifying anything. (We use an update later in this function though.)
There was a problem hiding this comment.
Does setting election.date = datetime.now() emit an SQL UPDATE? or does it just update the local variable. iirc if you do this assignment then commit, it should update the assignment without the following UPDATE.
There was a problem hiding this comment.
Didn't you suggest above to not commit at the end of this and let API endpoints handle the commit? The elections/update_election endpoint has the commit for this function.
There was a problem hiding this comment.
that is true! i wasn't suggesting to commit here either, just talking about the hypothetical behaviour. I suppose even if you don't commit, doing db_session.execute(...) emits SQL statements.
Looks at this stackoverflow https://stackoverflow.com/questions/9667138/how-to-update-sqlalchemy-row-entry#:~:text=UPDATE%20using%20SQLAlchemy%3A-,user.no_of_logins%20%2B%3D%201,-session.commit()
it implies that setting election.start_datetime = params.start_datetime actually emits some SQL statements! I don't like this choice by sqlalchemy, but there's not much we can do... I think this means you do not need the update statement at the end of this file. Please test it & get back to me if possible!
There was a problem hiding this comment.
OR, you can just remove the update at the end & say yolo
Co-authored-by: Gabe <[email protected]>
…ey has been deprecated
…lected changes across urls.py
…n the project. Changed database.DBSession -> AsyncSession to match other crud functions.
|
Alright, brief high level summary. This PR introduces the following:
This still needs to introduce/do the following:
I'll work on these in my next PR. As of this comment, all of the above comments should have been addressed. |
…e -> end_datetime in elections module.
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="You do not have permission to access this resource", | ||
| headers={"WWW-Authenticate": "Basic"}, | ||
| ) |
There was a problem hiding this comment.
csss-site-backend/src/officers/urls.py
Line 92 in 2ba2e46
you can simplify exceptions a little bit
| aaa | ||
| """ | ||
| session_id = request.cookies.get("session_id", None) | ||
| user_auth = await _validate_user(db_session, session_id) |
There was a problem hiding this comment.
csss-site-backend/src/officers/urls.py
Line 40 in 2ba2e46
I made a function which may have the same behaviour as _validate_user; maybe you can reuse the code?
| sa.Column("nominee_election", sa.String(length=32), nullable=False), | ||
| sa.Column("speech", sa.Text(), nullable=True), | ||
| sa.Column("position", sa.String(length=64), nullable=False), | ||
| sa.ForeignKeyConstraint(["computing_id"], ["election_nominee.computing_id"], ), |
There was a problem hiding this comment.
Why is there a comma and the end of the inside of the function call? Any idea?
src/elections/urls.py
Outdated
| async def create_election( | ||
| request: Request, | ||
| db_session: database.DBSession, | ||
| name: str, | ||
| election_type: str, | ||
| date: datetime | None = None, | ||
| end_date: datetime | None = None, | ||
| survey_link: str | None = None | ||
| ): |
There was a problem hiding this comment.
end_date and date should be changed to end_datetime and start_datetime respectively
| Does not validate if an election _already_ exists | ||
| """ | ||
|
|
||
| election = (await db_session.execute(sqlalchemy.select(Election).filter_by(slug=params.slug))).scalar_one() |
There was a problem hiding this comment.
to be consistent, let's only use the SQL syntax
| election = (await db_session.execute(sqlalchemy.select(Election).filter_by(slug=params.slug))).scalar_one() | |
| election = await db_session.scalar(sqlalchemy.select(Election).where(Election.slug == params.slug)) |
| raise RequestValidationError() | ||
|
|
||
| params = ElectionParameters( | ||
| _slugify(name), |
There was a problem hiding this comment.
since slug is used as a unique identifier, what if we have a duplicate slug here ("My election" vs "my election", etc...)? We should have a check to make sure that slug does not currently exist, so we don't accidentally overwrite a past election.
| detail="You do not have permission to access this resource", | ||
| headers={"WWW-Authenticate": "Basic"}, | ||
| ) | ||
| if slug is not None: |
There was a problem hiding this comment.
slug is not allowed to be none here, since fastapi actually enforces input types (iirc) -> it's worth a simple test anyways as a santity check
| import officers.urls | ||
| import permission.urls | ||
| import tests.urls |
There was a problem hiding this comment.
oops! This is from the merge of main into this branch; please remove this (was something weird I was doing, give it no mind)
| import tests.urls |
| """ | ||
| An current elections officer has access to all elections, prior elections officers have no access. | ||
| """ | ||
| officer_terms = await officers.crud.current_executive_team(db_session, True) |
There was a problem hiding this comment.
current_executive_team has been replaced with current_officers. The behaviour shooooould be the same
| An current elections officer has access to all elections, prior elections officers have no access. | ||
| """ | ||
| officer_terms = await officers.crud.current_executive_team(db_session, True) | ||
| current_election_officer = officer_terms.get(officers.constants.OfficerPosition.ElectionsOfficer.value)[0] |
There was a problem hiding this comment.
what if there are multiple active elections officers? I can imagine this scenario. Instead of using the function current_officers, instead get the list of all active officer positions a user has & check if one of them is elections officer. This can be done more easily using get_officer_terms
elections PR, woo!