DEV-687 Implement save calendar events#29
Conversation
ZhaoSongZh7
left a comment
ZhaoSongZh7
left a comment
There was a problem hiding this comment.
Change "unsave" to "delete" in user_saved_events_view.py. Maybe implement helper method for reducing the repetition of the error checking.
jfmath04
left a comment
jfmath04
left a comment
There was a problem hiding this comment.
I left a few comments to review
| is_saved = serializers.SerializerMethodField() | ||
|
|
||
| def get_is_saved(self, obj: Event) -> bool: | ||
| user = self.context["request"].user | ||
|
|
||
| return obj.saved_by.contains(user) | ||
|
|
There was a problem hiding this comment.
This will trigger a query per event when serializing to get whether the user saved the event. Can we do a query set annotation instead where we take the events we want to serialize, make a query to add the is_saved field into the query set, and then put the annotated query set into the serializer? This way we only make one query for all the events we want to serialize. Make sure all of the calls to the Event Serializer are updated to reflect changes (merge from preview first).
|
|
||
| try: | ||
| event = Event.objects.get(pk=validated_data["event_id"]) | ||
| event.saved_by.add(request.user) |
| from rest_framework.response import Response | ||
| from rest_framework.views import APIView | ||
|
|
||
| from hoagiecalendar.api.user_events_view import EventSerializer |
| except Event.DoesNotExist: | ||
| return Response({"error": "event not found"}, status=status.HTTP_404_NOT_FOUND) | ||
| except Exception as e: | ||
| return Response({"error": f"an error occured while saving event for user: {e}"}) |
There was a problem hiding this comment.
this needs a status 500.
also, can we leave out the actual error e and just return that there was an internal error? Write a TODO comment to log the error detail once we add logging.
| except Event.DoesNotExist: | ||
| return Response({"error": "event not found"}, status=status.HTTP_404_NOT_FOUND) | ||
| except Exception as e: | ||
| return Response({"error": f"an error occured while unsaving event for user: {e}"}) |
There was a problem hiding this comment.
this also needs a status 500.
also, can we leave out the actual error e and just return that there was an internal error? Write a TODO comment to log the error detail once we add logging.
| event = Event.objects.get(pk=validated_data["event_id"]) | ||
| event.saved_by.add(request.user) | ||
|
|
||
| return Response({"message": "event saved"}, status=status.HTTP_201_CREATED) |
There was a problem hiding this comment.
Let's return the updated event serialized here with status 200 since no new object was created.
| event = Event.objects.get(pk=validated_data["event_id"]) | ||
| event.saved_by.add(request.user) | ||
|
|
||
| return Response({"message": "event unsaved"}, status=status.HTTP_200_OK) |
There was a problem hiding this comment.
This should be status 204. Also no message needed since there is no data to return.
There was a problem hiding this comment.
Can you make a tests folder and put this in there? also delete the existing test.py file.
There was a problem hiding this comment.
That wasn't meant to be committed. I can remove it for now and then add a more thorough test case later.
| def post(self, request: Request) -> Response: | ||
| serializer = SaveEventRequestSerializer(data=request.data) | ||
| if not serializer.is_valid(): | ||
| return Response({"error": serializer.errors}, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
serializer.errors doesn't need to be nested in the dict. just put it in directly
| def delete(self, request: Request) -> Response: | ||
| serializer = SaveEventRequestSerializer(data=request.data) | ||
| if not serializer.is_valid(): | ||
| return Response({"error": serializer.errors}, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
serializer.errors doesn't need to be nested in the dict. just put it in directly
3 participants
References
Proposed Changes
saved_byas aManyToManyFieldunderEventuser_saved_events_viewfor new routes.