From 0c4d201a86a481e200bd5a228889aaefa3e5d5de Mon Sep 17 00:00:00 2001 From: Krystle Salazar Date: Fri, 10 Mar 2023 15:51:18 -0400 Subject: [PATCH 1/4] Add failing test for thumbnail endpoint --- api/test/unit/views/image_views_test.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/api/test/unit/views/image_views_test.py b/api/test/unit/views/image_views_test.py index a9580f01c9e..61388210c3f 100644 --- a/api/test/unit/views/image_views_test.py +++ b/api/test/unit/views/image_views_test.py @@ -3,6 +3,9 @@ from dataclasses import dataclass from pathlib import Path from test.factory.models.image import ImageFactory +from unittest.mock import ANY, patch + +from django.http import HttpResponse import pytest from requests import Request, Response @@ -56,3 +59,18 @@ def test_oembed_sends_ua_header(api_client, requests): assert len(requests.requests) > 0 for r in requests.requests: assert r.headers == ImageViewSet.OEMBED_HEADERS + + +@pytest.mark.django_db +def test_thumbnail_uses_upstream_thumb(api_client): + image = ImageFactory.create( + url="http://example.com/image.jpg", + thumbnail="http://example.com/thumb.jpg", + provider="rawpixel", + ) + with patch("catalog.api.views.media_views.MediaViewSet.thumbnail") as thumb_call: + mock_response = HttpResponse("mock_response") + thumb_call.return_value = mock_response + api_client.get(f"/v1/images/{image.identifier}/thumb/") + + thumb_call.assert_called_once_with(image.thumbnail, ANY) From e020c5a5a2040f33249925118bf090f797a8baed Mon Sep 17 00:00:00 2001 From: Krystle Salazar Date: Fri, 10 Mar 2023 15:52:40 -0400 Subject: [PATCH 2/4] Use upstream thumbnails on specific providers --- api/catalog/api/views/image_views.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/api/catalog/api/views/image_views.py b/api/catalog/api/views/image_views.py index 05389b53270..a9cb4e5b057 100644 --- a/api/catalog/api/views/image_views.py +++ b/api/catalog/api/views/image_views.py @@ -1,5 +1,4 @@ import io -import re import struct from django.conf import settings @@ -107,17 +106,10 @@ def oembed(self, request, *_, **__): ) def thumbnail(self, request, *_, **__): image = self.get_object() - image_url = image.url - if not image_url: - raise NotFound("Could not find image.", 404) - - # Hotfix to use scaled down version of the image from SMK - # TODO Remove when this issue is addressed: - # TODO https://github.com/WordPress/openverse-catalog/issues/698 - if "iip.smk.dk" in image_url: - width = settings.THUMBNAIL_WIDTH_PX - image_url = re.sub(r"!\d+,", f"!{width},", image_url) + # TODO: Remove the Phylopic check once the thumbnails are fixed. + if image.thumbnail and image.provider != "phylopic": + image_url = image.thumbnail return super().thumbnail(image_url, request) From 90456a170868ad1a684e9c2c8a2f28aaac22bbe5 Mon Sep 17 00:00:00 2001 From: Krystle Salazar Date: Sat, 11 Mar 2023 14:36:25 -0400 Subject: [PATCH 3/4] Add test case for Phylopic --- api/justfile | 2 +- api/test/unit/views/image_views_test.py | 15 ++++++++++++--- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/api/justfile b/api/justfile index c7f132703ac..63f1599258b 100644 --- a/api/justfile +++ b/api/justfile @@ -83,7 +83,7 @@ collectstatic: ######### # Run API tests inside the Docker container -test *args: _all-up +test *args: just ../exec web ./test/run_test.sh {{ args }} # Run API tests locally diff --git a/api/test/unit/views/image_views_test.py b/api/test/unit/views/image_views_test.py index 61388210c3f..c5f1b288fd0 100644 --- a/api/test/unit/views/image_views_test.py +++ b/api/test/unit/views/image_views_test.py @@ -62,15 +62,24 @@ def test_oembed_sends_ua_header(api_client, requests): @pytest.mark.django_db -def test_thumbnail_uses_upstream_thumb(api_client): +@pytest.mark.parametrize( + "provider, expected_thumb_url", + [ + # Rawpixel is a provider with working thumbnail URLs + ("rawpixel", "http://example.com/thumb.jpg"), + # Phylopic thumbnails should be omitted until fixed + ("phylopic", "http://example.com/image.jpg"), + ], +) +def test_thumbnail_uses_upstream_thumb(api_client, provider, expected_thumb_url): image = ImageFactory.create( url="http://example.com/image.jpg", thumbnail="http://example.com/thumb.jpg", - provider="rawpixel", + provider=provider, ) with patch("catalog.api.views.media_views.MediaViewSet.thumbnail") as thumb_call: mock_response = HttpResponse("mock_response") thumb_call.return_value = mock_response api_client.get(f"/v1/images/{image.identifier}/thumb/") - thumb_call.assert_called_once_with(image.thumbnail, ANY) + thumb_call.assert_called_once_with(expected_thumb_url, ANY) From 0f38b02f0dcd2da5e30f0c99e7cfd4b21e575392 Mon Sep 17 00:00:00 2001 From: Krystle Salazar Date: Mon, 13 Mar 2023 17:39:21 -0400 Subject: [PATCH 4/4] Remove Phylopic exception since it's deactivated --- api/catalog/api/views/image_views.py | 5 +---- api/test/unit/views/image_views_test.py | 18 ++++++------------ 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/api/catalog/api/views/image_views.py b/api/catalog/api/views/image_views.py index a9cb4e5b057..eeeacabbd8a 100644 --- a/api/catalog/api/views/image_views.py +++ b/api/catalog/api/views/image_views.py @@ -106,10 +106,7 @@ def oembed(self, request, *_, **__): ) def thumbnail(self, request, *_, **__): image = self.get_object() - image_url = image.url - # TODO: Remove the Phylopic check once the thumbnails are fixed. - if image.thumbnail and image.provider != "phylopic": - image_url = image.thumbnail + image_url = image.thumbnail or image.url return super().thumbnail(image_url, request) diff --git a/api/test/unit/views/image_views_test.py b/api/test/unit/views/image_views_test.py index c5f1b288fd0..89a6845e595 100644 --- a/api/test/unit/views/image_views_test.py +++ b/api/test/unit/views/image_views_test.py @@ -63,23 +63,17 @@ def test_oembed_sends_ua_header(api_client, requests): @pytest.mark.django_db @pytest.mark.parametrize( - "provider, expected_thumb_url", - [ - # Rawpixel is a provider with working thumbnail URLs - ("rawpixel", "http://example.com/thumb.jpg"), - # Phylopic thumbnails should be omitted until fixed - ("phylopic", "http://example.com/image.jpg"), - ], + "has_thumbnail, expected_thumb_url", + [(True, "http://example.com/thumb.jpg"), (False, "http://example.com/image.jpg")], ) -def test_thumbnail_uses_upstream_thumb(api_client, provider, expected_thumb_url): +def test_thumbnail_uses_upstream_thumb(api_client, has_thumbnail, expected_thumb_url): + thumb_url = "http://example.com/thumb.jpg" if has_thumbnail else None image = ImageFactory.create( url="http://example.com/image.jpg", - thumbnail="http://example.com/thumb.jpg", - provider=provider, + thumbnail=thumb_url, ) with patch("catalog.api.views.media_views.MediaViewSet.thumbnail") as thumb_call: mock_response = HttpResponse("mock_response") thumb_call.return_value = mock_response api_client.get(f"/v1/images/{image.identifier}/thumb/") - - thumb_call.assert_called_once_with(expected_thumb_url, ANY) + thumb_call.assert_called_once_with(expected_thumb_url, ANY)