Feat/devltz/reviews#17
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a reviews feature for properties, including a new Reviews model + migration, review validation, and API endpoints for listing/creating reviews and exposing average rating on property reads.
Changes:
- Added
Reviewsmodel with a DB-level uniqueness constraint (one review per user per property) and corresponding migration. - Added review validators + a
ReviewsSerializer. - Added nested review routes under properties and exposed
average_ratingonPropertiesReadSerializer.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/properties/views/property_views.py | Adds review list/create + review RUD views and related permission helpers. |
| apps/properties/validators.py | Adds validators for rating range and comment length. |
| apps/properties/urls.py | Adds nested /properties/<pk>/reviews/ routes and search/ route. |
| apps/properties/serializers/reviews_serializers.py | Implements ReviewsSerializer including duplicate-review validation. |
| apps/properties/serializers/property_serializers.py | Adds average_rating field to property read serializer. |
| apps/properties/models.py | Adds the Reviews model and uniqueness constraint. |
| apps/properties/migrations/0003_reviews.py | Creates the reviews table and constraint. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class RUDReviewPropertyView(generics.RetrieveUpdateDestroyAPIView): | ||
| queryset = Properties.objects.all() | ||
| lookup_field = "pk" | ||
|
|
||
| def get_permissions(self): | ||
| if self.request.method in ["PUT", "PATCH", "DELETE"]: | ||
| return [IsAuthenticated(), IsPropertyOwner()] | ||
| return [AllowAny()] |
There was a problem hiding this comment.
RUDReviewPropertyView is wired as a review detail endpoint, but it currently queries Properties and looks up by pk, while the URL provides review_pk. This will retrieve the wrong model (or 404) and will also fail because no serializer_class/get_serializer_class is defined for this view. Update it to operate on Reviews (and filter by the parent property), align the lookup kwarg with review_pk, and use object-level permissions appropriate for review ownership (e.g., IsReviewOwner) for update/delete.
| from rest_framework import generics, status, serializers | ||
| from rest_framework.permissions import IsAuthenticated, AllowAny | ||
| from rest_framework.permissions import BasePermission | ||
| from rest_framework.exceptions import PermissionDenied |
There was a problem hiding this comment.
Unused imports (status, serializers, PermissionDenied) were introduced here. Please remove them or use them (e.g., status.HTTP_204_NO_CONTENT), since unused imports can break lint/CI and add confusion.
| from rest_framework import generics, status, serializers | |
| from rest_framework.permissions import IsAuthenticated, AllowAny | |
| from rest_framework.permissions import BasePermission | |
| from rest_framework.exceptions import PermissionDenied | |
| from rest_framework import generics | |
| from rest_framework.permissions import IsAuthenticated, AllowAny | |
| from rest_framework.permissions import BasePermission |
| @@ -75,6 +112,5 @@ def destroy(self, request, *args, **kwargs): | |||
| "message": "Delete successfull!" | |||
There was a problem hiding this comment.
Response message has a spelling error: "successfull" should be "successful".
| "message": "Delete successfull!" | |
| "message": "Delete successful!" |
| #path("search/filters/", property_views.FilterPropertyView.as_view()), | ||
| path("search/", property_views.SearchPropertyAIView.as_view()), | ||
| path("<int:pk>/reviews/", property_views.CreateListReviewPropertyView.as_view()), | ||
| path("<int:pk>/reviews/<int:review_pk>/", property_views.RUDReviewPropertyView.as_view()), |
There was a problem hiding this comment.
This route provides review_pk, but the corresponding view currently uses lookup_field = "pk" and doesn't reference review_pk, so the endpoint won't resolve the intended review. Align the view's lookup kwarg/field with this URL pattern (and ensure it queries Reviews, not Properties).
| path("<int:pk>/reviews/<int:review_pk>/", property_views.RUDReviewPropertyView.as_view()), | |
| path("<int:property_pk>/reviews/<int:pk>/", property_views.RUDReviewPropertyView.as_view()), |
| def validate(self, data): | ||
| request = self.context.get("request") | ||
| property_id = self.context.get("property_id") | ||
| if Reviews.objects.filter(user=request.user, property_id=property_id).exists(): |
There was a problem hiding this comment.
validate() enforces a unique (user, property) review unconditionally; this will also raise on updates (it finds the existing review, which may be the instance being updated). Adjust the check to only run on create (when self.instance is None) or exclude self.instance.pk from the query. Also consider deriving property_id from self.instance.property_id if it's not present in serializer context.
| if Reviews.objects.filter(user=request.user, property_id=property_id).exists(): | |
| if property_id is None and self.instance is not None: | |
| property_id = self.instance.property_id | |
| queryset = Reviews.objects.filter(user=request.user, property_id=property_id) | |
| if self.instance is not None and self.instance.pk is not None: | |
| queryset = queryset.exclude(pk=self.instance.pk) | |
| if queryset.exists(): |
| @@ -1,7 +1,10 @@ | |||
| from unittest import result | |||
There was a problem hiding this comment.
from unittest import result appears to be an accidental/unused import and can shadow local variable names (you also use result inside get_average_rating). Please remove this import.
| from unittest import result |
| images = PropertiesPhotosSerializer(many=True, read_only=True, source="photos") | ||
| average_rating = serializers.SerializerMethodField() | ||
|
|
||
| def get_average_rating(self, obj): |
There was a problem hiding this comment.
get_average_rating() performs an aggregate query per property instance. When serializing a list of properties, this will create an N+1 query pattern. Consider annotating the queryset with the average rating (or using prefetch_related + aggregation) in the view so the rating can be returned without per-row queries.
| def get_average_rating(self, obj): | |
| def get_average_rating(self, obj): | |
| if hasattr(obj, "average_rating"): | |
| return obj.average_rating |
…devltz/reviews
No description provided.