From 539629076f2f9abee6e7881b039c65ebe5e41ee3 Mon Sep 17 00:00:00 2001 From: mono Date: Sat, 13 Jun 2026 14:57:25 -0500 Subject: [PATCH] fix: address PR review issues for catalogue images - Use PNG format instead of JPEG for transparency support - Non-strict mode only resizes if image width > target width - Replace CATALOGUE_BACKGROUND_IMAGES_COLOR with CATALOGUE_BACKGROUND_IMAGES_RGBA (tuple default (0,0,0,0)) - Move all inline imports to top of test file - Add test_resize_non_strict_no_upscale test case - Add comment explaining settings.DEBUG guard for media serving --- tienda_ilusion/config/settings/base.py | 7 +++- tienda_ilusion/config/urls.py | 2 ++ tienda_ilusion/don_confiao/image_service.py | 33 +++++++---------- .../tests/test_catalogue_images.py | 35 ++++++++----------- 4 files changed, 34 insertions(+), 43 deletions(-) diff --git a/tienda_ilusion/config/settings/base.py b/tienda_ilusion/config/settings/base.py index f9fe0c8..45e9560 100644 --- a/tienda_ilusion/config/settings/base.py +++ b/tienda_ilusion/config/settings/base.py @@ -146,7 +146,12 @@ MEDIA_URL = "/media/" CATALOGUE_IMAGE_WIDTH = int(os.environ.get("CATALOGUE_IMAGE_WIDTH", "600")) CATALOGUE_IMAGE_HEIGHT = int(os.environ.get("CATALOGUE_IMAGE_HEIGHT", "600")) CATALOGUE_STRICT_DIMENSION = os.environ.get("CATALOGUE_STRICT_DIMENSION", "False").lower() in ("true", "1", "yes") -CATALOGUE_BACKGROUND_IMAGES_COLOR = os.environ.get("CATALOGUE_BACKGROUND_IMAGES_COLOR") or None +CATALOGUE_BACKGROUND_IMAGES_RGBA = os.environ.get( + "CATALOGUE_BACKGROUND_IMAGES_RGBA", "0,0,0,0" +) +# Parse RGBA string to tuple +_rgba_parts = CATALOGUE_BACKGROUND_IMAGES_RGBA.split(",") +CATALOGUE_BACKGROUND_IMAGES_RGBA = tuple(int(p.strip()) for p in _rgba_parts) CATALOGUE_MAX_UPLOAD_SIZE = int(os.environ.get("CATALOGUE_MAX_UPLOAD_SIZE", str(5 * 1024 * 1024))) # Default primary key field type diff --git a/tienda_ilusion/config/urls.py b/tienda_ilusion/config/urls.py index 29a01d1..b7cfe7e 100644 --- a/tienda_ilusion/config/urls.py +++ b/tienda_ilusion/config/urls.py @@ -35,5 +35,7 @@ urlpatterns = [ path('api/users/', include('users.urls')), ] +# Serve media files through Django only in development. +# In production/staging, media is served by the web server (nginx). if settings.DEBUG: urlpatterns += static(settings.MEDIA_URL, document_root=settings.MEDIA_ROOT) diff --git a/tienda_ilusion/don_confiao/image_service.py b/tienda_ilusion/don_confiao/image_service.py index c4600ed..9d358f4 100644 --- a/tienda_ilusion/don_confiao/image_service.py +++ b/tienda_ilusion/don_confiao/image_service.py @@ -1,5 +1,3 @@ -import os - from django.conf import settings from PIL import Image @@ -8,35 +6,28 @@ def resize_catalogue_image(image_field): width = settings.CATALOGUE_IMAGE_WIDTH height = settings.CATALOGUE_IMAGE_HEIGHT strict = settings.CATALOGUE_STRICT_DIMENSION - bg_color = settings.CATALOGUE_BACKGROUND_IMAGES_COLOR + bg_color = settings.CATALOGUE_BACKGROUND_IMAGES_RGBA img = Image.open(image_field.path) img = img.convert("RGBA") if strict: - img.thumbnail((width, height), Image.LANCZOS) - canvas = Image.new("RGBA", (width, height), (0, 0, 0, 0)) - if bg_color: - canvas = Image.new( - "RGBA", (width, height), _hex_to_rgba(bg_color) - ) + img = resize_to_fit(img, width, height) + canvas = Image.new("RGBA", (width, height), bg_color) x = (width - img.width) // 2 y = (height - img.height) // 2 canvas.paste(img, (x, y), img) img = canvas else: - new_height = int(img.height * width / img.width) - img = img.resize((width, new_height), Image.LANCZOS) + if img.width > width: + new_height = int(img.height * width / img.width) + img = img.resize((width, new_height), Image.LANCZOS) - if img.mode == "RGBA": - background = Image.new("RGB", img.size, (255, 255, 255)) - background.paste(img, mask=img.split()[3]) - img = background - - img.save(image_field.path, format="JPEG", quality=85) + img.save(image_field.path, format="PNG") -def _hex_to_rgba(hex_color): - hex_color = hex_color.lstrip("#") - r, g, b = tuple(int(hex_color[i : i + 2], 16) for i in (0, 2, 4)) - return r, g, b, 255 +def resize_to_fit(img, max_width, max_height): + ratio = min(max_width / img.width, max_height / img.height) + new_width = int(img.width * ratio) + new_height = int(img.height * ratio) + return img.resize((new_width, new_height), Image.LANCZOS) diff --git a/tienda_ilusion/don_confiao/tests/test_catalogue_images.py b/tienda_ilusion/don_confiao/tests/test_catalogue_images.py index 2d7317a..2a52db0 100644 --- a/tienda_ilusion/don_confiao/tests/test_catalogue_images.py +++ b/tienda_ilusion/don_confiao/tests/test_catalogue_images.py @@ -6,8 +6,10 @@ from django.core.files.uploadedfile import SimpleUploadedFile from django.test import override_settings from PIL import Image from rest_framework import status -from rest_framework.test import APITestCase +from rest_framework.test import APIClient, APITestCase +from rest_framework_simplejwt.tokens import RefreshToken +from ..models.catalogue_images import CatalogueImage from ..models.products import Product from .Mixins import LoginMixin @@ -32,7 +34,6 @@ class TestCatalogueImageModel(APITestCase, LoginMixin): ) def test_create_catalogue_image(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() catalogue_image = CatalogueImage.objects.create( product=self.product, image=image_file @@ -43,7 +44,6 @@ class TestCatalogueImageModel(APITestCase, LoginMixin): self.assertIsNotNone(catalogue_image.uploaded_at) def test_catalogue_image_product_relation(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() ci = CatalogueImage.objects.create( product=self.product, image=image_file @@ -51,7 +51,6 @@ class TestCatalogueImageModel(APITestCase, LoginMixin): self.assertIn(ci, self.product.catalogue_images.all()) def test_catalogue_image_str(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() ci = CatalogueImage.objects.create( product=self.product, image=image_file @@ -60,7 +59,6 @@ class TestCatalogueImageModel(APITestCase, LoginMixin): self.assertEqual(str(ci), expected) def test_cascade_delete_with_product(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() CatalogueImage.objects.create( product=self.product, image=image_file @@ -89,8 +87,6 @@ class TestCatalogueImageAPIPermissions(APITestCase, LoginMixin): email="regular@example.com", password="regularpass", ) - from rest_framework_simplejwt.tokens import RefreshToken - from rest_framework.test import APIClient refresh = RefreshToken.for_user(self.user) self.client = APIClient() self.client.credentials( @@ -109,8 +105,6 @@ class TestCatalogueImageAPIPermissions(APITestCase, LoginMixin): email="regular2@example.com", password="regularpass", ) - from rest_framework_simplejwt.tokens import RefreshToken - from rest_framework.test import APIClient refresh = RefreshToken.for_user(self.user) self.client = APIClient() self.client.credentials( @@ -122,12 +116,9 @@ class TestCatalogueImageAPIPermissions(APITestCase, LoginMixin): self.assertEqual(response.status_code, status.HTTP_200_OK) def test_delete_catalogue_image_non_admin(self): - from ..models.catalogue_images import CatalogueImage admin_user = User.objects.create_superuser( username="admin2", email="admin2@example.com", password="adminpass" ) - from rest_framework_simplejwt.tokens import RefreshToken - from rest_framework.test import APIClient admin_refresh = RefreshToken.for_user(admin_user) admin_client = APIClient() admin_client.credentials( @@ -235,7 +226,6 @@ class TestCatalogueImageAPI(APITestCase, LoginMixin): response = self.client.delete(url) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) - from ..models.catalogue_images import CatalogueImage self.assertEqual(CatalogueImage.objects.count(), 0) def test_create_catalogue_image_invalid_product(self): @@ -260,15 +250,13 @@ class TestCatalogueImageResize(APITestCase, LoginMixin): ) def _create_and_get_image_obj(self, width, height, strict=False, - bg_color=None): - from ..models.catalogue_images import CatalogueImage + bg_color=(0, 0, 0, 0)): settings_overrides = { "CATALOGUE_IMAGE_WIDTH": 600, "CATALOGUE_IMAGE_HEIGHT": 600, "CATALOGUE_STRICT_DIMENSION": strict, + "CATALOGUE_BACKGROUND_IMAGES_RGBA": bg_color, } - if bg_color is not None: - settings_overrides["CATALOGUE_BACKGROUND_IMAGES_COLOR"] = bg_color with override_settings(**settings_overrides): image_file = _create_test_image(width=width, height=height) @@ -306,6 +294,14 @@ class TestCatalogueImageResize(APITestCase, LoginMixin): self.assertEqual(img.width, 600) self.assertEqual(img.height, 600) + def test_resize_non_strict_no_upscale(self): + ci = self._create_and_get_image_obj( + width=400, height=300, strict=False + ) + with Image.open(ci.image.path) as img: + self.assertEqual(img.width, 400) + self.assertEqual(img.height, 300) + def test_resize_strict_mode_landscape(self): ci = self._create_and_get_image_obj( width=800, height=600, strict=True @@ -324,7 +320,7 @@ class TestCatalogueImageResize(APITestCase, LoginMixin): def test_resize_strict_mode_with_color_background(self): ci = self._create_and_get_image_obj( - width=800, height=600, strict=True, bg_color="#FFFFFF" + width=800, height=600, strict=True, bg_color=(255, 255, 255, 255) ) with Image.open(ci.image.path) as img: self.assertEqual(img.width, 600) @@ -357,7 +353,6 @@ class TestProductListingWithImages(APITestCase, LoginMixin): self.assertIn("catalogue_images", product) def test_product_list_catalogue_images_urls(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() CatalogueImage.objects.create( product=self.product, image=image_file @@ -376,7 +371,6 @@ class TestProductListingWithImages(APITestCase, LoginMixin): ) def test_product_list_catalogue_images_multiple(self): - from ..models.catalogue_images import CatalogueImage img1 = _create_test_image(name="img1.png") img2 = _create_test_image(name="img2.png") CatalogueImage.objects.create(product=self.product, image=img1) @@ -402,7 +396,6 @@ class TestProductListingWithImages(APITestCase, LoginMixin): self.assertEqual(product_data["catalogue_images"], []) def test_product_detail_includes_catalogue_images(self): - from ..models.catalogue_images import CatalogueImage image_file = _create_test_image() CatalogueImage.objects.create( product=self.product, image=image_file