Handle download failures (#43)

alxbridge · web-flow · commit 327d83a1d58f · 2025-11-12T09:14:54.000Z
- adds handling to ensure that error responses from Bynder aren't stored as though they're asset files
- surfaces a message to editors in chooser modals if this happens
- logs any sync issue when running the management commands
diff --git 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
@@ -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
@@ -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,20 @@ 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(
+                self.style.ERROR(
+                    f"ERROR: Failed to download asset '{asset_data['id']}': {e}\n"
+                )
+            )
+            self.stdout.write(
+                self.style.WARNING(
+                    f"Skipping update for {repr(obj)}. The asset will be retried on the next sync.\n"
+                )
+            )
 
 
 class BaseBynderRefreshCommand(BaseModelCommand):
@@ -244,5 +257,17 @@ 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(
+                self.style.ERROR(
+                    f"ERROR: Failed to download asset '{asset_data['id']}': {e}\n"
+                )
+            )
+            self.stdout.write(
+                self.style.WARNING(
+                    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
@@ -21,6 +21,20 @@ class BynderChooserModalOnloadHandlerFactory {
     modal.close();
   }
 
+  onLoadErrorStep(modal, jsonData) {
+    // Display error message in the modal
+    $(modal.body).append(
+      '<div class="help-block help-critical">' +
+        '<strong>' +
+        gettext('Server Error') +
+        ': </strong>' +
+        jsonData.error_message +
+        '</div>',
+    );
+    // 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 +92,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
@@ -1,6 +1,7 @@
 import mimetypes
 import os
 
+from http import HTTPStatus
 from io import BytesIO
 
 import requests
@@ -14,7 +15,7 @@
 from wagtail.models import Collection
 from willow import Image
 
-from .exceptions import BynderAssetFileTooLarge
+from .exceptions import BynderAssetDownloadError, BynderAssetFileTooLarge
 
 
 _DEFAULT_COLLECTION = Local()
@@ -24,11 +25,21 @@ 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 != HTTPStatus.OK:
+        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)
+    # Stream the file to memory. We use iter_content() instead of the default iterator for requests.Response,
+    # as the latter uses iter_lines() which isn't suitable for streaming binary data.
+    for chunk in response.iter_content():
+        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
@@ -1,8 +1,10 @@
 from typing import TYPE_CHECKING
 
 from django.conf import settings
+from django.utils.html import format_html
 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 +15,8 @@
 else:
     from wagtail.documents.views.documents import DeleteView, EditView
 
+from wagtail_bynder.exceptions import BynderAssetDownloadError
+
 from .mixins import BynderAssetCopyMixin, RedirectToBynderMixin
 
 
@@ -72,10 +76,26 @@ 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": format_html(
+                        "<strong>Failed to download document from Bynder:</strong> {error} Please try again later.",
+                        error=e,
+                    ),
+                },
+            )
         return self.get_chosen_response(obj)
diff --git a/src/wagtail_bynder/views/image.py b/src/wagtail_bynder/views/image.py
@@ -1,8 +1,10 @@
 from typing import TYPE_CHECKING
 
 from django.conf import settings
+from django.utils.html import format_html
 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 +15,8 @@
 else:
     from wagtail.images.views.images import DeleteView, EditView
 
+from wagtail_bynder.exceptions import BynderAssetDownloadError
+
 from .mixins import BynderAssetCopyMixin, RedirectToBynderMixin
 
 
@@ -70,10 +74,26 @@ 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": format_html(
+                        "<strong>Failed to download image from Bynder:</strong> {error} Please try again later.",
+                        error=e,
+                    ),
+                },
+            )
         return self.get_chosen_response(obj)
diff --git a/src/wagtail_bynder/views/video.py b/src/wagtail_bynder/views/video.py
@@ -2,12 +2,15 @@
 
 from django.conf import settings
 from django.utils.functional import cached_property
+from django.utils.html import format_html
 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 +37,28 @@ 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": format_html(
+                        "<strong>Failed to fetch video from Bynder:</strong> {error} Please try again later.",
+                        error=e,
+                    ),
+                },
+            )
         return self.get_chosen_response(obj)
 
 
diff --git 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
diff --git a/tests/test_models.py b/tests/test_models.py
diff --git a/tests/test_utils.py b/tests/test_utils.py
