Add honourary member endpoints#150
Conversation
jbriones1
left a comment
jbriones1
left a comment
There was a problem hiding this comment.
Thanks for the PR, looks really good!
One thing that will need to change in many places is that "honourary" is less common than "honorary", even in Canada. That's my bad since that's how I wrote it in the issue. Please go through and change everything that says "honourary" to "honorary"
There was a problem hiding this comment.
This file can be removed.
| if honorary_member is None: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=f"honorary member term with id={term_id} does not exist", | ||
| ) |
There was a problem hiding this comment.
<module>/crud.py files shouldn't raise HTTP errors, this will be changed for the Officers and Nominees module in the future. Return None and have the error be raised in honorary/urls.py.
This is so unit tests for CRUD operation don't need to test HTTPExceptions that are meant for API endpoints.
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="cannot update or delete a term that has already ended", | ||
| ) |
There was a problem hiding this comment.
Same reason as above You can change this to a function that returns a boolean and rename it to has_term_ended(honorary_member: HonoraryMemberDB) and then raise the HTTPException where needed.
Also, this should return status code 409 Conflict instead of 403 Forbidden. We use 409 to indicate that the request is well-formed, but the state of the resource doesn't allow it, while 403 Forbidden is when someone is authenticated, but not authorized to carry out an action.
|
|
||
|
|
||
| class HonoraryMemberCreate(BaseModel): | ||
| name: str = Field(..., max_length=128) |
There was a problem hiding this comment.
Use HONORARY_MEMBER_MAX_LENGTH = 128.
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.create_table('honorary_member', | ||
| sa.Column('id', sa.Integer(), autoincrement=True, nullable=False), | ||
| sa.Column('name', sa.String(length=128), nullable=False), |
There was a problem hiding this comment.
For the max length, make a file in the honorary folder and call it constants.py. Define a constant HONORARY_MEMBER_MAX_LENGTH = 128 and then use it in the models file, table definition, and this file.
| __tablename__ = "honorary_member" | ||
|
|
||
| id: Mapped[int] = mapped_column(Integer, primary_key=True, autoincrement=True) | ||
| name: Mapped[str] = mapped_column(String(128), nullable=False) |
There was a problem hiding this comment.
Use HONORARY_MEMBER_MAX_LENGTH = 128.
| honorary_members: list[HonoraryMemberDB], | ||
| ) -> list[HonoraryMemberDB]: | ||
| db_session.add_all(honorary_members) | ||
| await db_session.flush() |
There was a problem hiding this comment.
Remove the flush here and add it wherever it needs to happen.
There was a problem hiding this comment.
Add a responses dict to each endpoint to indicate what status codes can be returned. See
csss-site-backend/src/nominees/urls.py
Line 112 in aac1432
2 participants
Closes #130
Summary:
Testing: