Feature/event issue #108 ready for review#146
Conversation
…e date and datetime
arunPdl02
changed the title
jbriones1
left a comment
jbriones1
left a comment
There was a problem hiding this comment.
Thanks for the PR, very clean and understandable code.
For future reference, you can just assume anything that creates, updates, or deletes things from the database requires site admin access. Reads might be harder to understand, so you can ask for those.
| sa.Column('start_date', sa.Date(), nullable=True), | ||
| sa.Column('end_date', sa.Date(), nullable=True), |
There was a problem hiding this comment.
Change these to repeat_start/end_date so its more clear what is being stored. Sorry, it wasn't clear in the spec.
| start_date: datetime.date | None = None | ||
| end_date: datetime.date | None = None |
There was a problem hiding this comment.
Update these to say repeat_start/end_date.
| start_date: datetime.date | None = None | ||
| end_date: datetime.date | None = None |
There was a problem hiding this comment.
Update these to say repeat_start/end_date.
There was a problem hiding this comment.
To reduce the amount of duplicated fields you can use inheritance. Makes it less error-prone to update the exact same field across multiple models.
class EventPublic(BaseModel):
name: str
start_time:
...
class Event(EventPublic):
eid: int
# has all of the fields from EventPublicAlso, I think it's fine to return the eid, so you can remove EventPublic and just return Event.
| start_date: datetime.date | None = None | ||
| end_date: datetime.date | None = None |
There was a problem hiding this comment.
Update these to say repeat_start/end_date.
| "start_date": self.start_date, | ||
| "end_date": self.end_date, | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd prefer using Pydantic's model_dump() if this isn't used anywhere. If it isn't used just remove it so others don't make their own serialization objects.
| responses={ | ||
| 404:{"description": "Event doesn't exist."} | ||
| }, | ||
| operation_id="update_event" |
There was a problem hiding this comment.
Add a site admin dependency.
| final_start_time = body.start_time if body.start_time is not None else db_event.start_time | ||
| final_end_time = body.end_time if body.end_time is not None else db_event.end_time | ||
|
|
||
| if final_start_time > final_end_time: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="The event start time must be before the end time" | ||
| ) | ||
|
|
||
| if not body.start_date and body.end_date: | ||
| if not db_event.start_date: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="The event start date and event end date must be initilized at the same time" | ||
| ) | ||
| if db_event.start_date > body.end_date: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="The event start date must be before the event end date" | ||
| ) | ||
| if body.start_date and not body.end_date: | ||
| if not db_event.end_date: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="The event start date and event end date must be initilized at the same time" | ||
| ) | ||
| if body.start_date > db_event.end_date: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="The event start date must be before the event end date" | ||
| ) |
There was a problem hiding this comment.
Validation should be handled by the model validators you wrote. You can merge the data into a single Pydantic Event object, which should automatically trigger the validator. For example:
db_event = await event.crud.get_event_by_eid(db_session, eid)
...
db_data = Event.model_validate(db_event).model_dump()
patch_data = body.model_dump(exclude_unset=True)
merged_data = {**db_data, **patch_data}
try:
updated_data = Event(**merged_data) # validators would run here
except ValidationError as e:
raise HTTPException(...)| if self.start_time and self.end_time: | ||
| if self.start_time > self.end_time: | ||
| raise ValueError("The event start time must be before end time") | ||
| return self |
There was a problem hiding this comment.
nit: You don't have to do this, but maybe you could add a separate validator in case this has a repeat_start_date and repeat_end_date on the object.
| 404:{"description": "Event doesn't exist."} | ||
| }, | ||
| operation_id="delete_event", | ||
| # dependecies=[Depends()], |
There was a problem hiding this comment.
Add a site admin dependency.
| start_date: datetime.date | None = None | ||
| end_date: datetime.date | None = None | ||
|
|
||
| class EventPublic(BaseModel): |
There was a problem hiding this comment.
Returning the eid is fine here, so you can remove this model and just use Event.
2 participants
This PR implements the event feature that allows CRUD operations.
Created database schema using sqlalchemy with proper constraints.
Implement the following APIs:
GET( /event)
GET( /event/)
GET(/event//)
POST (/event)
PATCH (/event/)
DELETE (/event/)
Closes #108