diff --git a/CHANGELOG.md b/CHANGELOG.md index 7672069..eadc903 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Optimisation: Only initialise view classes in `PatchWagtailURLsMiddleware` when we know they are being used as a replacement ([#36](https://github.com/torchbox/wagtail-bynder/pull/36)) @ababic +### Fixed + +- Improved handling for unexpected server error responses when downloading Bynder assets ([#40](https://github.com/torchbox/wagtail-bynder/issues/40)) + ## [0.6] - 2024-07-29 ### Added diff --git a/src/wagtail_bynder/exceptions.py b/src/wagtail_bynder/exceptions.py index 93f6161..d7f7933 100644 --- a/src/wagtail_bynder/exceptions.py +++ b/src/wagtail_bynder/exceptions.py @@ -10,3 +10,9 @@ class BynderAssetFileTooLarge(Exception): Raised when an asset file being downloaded from Bynder is found to be larger than specified in ``settings.BYNDER_MAX_DOWNLOAD_FILE_SIZE`` """ + + +class BynderAssetDownloadError(Exception): + """ + Raised when a server error occurs while downloading an asset from Bynder. + """ diff --git a/src/wagtail_bynder/management/commands/base.py b/src/wagtail_bynder/management/commands/base.py index c7c4b02..59e5a1f 100644 --- a/src/wagtail_bynder/management/commands/base.py +++ b/src/wagtail_bynder/management/commands/base.py @@ -9,6 +9,7 @@ from django.utils.translation import gettext_lazy as _ from requests import HTTPError +from wagtail_bynder.exceptions import BynderAssetDownloadError from wagtail_bynder.models import BynderAssetMixin from wagtail_bynder.utils import get_bynder_client @@ -164,8 +165,16 @@ def update_object(self, obj: BynderAssetMixin, asset_data: dict[str, Any]) -> No self.stdout.write(f" {key}: {value}") self.stdout.write("-" * 80) - obj.update_from_asset_data(asset_data) - obj.save() + try: + obj.update_from_asset_data(asset_data) + obj.save() + except BynderAssetDownloadError as e: + self.stdout.write( + f"ERROR: Failed to download asset '{asset_data['id']}': {e}\n" + ) + self.stdout.write( + f"Skipping update for {repr(obj)}. The asset will be retried on the next sync.\n" + ) class BaseBynderRefreshCommand(BaseModelCommand): @@ -244,5 +253,13 @@ def update_object(self, obj: BynderAssetMixin, asset_data: dict[str, Any]) -> No self.stdout.write( f"Updating <{self.model._meta.label}: pk='{obj.pk}' title='{obj.title}'>" # type: ignore[attr-defined] ) - obj.update_from_asset_data(asset_data, force_download=self.force_download) - obj.save() + try: + obj.update_from_asset_data(asset_data, force_download=self.force_download) + obj.save() + except BynderAssetDownloadError as e: + self.stdout.write( + f"ERROR: Failed to download asset '{asset_data['id']}': {e}\n" + ) + self.stdout.write( + f"Skipping update for {repr(obj)}. The asset will be retried on the next sync.\n" + ) diff --git a/src/wagtail_bynder/static/wagtailadmin/js/chooser-modal-handler-factory.js b/src/wagtail_bynder/static/wagtailadmin/js/chooser-modal-handler-factory.js index a70bbb3..0ffcf81 100644 --- a/src/wagtail_bynder/static/wagtailadmin/js/chooser-modal-handler-factory.js +++ b/src/wagtail_bynder/static/wagtailadmin/js/chooser-modal-handler-factory.js @@ -21,6 +21,26 @@ class BynderChooserModalOnloadHandlerFactory { modal.close(); } + onLoadErrorStep(modal, jsonData) { + // Display error message in the modal + const errorHtml = ` +
+ +
+ `; + // Insert error at the top of the modal body + $(modal.body).prepend(errorHtml); + // Re-initialize the Bynder view so user can try again + this.initBynderCompactView(modal); + } + initBynderCompactView(modal) { // NOTE: This div is added to the template: // wagtailadmin/chooser/choose-bynder.html template @@ -78,6 +98,9 @@ class BynderChooserModalOnloadHandlerFactory { [this.chosenStepName]: (modal, jsonData) => { this.onLoadChosenStep(modal, jsonData); }, + error: (modal, jsonData) => { + this.onLoadErrorStep(modal, jsonData); + }, }; } } diff --git a/src/wagtail_bynder/utils.py b/src/wagtail_bynder/utils.py index b971f49..290a76e 100644 --- a/src/wagtail_bynder/utils.py +++ b/src/wagtail_bynder/utils.py @@ -14,7 +14,7 @@ from wagtail.models import Collection from willow import Image -from .exceptions import BynderAssetFileTooLarge +from .exceptions import BynderAssetDownloadError, BynderAssetFileTooLarge _DEFAULT_COLLECTION = Local() @@ -24,11 +24,20 @@ def download_file( url: str, max_filesize: int, max_filesize_setting_name: str ) -> InMemoryUploadedFile: name = os.path.basename(url) + response = requests.get(url, timeout=20, stream=True) + + # Make sure we don't store error responses instead of the file requested + if response.status_code != 200: + raise BynderAssetDownloadError( + f"Server error downloading '{name}' from Bynder. " + ) # Stream file to memory file = BytesIO() - for line in requests.get(url, timeout=20, stream=True): - file.write(line) + for chunk in response.iter_content(chunk_size=8192): + if not chunk: + continue + file.write(chunk) if file.tell() > max_filesize: file.truncate(0) raise BynderAssetFileTooLarge( diff --git a/src/wagtail_bynder/views/document.py b/src/wagtail_bynder/views/document.py index 6697252..b217de0 100644 --- a/src/wagtail_bynder/views/document.py +++ b/src/wagtail_bynder/views/document.py @@ -3,6 +3,7 @@ from django.conf import settings from django.views.generic import UpdateView from wagtail import VERSION as WAGTAIL_VERSION +from wagtail.admin.modal_workflow import render_modal_workflow from wagtail.documents import get_document_model from wagtail.documents.views import chooser as chooser_views @@ -13,6 +14,8 @@ else: from wagtail.documents.views.documents import DeleteView, EditView +from wagtail_bynder.exceptions import BynderAssetDownloadError + from .mixins import BynderAssetCopyMixin, RedirectToBynderMixin @@ -72,10 +75,23 @@ class DocumentChosenView(BynderAssetCopyMixin, chooser_views.DocumentChosenView) def get(self, request: "HttpRequest", pk: str) -> "JsonResponse": try: - obj = self.model.objects.get(bynder_id=pk) - except self.model.DoesNotExist: - obj = self.create_object(pk) - else: - if getattr(settings, "BYNDER_SYNC_EXISTING_DOCUMENTS_ON_CHOOSE", False): - self.update_object(pk, obj) + try: + obj = self.model.objects.get(bynder_id=pk) + except self.model.DoesNotExist: + obj = self.create_object(pk) + else: + if getattr(settings, "BYNDER_SYNC_EXISTING_DOCUMENTS_ON_CHOOSE", False): + self.update_object(pk, obj) + except BynderAssetDownloadError as e: + # Return error step to display message in the chooser modal + return render_modal_workflow( + request, + None, + None, + None, + json_data={ + "step": "error", + "error_message": f"Failed to download document from Bynder: {str(e)} Please try again later.", + }, + ) return self.get_chosen_response(obj) diff --git a/src/wagtail_bynder/views/image.py b/src/wagtail_bynder/views/image.py index f07e346..5355935 100644 --- a/src/wagtail_bynder/views/image.py +++ b/src/wagtail_bynder/views/image.py @@ -3,6 +3,7 @@ from django.conf import settings from django.views.generic import UpdateView from wagtail import VERSION as WAGTAIL_VERSION +from wagtail.admin.modal_workflow import render_modal_workflow from wagtail.images import get_image_model from wagtail.images.views import chooser as chooser_views @@ -13,6 +14,8 @@ else: from wagtail.images.views.images import DeleteView, EditView +from wagtail_bynder.exceptions import BynderAssetDownloadError + from .mixins import BynderAssetCopyMixin, RedirectToBynderMixin @@ -70,10 +73,23 @@ class ImageChosenView(BynderAssetCopyMixin, chooser_views.ImageChosenView): def get(self, request: "HttpRequest", pk: str) -> "JsonResponse": try: - obj = self.model.objects.get(bynder_id=pk) - except self.model.DoesNotExist: - obj = self.create_object(pk) - else: - if getattr(settings, "BYNDER_SYNC_EXISTING_IMAGES_ON_CHOOSE", False): - self.update_object(pk, obj) + try: + obj = self.model.objects.get(bynder_id=pk) + except self.model.DoesNotExist: + obj = self.create_object(pk) + else: + if getattr(settings, "BYNDER_SYNC_EXISTING_IMAGES_ON_CHOOSE", False): + self.update_object(pk, obj) + except BynderAssetDownloadError as e: + # Return error step to display message in the chooser modal + return render_modal_workflow( + request, + None, + None, + None, + json_data={ + "step": "error", + "error_message": f"Failed to download image from Bynder: {str(e)} Please try again later.", + }, + ) return self.get_chosen_response(obj) diff --git a/src/wagtail_bynder/views/video.py b/src/wagtail_bynder/views/video.py index 55e5746..6f7b99f 100644 --- a/src/wagtail_bynder/views/video.py +++ b/src/wagtail_bynder/views/video.py @@ -3,11 +3,13 @@ from django.conf import settings from django.utils.functional import cached_property from django.utils.translation import gettext_lazy as _ +from wagtail.admin.modal_workflow import render_modal_workflow from wagtail.admin.views.generic.chooser import ChooseView from wagtail.snippets.views.chooser import SnippetChooserViewSet, SnippetChosenView from wagtail.snippets.views.snippets import EditView, SnippetViewSet from wagtail_bynder import get_video_model +from wagtail_bynder.exceptions import BynderAssetDownloadError from .mixins import BynderAssetCopyMixin, RedirectToBynderMixin @@ -34,12 +36,25 @@ def page_subtitle(self): class VideoChosenView(BynderAssetCopyMixin, SnippetChosenView): def get(self, request: "HttpRequest", pk: str) -> "JsonResponse": try: - obj = self.model.objects.get(bynder_id=pk) - except self.model.DoesNotExist: - obj = self.create_object(pk) - else: - if getattr(settings, "BYNDER_SYNC_EXISTING_VIDEOS_ON_CHOOSE", False): - self.update_object(pk, obj) + try: + obj = self.model.objects.get(bynder_id=pk) + except self.model.DoesNotExist: + obj = self.create_object(pk) + else: + if getattr(settings, "BYNDER_SYNC_EXISTING_VIDEOS_ON_CHOOSE", False): + self.update_object(pk, obj) + except BynderAssetDownloadError as e: + # Return error step to display message in the chooser modal + return render_modal_workflow( + request, + None, + None, + None, + json_data={ + "step": "error", + "error_message": f"Failed to fetch video from Bynder: {str(e)} Please try again later.", + }, + ) return self.get_chosen_response(obj) diff --git a/tests/test_image_chooser_views.py b/tests/test_image_chooser_views.py index 22d1797..f146481 100644 --- a/tests/test_image_chooser_views.py +++ b/tests/test_image_chooser_views.py @@ -5,6 +5,8 @@ from testapp.factories import CustomImageFactory from wagtail.test.utils import WagtailTestUtils +from wagtail_bynder.exceptions import BynderAssetDownloadError + from .utils import TEST_ASSET_ID @@ -106,3 +108,45 @@ def test_uses_existing_image_and_updates_it(self, update_object_mock): }, }, ) + + def test_returns_error_step_when_download_fails(self): + """Test that download errors return an error step instead of crashing""" + with mock.patch( + "wagtail_bynder.views.image.ImageChosenView.create_object", + ) as create_object_mock: + # Mock create_object to raise download error + create_object_mock.side_effect = BynderAssetDownloadError( + "Server error downloading 'test.jpg' from Bynder. " + ) + + response = self.client.get(str(self.url)) + + # Should return error step, not crash + self.assertEqual(response.status_code, 200) + response_data = response.json() + self.assertEqual(response_data["step"], "error") + self.assertIn("error_message", response_data) + self.assertIn("Server error downloading", response_data["error_message"]) + + @override_settings(BYNDER_SYNC_EXISTING_IMAGES_ON_CHOOSE=True) + def test_returns_error_step_when_update_fails(self): + """Test that download errors during update return an error step""" + # Create an image with a matching bynder_id + CustomImageFactory.create(bynder_id=TEST_ASSET_ID) + + with mock.patch( + "wagtail_bynder.views.image.ImageChosenView.update_object", + ) as update_object_mock: + # Mock update_object to raise download error + update_object_mock.side_effect = BynderAssetDownloadError( + "Server error downloading 'test.jpg' from Bynder. " + ) + + response = self.client.get(str(self.url)) + + # Should return error step, not crash + self.assertEqual(response.status_code, 200) + response_data = response.json() + self.assertEqual(response_data["step"], "error") + self.assertIn("error_message", response_data) + self.assertIn("Server error downloading", response_data["error_message"]) diff --git a/tests/test_management_commands.py b/tests/test_management_commands.py index 27beb17..e0bbed8 100644 --- a/tests/test_management_commands.py +++ b/tests/test_management_commands.py @@ -10,6 +10,7 @@ from requests import HTTPError, Response from testapp.factories import CustomDocumentFactory, CustomImageFactory, VideoFactory +from wagtail_bynder.exceptions import BynderAssetDownloadError from wagtail_bynder.management.commands.refresh_bynder_documents import ( Command as UpdateDocuments, ) @@ -399,6 +400,183 @@ class UpdateVideosTestCase(RefreshCommandTestsMixin, TestCase): factory_class = VideoFactory +class SyncCommandErrorHandlingMixin: + """ + A mixin class for testing error handling in sync commands + ('update_stale_images', 'update_stale_documents', 'update_stale_videos'). + """ + + command_name: str = "" + factory_class: Type + + @classmethod + def setUpTestData(cls): + # Create an object with old bynder_last_modified so it's considered stale + cls.test_obj = cls.factory_class( + bynder_id=TEST_ASSET_ID, + bynder_last_modified=datetime.datetime(2000, 1, 1, tzinfo=datetime.UTC), + ) + + def setUp(self): + super().setUp() + + # Define a mock to stand-in for the Bynder API client + self.mock_api_client = mock.Mock() + self.mock_api_client.media_list.return_value = [TEST_ASSET_DATA] + self.mock_api_client.media_info.return_value = TEST_ASSET_DATA + + def call_command_with_error(self, error): + """ + Calls the command with mocked API client and update_from_asset_data + that raises the specified error. + """ + stdout = StringIO() + + with ( + mock.patch( + "wagtail_bynder.management.commands.base.get_bynder_client", + return_value=mock.Mock(asset_bank_client=self.mock_api_client), + ), + mock.patch.object( + self.test_obj.__class__, "update_from_asset_data", side_effect=error + ), + ): + call_command( + self.command_name, + stdout=stdout, + stderr=StringIO(), + ) + + return stdout.getvalue() + + def test_handles_download_errors_gracefully(self): + """Test that command handles download errors without crashing""" + error = BynderAssetDownloadError( + "Server error downloading 'test.jpg' from Bynder. " + ) + output = self.call_command_with_error(error) + + # Check that error was written to stdout + self.assertIn("ERROR", output) + self.assertIn("Failed to download", output) + self.assertIn(TEST_ASSET_ID, output) + + # Check that warning about skipping was written + self.assertIn("Skipping update", output) + + +class RefreshCommandErrorHandlingMixin: + """ + A mixin class for testing error handling in refresh commands + ('refresh_bynder_images', 'refresh_bynder_documents', 'refresh_bynder_videos'). + """ + + command_name: str = "" + factory_class: Type + + @classmethod + def setUpTestData(cls): + cls.test_obj = cls.factory_class(bynder_id=TEST_ASSET_ID) + + def setUp(self): + super().setUp() + + # Define a mock to stand-in for the Bynder API client + self.mock_api_client = mock.Mock() + self.mock_api_client.media_info.return_value = TEST_ASSET_DATA + + def call_command_with_error(self, error): + """ + Calls the command with mocked API client and update_from_asset_data + that raises the specified error. + """ + stdout = StringIO() + + with ( + mock.patch( + "wagtail_bynder.management.commands.base.get_bynder_client", + return_value=mock.Mock(asset_bank_client=self.mock_api_client), + ), + mock.patch.object( + self.test_obj.__class__, "update_from_asset_data", side_effect=error + ), + ): + call_command( + self.command_name, + stdout=stdout, + stderr=StringIO(), + ) + + return stdout.getvalue() + + def test_handles_download_errors_gracefully(self): + """Test that command handles download errors without crashing""" + error = BynderAssetDownloadError( + "Server error downloading 'test.jpg' from Bynder. " + ) + output = self.call_command_with_error(error) + + # Check that error was written to stdout + self.assertIn("ERROR", output) + self.assertIn("Failed to download", output) + + +class UpdateStaleImagesErrorHandlingTestCase(SyncCommandErrorHandlingMixin, TestCase): + """ + Tests for error handling in the 'update_stale_images' management command. + """ + + command_name = "update_stale_images" + factory_class = CustomImageFactory + + +class UpdateStaleDocumentsErrorHandlingTestCase( + SyncCommandErrorHandlingMixin, TestCase +): + """ + Tests for error handling in the 'update_stale_documents' management command. + """ + + command_name = "update_stale_documents" + factory_class = CustomDocumentFactory + + +class UpdateStaleVideosErrorHandlingTestCase(SyncCommandErrorHandlingMixin, TestCase): + """ + Tests for error handling in the 'update_stale_videos' management command. + """ + + command_name = "update_stale_videos" + factory_class = VideoFactory + + +class RefreshImagesErrorHandlingTestCase(RefreshCommandErrorHandlingMixin, TestCase): + """ + Tests for error handling in the 'refresh_bynder_images' management command. + """ + + command_name = "refresh_bynder_images" + factory_class = CustomImageFactory + + +class RefreshDocumentsErrorHandlingTestCase(RefreshCommandErrorHandlingMixin, TestCase): + """ + Tests for error handling in the 'refresh_bynder_documents' management command. + """ + + command_name = "refresh_bynder_documents" + factory_class = CustomDocumentFactory + + +class RefreshVideosErrorHandlingTestCase(RefreshCommandErrorHandlingMixin, TestCase): + """ + Tests for error handling in the 'refresh_bynder_videos' management command. + """ + + command_name = "refresh_bynder_videos" + factory_class = VideoFactory + + class UpdateStaleImagesTestCase(SyncCommandTestsMixin, SimpleTestCase): """ Unit tests for the 'update_stale_images' management command. diff --git a/tests/test_models.py b/tests/test_models.py index f5276a3..c2f8ed6 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -8,7 +8,7 @@ from wagtail.images import get_image_model from wagtail_bynder import get_video_model -from wagtail_bynder.exceptions import BynderAssetDataError +from wagtail_bynder.exceptions import BynderAssetDataError, BynderAssetDownloadError from wagtail_bynder.utils import filename_from_url from .utils import ( @@ -118,6 +118,23 @@ def test_update_from_asset_data(self): self.assertEqual(self.obj.is_limited_use, self.asset_data["limited"] == 1) self.assertEqual(self.obj.is_public, self.asset_data["isPublic"] == 1) + def test_download_error_prevents_bad_file_creation(self): + """Test that server errors prevent creation of bad files""" + # Mock the requests.get to return a 502 error + mock_response = mock.Mock() + mock_response.status_code = 502 + + with ( + mock.patch("wagtail_bynder.utils.requests.get", return_value=mock_response), + self.assertRaises(BynderAssetDownloadError) as cm, + ): + self.obj.download_file(self.asset_data["original"]) + + # Verify the error message is about server error + self.assertIn("Server error downloading", str(cm.exception)) + # Verify no file was created on the object + self.assertFalse(self.obj.file) + class BynderSyncedImageTests(SimpleTestCase): def setUp(self): @@ -363,6 +380,23 @@ def test_convert_downloaded_image(self): ), ) + def test_download_error_prevents_bad_file_creation(self): + """Test that server errors prevent creation of bad files""" + # Mock the requests.get to return a 502 error + mock_response = mock.Mock() + mock_response.status_code = 502 + + with ( + mock.patch("wagtail_bynder.utils.requests.get", return_value=mock_response), + self.assertRaises(BynderAssetDownloadError) as cm, + ): + self.obj.download_file(self.asset_data["thumbnails"]["WagtailSource"]) + + # Verify the error message is about server error + self.assertIn("Server error downloading", str(cm.exception)) + # Verify no file was created on the object + self.assertFalse(self.obj.file) + class BynderSyncedVideoTests(SimpleTestCase): def setUp(self): diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 0000000..264a812 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,43 @@ +from unittest import mock + +from django.test import SimpleTestCase + +from wagtail_bynder.exceptions import BynderAssetDownloadError +from wagtail_bynder.utils import download_file + + +class DownloadFileTests(SimpleTestCase): + """Tests for the download_file function's error handling""" + + def test_download_file_raises_error_on_non_200_status(self): + """Test that download_file raises BynderAssetDownloadError for non-200 status codes""" + mock_response = mock.Mock() + mock_response.status_code = 502 + + with ( + mock.patch("wagtail_bynder.utils.requests.get", return_value=mock_response), + self.assertRaises(BynderAssetDownloadError) as cm, + ): + download_file("https://example.com/file.jpg", 5242880, "TEST_SETTING") + + # Verify error message includes filename + self.assertIn("file.jpg", str(cm.exception)) + self.assertIn("Server error downloading", str(cm.exception)) + + def test_download_file_succeeds_on_200(self): + """Test that download_file works correctly with 200 status""" + mock_response = mock.Mock() + mock_response.status_code = 200 + # Mock iter_content to return chunks + mock_response.iter_content = mock.Mock(return_value=[b"test ", b"data"]) + + with mock.patch( + "wagtail_bynder.utils.requests.get", return_value=mock_response + ): + result = download_file( + "https://example.com/good.jpg", 5242880, "TEST_SETTING" + ) + + # Should return an InMemoryUploadedFile + self.assertEqual(result.name, "good.jpg") + self.assertGreater(result.size, 0)