implemented car listing api#14
Conversation
|
|
||
|
|
||
| class Car(TimeStampedModel): | ||
| model = models.ForeignKey(CarModel, on_delete=models.PROTECT, related_name='cars') |
There was a problem hiding this comment.
Can we separate out or make blocks of related fields for readability ?
|
|
||
|
|
||
| class Car(TimeStampedModel): | ||
| model = models.ForeignKey(CarModel, on_delete=models.PROTECT, related_name='cars') |
There was a problem hiding this comment.
Because Protect prevents accidental data loss: you can’t delete a model while any Car references it. If we used CASCADE, deleting a model would delete all linked Car rows to null. And which will create the confusion regarding car model
| class Car(TimeStampedModel): | ||
| model = models.ForeignKey(CarModel, on_delete=models.PROTECT, related_name='cars') | ||
| seller = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name='cars') | ||
| year = models.IntegerField() |
| price = models.DecimalField(max_digits=12, decimal_places=2) | ||
| condition = models.CharField(max_length=20, choices=CAR_CONDITION_CHOICES) | ||
| description = models.TextField(blank=True) | ||
| is_deleted = models.BooleanField(default=False) |
There was a problem hiding this comment.
Use is_active for soft delete. This improvement was highlighted previously as well.
| indexes = [ | ||
| models.Index(fields=['seller_id']), | ||
| models.Index(fields=['is_deleted']), | ||
| models.Index(fields=['model_id']), | ||
| ] |
There was a problem hiding this comment.
Why we have decided these specific indexes?
There was a problem hiding this comment.
These indexes were added based on expected query patterns seller_id for seller-based filtering, model_id for model-based searches/filtering, and is_active because soft delete filtering is frequently used.
| is_deleted = models.BooleanField(default=False) | ||
|
|
||
| class Meta: | ||
| db_table = 'cars' |
| car = models.ForeignKey(Car, on_delete=models.CASCADE, related_name='images') | ||
| image = models.ImageField(upload_to='cars/images/') | ||
| is_primary = models.BooleanField(default=False) | ||
| display_order = models.IntegerField(default=0) |
There was a problem hiding this comment.
should not it be auto-generated but user can edit if want?
| class CarQuerysetMixin: | ||
| def get_base_queryset(self): | ||
| return ( | ||
| Car.objects.filter(is_deleted=False) | ||
| .select_related('model__brand', 'seller') | ||
| .prefetch_related('images') | ||
| ) |
There was a problem hiding this comment.
Try to create Soft delete mixin for reusable field in models/
And create custom manager for filtering query set
| @@ -0,0 +1,6 @@ | |||
| CAR_CONDITION_CHOICES = [ | |||
|
|
||
| @admin.register(Car) | ||
| class CarAdmin(admin.ModelAdmin): | ||
| list_display = ['id', 'model', 'seller', 'year', 'license_plate', 'city', 'price', 'condition', 'is_deleted', 'created', 'modified'] |
There was a problem hiding this comment.
look into pep8 for improving readability here. look for how we can divide the list elements
| def with_inactive(self): | ||
| return super().get_queryset() | ||
|
|
||
| def only_inactive(self): | ||
| return super().get_queryset().filter(is_active=False) |
There was a problem hiding this comment.
Why we have created these methods when we are not using them anywhere?
| from django.db import models | ||
|
|
||
| from common.managers import ActiveQuerySet | ||
|
|
There was a problem hiding this comment.
Can we use name "core" instead of common ?
| def current_year_plus_one(): | ||
| return datetime.date.today().year + 1 | ||
|
|
||
|
|
||
| def validate_car_year(value): | ||
| current_year = datetime.date.today().year | ||
| if value < 1886 or value > current_year + 1: | ||
| raise ValidationError(f"Year must be between 1886 and {current_year + 1}.") |
There was a problem hiding this comment.
Why we have these methods in models.py ?
|
|
||
| year = models.IntegerField( | ||
| validators=[ | ||
| MinValueValidator(1886), | ||
| MaxValueValidator(current_year_plus_one), | ||
| ] | ||
| ) | ||
|
|
||
| license_plate = models.CharField(max_length=20, unique=True) | ||
|
|
||
| city = models.CharField(max_length=100) |
| filterset_class = CarFilter | ||
|
|
||
| def get_queryset(self): | ||
| return Car.objects.with_related() |
There was a problem hiding this comment.
use proper naming instead of with_related
Renamed the common package to core and updated imports across the app. Extracted car year validation into a new cars/validators.py (validate_car_year) and replaced inline datetime/validator logic in the Car model. Switched imports to core.managers/core.model, renamed CarQuerySet.with_related to with_car_relations and removed the with_inactive/only_inactive helpers, and updated views to call the new queryset helper methods. These changes centralize shared utilities, enable validator reuse, and simplify the queryset API.
Introduce a SoftDeleteMixin and remove legacy core model/manager implementations. Deleted core/managers.py and core/model.py and added core/mixins.py with SoftDeleteMixin (adds is_active and soft_delete()). Update cars to use the new mixin: Car now inherits SoftDeleteMixin, CarQuerySet no longer depends on the removed ActiveQuerySet (uses models.QuerySet), and views now call with_car_relations() and car.soft_delete() instead of manual is_active toggling or with_optimized_queries(). This simplifies soft-delete behavior and decouples car query logic from the old core manager utilities.
| import datetime | ||
| from django.core.exceptions import ValidationError | ||
|
|
||
| def validate_car_year(value): |
There was a problem hiding this comment.
Why we are not using serializer for it ?
Move year validation from a separate validator into CarCreateUpdateSerializer (validate_year) and remove cars/validators.py. Rename ImagesSerializer to ImageSerializer and update usages. Simplify and reformat filter and model field declarations for readability. Adjust permissions and view behavior: use class-level queryset and permission_classes (IsAuthenticatedOrReadOnly where appropriate), replace destroy override with perform_destroy calling soft_delete, and add proper object-permission checks for car image uploads (get_car + check_object_permissions). Minor cleanup in permissions and core mixin (whitespace/import formatting and safer user auth check).
2 participants
No description provided.