Handle download failures

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

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.
+    """

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"
+            )

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 = `
+      <div class="messages" role="status">
+        <ul>
+          <li class="error">
+            <svg class="icon icon-warning messages-icon" aria-hidden="true">
+              <use href="#icon-warning"></use>
+            </svg>
+            ${jsonData.error_message || 'An error occurred'}
+          </li>
+        </ul>
+      </div>
+    `;
+    // 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);
+      },
     };
   }
 }

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(

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"<strong>Failed to download document from Bynder:</strong> {str(e)} Please try again later.",
+                },
+            )
         return self.get_chosen_response(obj)

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"<strong>Failed to download image from Bynder:</strong> {str(e)} Please try again later.",
+                },
+            )
         return self.get_chosen_response(obj)

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"<strong>Failed to fetch video from Bynder:</strong> {str(e)} Please try again later.",
+                },
+            )
         return self.get_chosen_response(obj)
 
 

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"])