Wagtail/Smartling robustness improvements (#15699)

* Fix docs to provide better example of how to download CMS media

* Upgrade to 0.2.0 release of our fork of wagtaildraftsharing

* Update our visual context callback to be more robust.

Ensure the visual context sent to Smartling is always with a fresh
sharing link with no expiry

By default, these now last 28 days, which might be enough, but we don't
want to take the risk, given how fiddly the cleanup is of an expired
link causing initial sync to fail

* Ensure our wrapper for the sync_smartling management command logs to Sentry

It already has SENTRY_DSN available to it

* Add more Sentry logging to our visual context callback

And also make it more robust - preferring to get strings to Smartling
without a visual context, rather than fail altogether

* Add comment about settings name to avoid wasted checking time

* Upgrade to wagtail-localize-smartling 0.8 with async batch upload

This will get us around the File Locked issue we saw in prod

* Expand test coverage

bedrock/cms/management/commands/run_smartling_sync.py

@@ -9,6 +9,7 @@
 from django.core.management.base import BaseCommand
 
 import requests
+from sentry_sdk import capture_exception
 
 from bedrock.base.config_manager import config
 
@@ -32,3 +33,4 @@ def handle(self, *args, **kwargs):
                 sys.stdout.write("Snitch pinged\n")
         except Exception as ex:
             sys.stderr.write(f"\nsync_smartling did not execute successfully: {ex}\n")
+            capture_exception(ex)

bedrock/cms/tests/test_callbacks.py

@@ -11,7 +11,7 @@
 from wagtaildraftsharing.models import WagtaildraftsharingLink
 
 from bedrock.cms.tests.factories import SimpleRichTextPageFactory, WagtailUserFactory
-from bedrock.cms.wagtail_localize_smartling.callbacks import visual_context
+from bedrock.cms.wagtail_localize_smartling.callbacks import _get_html_for_sharing_link, visual_context
 
 
 @pytest.mark.django_db
@@ -54,4 +54,53 @@ def test_visual_context__for_inviable_object(client):
     with pytest.raises(IncapableVisualContextCallback) as exc:
         url, html = visual_context(smartling_job=mock_job)
 
-    assert exc.value.args[0] == "Object was not visually previewable"
+    assert exc.value.args[0] == "Object was not visually previewable (i.e. not a Page)"
+
+
+@pytest.mark.django_db
+@mock.patch("bedrock.cms.wagtail_localize_smartling.callbacks.capture_message")
+def test_visual_context__for_page__with_no_revision(mock_capture_message, client):
+    top_level_page = SimpleRichTextPageFactory()
+
+    page = SimpleRichTextPageFactory(parent=top_level_page, slug="visual-context-text-page")
+    page.save_revision()
+
+    wagtail_factories.SiteFactory(
+        root_page=top_level_page,
+        is_default_site=True,
+        hostname=client._base_environ()["SERVER_NAME"],
+    )
+
+    user = WagtailUserFactory()
+
+    mock_job = mock.Mock()
+    mock_job.translation_source.get_source_instance.return_value = page
+    mock_job.user = user
+
+    page.latest_revision = None
+    page.save()
+    with pytest.raises(IncapableVisualContextCallback) as ctx:
+        visual_context(smartling_job=mock_job)
+
+    assert ctx.value.args[0] == (
+        "Object was not visually previewable because it didn't have a saved revision. Are you a developer with a local export?"
+    )
+    mock_capture_message.assert_called_once_with(f"Unable to get a latest_revision for {page} so unable to send visual context.")
+
+
+# The happy path is implicitly tested in test_visual_context__*, above
+@mock.patch("bedrock.cms.wagtail_localize_smartling.callbacks.capture_exception")
+@mock.patch("bedrock.cms.wagtail_localize_smartling.callbacks.SharingLinkView.as_view")
+def test__get_html_for_sharing_link__unhappy_path(mock_sharing_link_as_view, mock_capture_exception):
+    test_exception = Exception("Boom!")
+    mock_sharing_link_as_view.return_value = mock.Mock(side_effect=test_exception)
+
+    sharing_link = mock.Mock()
+    sharing_link.url = "https://example.com/1231313211/link"
+
+    with pytest.raises(IncapableVisualContextCallback) as ctx:
+        _get_html_for_sharing_link(sharing_link)
+
+    assert ctx.value.args[0] == "Was not able to get a HTML export from the sharing link"
+
+    mock_capture_exception.assert_called_once_with(test_exception)

bedrock/cms/tests/test_commands.py

@@ -7,7 +7,7 @@
 
 from django.contrib.auth.models import User
 from django.core.management import call_command
-from django.test import TransactionTestCase
+from django.test import TestCase, TransactionTestCase
 
 import everett
 
@@ -175,3 +175,24 @@ def test_multiple_emails_available_but_exist_already_somehow(self, mock_write):
                 call("User [email protected] already exists - not creating\n"),
             ],
         )
+
+
+class SmartlingSyncTests(TestCase):
+    @patch("bedrock.cms.management.commands.run_smartling_sync.call_command")
+    def test_sentry_logging_for_run_smartling_sync_command(self, mock_call_command):
+        test_exception = Exception("Boom!")
+        mock_call_command.side_effect = test_exception
+        with patch("bedrock.cms.management.commands.run_smartling_sync.capture_exception") as mock_capture_exception:
+            call_command("run_smartling_sync")
+        mock_capture_exception.assert_called_once_with(test_exception)
+
+    @patch("bedrock.cms.management.commands.bootstrap_local_admin.sys.stderr.write")
+    @patch("bedrock.cms.management.commands.run_smartling_sync.call_command")
+    def test_error_messaging_for_run_smartling_sync_command(self, mock_call_command, mock_stderr_write):
+        test_exception = Exception("Boom!")
+        mock_call_command.side_effect = test_exception
+        call_command("run_smartling_sync")
+
+        expected_output = "\nsync_smartling did not execute successfully: Boom!\n"
+        output = mock_stderr_write.call_args_list[0][0][0]
+        self.assertEqual(output, expected_output)

bedrock/cms/wagtail_localize_smartling/callbacks.py

@@ -18,19 +18,24 @@
 
 if TYPE_CHECKING:
     from wagtail_localize_smartling.models import Job
-
+from sentry_sdk import capture_exception, capture_message
 from wagtaildraftsharing.models import WagtaildraftsharingLink
 from wagtaildraftsharing.views import SharingLinkView
 
 
 def _get_html_for_sharing_link(sharing_link: WagtaildraftsharingLink) -> str:
     request = RequestFactory().get(sharing_link.url)
     view_func = SharingLinkView.as_view()
-    resp = view_func(
-        request=request,
-        key=sharing_link.key,
-    )
-    return resp.content.decode("utf-8")
+    try:
+        resp = view_func(
+            request=request,
+            key=sharing_link.key,
+        )
+        return resp.content.decode("utf-8")
+    except Exception as ex:
+        # Ensure Sentry gets any problem with turning the sharing link into HTML
+        capture_exception(ex)
+        raise IncapableVisualContextCallback("Was not able to get a HTML export from the sharing link")
 
 
 def _get_full_url_for_sharing_link(sharing_link: WagtaildraftsharingLink, page: "Page") -> str:
@@ -49,16 +54,25 @@ def visual_context(smartling_job: "Job") -> tuple[str, str]:
         # We can currently only supply visual context for Pages, but not for
         # other things like Snippets, so return early and show there's nothing
         # to be processed
-        raise IncapableVisualContextCallback("Object was not visually previewable")
+        raise IncapableVisualContextCallback("Object was not visually previewable (i.e. not a Page)")
 
     revision = content_obj.latest_revision
-
-    sharing_link = WagtaildraftsharingLink.objects.get_or_create_for_revision(
+    if revision is None:
+        # The only time we'll have a situation like this is when someone is using
+        # a DB export from dev/stage/prod, which has all of its revision history
+        # excluded from the export.
+        capture_message(f"Unable to get a latest_revision for {content_obj} so unable to send visual context.")
+        raise IncapableVisualContextCallback(
+            "Object was not visually previewable because it didn't have a saved revision. Are you a developer with a local export?"
+        )
+
+    # Always use `create_for_revision` to ensure max lifespan of the link
+    sharing_link = WagtaildraftsharingLink.objects.create_for_revision(
         revision=revision,
         user=smartling_job.user,
+        max_ttl=-1,  # -1 signifies "No expiry". If we pass None we get the default TTL
     )
 
     url = _get_full_url_for_sharing_link(sharing_link=sharing_link, page=content_obj)
     html = _get_html_for_sharing_link(sharing_link=sharing_link)
-
     return (url, html)

bedrock/settings/base.py

@@ -2325,7 +2325,7 @@ def lazy_wagtail_langs():
 WAGTAIL_CONTENT_LANGUAGES = lazy(lazy_wagtail_langs, list)()
 
 # Don't automatically make a page for a non-default locale availble in the default locale
-WAGTAILLOCALIZE_SYNC_LIVE_STATUS_ON_TRANSLATE = False
+WAGTAILLOCALIZE_SYNC_LIVE_STATUS_ON_TRANSLATE = False  # note that WAGTAILLOCALIZE is correct without the _
 
 # Settings for https://github.com/mozilla/wagtail-localize-smartling
 WAGTAIL_LOCALIZE_SMARTLING = {
@@ -2366,16 +2366,14 @@ def lazy_wagtail_langs():
     "VISUAL_CONTEXT_CALLBACK": "bedrock.cms.wagtail_localize_smartling.callbacks.visual_context",
 }
 
-WAGTAIL_DRAFTSHARING_ADMIN_MENU_POSITION = 9000
-# WAGTAIL_DRAFTSHARING_VERBOSE_NAME = "Internal Share"
-# WAGTAIL_DRAFTSHARING_VERBOSE_NAME_PLURAL = "Internal Shares"
-# WAGTAIL_DRAFTSHARING_MENU_ITEM_LABEL = "Create internal sharing link"
+WAGTAILDRAFTSHARING = {
+    "ADMIN_MENU_POSITION": 9000,
+    # MAX_TTL: 14 * 24 * 60 * 60
+    # VERBOSE_NAME: "Internal Share"
+    # VERBOSE_NAME_PLURAL: "Internal Shares"
+    # MENU_ITEM_LABEL: "Create internal sharing link"
+}
 
-# At the moment, wagtaildraftsharing's expiry is only set via settings. We're using
-# 21 days here because the links are used to send a preview of the source page to
-# Smartling translators, and we want to give them plenty of time. In the future
-# we can bring this back down to 7 days and set a Smartling-specific one of 21
-WAGTAILDRAFTSHARING_MAX_AGE = 21 * 24 * 60 * 60
 
 # Custom settings, not a core Wagtail ones, to scope out RichText options
 WAGTAIL_RICHTEXT_FEATURES_FULL = [

docs/cms.rst

@@ -109,7 +109,8 @@ sync down any images you don't already have.
   or ``AWS_DB_S3_BUCKET=bedrock-db-prod``.
 
   ``AWS_DB_S3_BUCKET=bedrock-db-stage make preflight``
-  ``AWS_DB_S3_BUCKET=bedrock-db-stage python manage.py download_media_to_local``
+
+  ``python manage.py download_media_to_local --environment=stage``
 
 Adding new content surfaces
 ===========================

requirements/dev.txt

@@ -2306,12 +2306,12 @@ wagtail-localize==1.10 \
     # via
     #   -r requirements/prod.txt
     #   wagtail-localize-smartling
-wagtail-localize-smartling==0.7.0 \
-    --hash=sha256:b9551683f5865dce45576ebfc00b5ad8eba3589afce231f74c4b57d62494be17 \
-    --hash=sha256:d9fe19ae24b025b773a27b6f43ac891b836b3e36e032271f6005c8e580cbfd19
+wagtail-localize-smartling==0.8.0 \
+    --hash=sha256:9a4842e51805614f22a12cb46f2f35379993680c71c66ca391a8068aa17ef210 \
+    --hash=sha256:e6ca356ce2310344a288cc1e85c1a280e696064d9c3a4559f0a2d12704083477
     # via -r requirements/prod.txt
-wagtaildraftsharing @ https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.1.3.tar.gz#egg=wagtaildraftsharing \
-    --hash=sha256:3ffe076b0c3b99e71bd1f0d9a02831f4413577f60b38fc258633fc3602b8409d
+wagtaildraftsharing @ https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.2.0.tar.gz#egg=wagtaildraftsharing \
+    --hash=sha256:7a3399c4d621628e6458f1ee3236b7d1d5fdd9577ee385d52e69edf7379f7562
     # via -r requirements/prod.txt
 wand==0.6.13 \
     --hash=sha256:e5dda0ac2204a40c29ef5c4cb310770c95d3d05c37b1379e69c94ea79d7d19c0 \

requirements/prod.in

@@ -56,8 +56,8 @@ sentry-processor==0.0.1
 sentry-sdk==2.18.0
 supervisor==4.2.5
 timeago==1.0.16
-https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.1.3.tar.gz#egg=wagtaildraftsharing
-wagtail-localize-smartling==0.7.0
+https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.2.0.tar.gz#egg=wagtaildraftsharing
+wagtail-localize-smartling==0.8.0
 wagtail-localize==1.10
 Wand==0.6.13  # For animated GIF support
 Wagtail==6.1.3

requirements/prod.txt

@@ -1627,12 +1627,12 @@ wagtail-localize==1.10 \
     # via
     #   -r requirements/prod.in
     #   wagtail-localize-smartling
-wagtail-localize-smartling==0.7.0 \
-    --hash=sha256:b9551683f5865dce45576ebfc00b5ad8eba3589afce231f74c4b57d62494be17 \
-    --hash=sha256:d9fe19ae24b025b773a27b6f43ac891b836b3e36e032271f6005c8e580cbfd19
+wagtail-localize-smartling==0.8.0 \
+    --hash=sha256:9a4842e51805614f22a12cb46f2f35379993680c71c66ca391a8068aa17ef210 \
+    --hash=sha256:e6ca356ce2310344a288cc1e85c1a280e696064d9c3a4559f0a2d12704083477
     # via -r requirements/prod.in
-wagtaildraftsharing @ https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.1.3.tar.gz#egg=wagtaildraftsharing \
-    --hash=sha256:3ffe076b0c3b99e71bd1f0d9a02831f4413577f60b38fc258633fc3602b8409d
+wagtaildraftsharing @ https://github.com/mozmeao/wagtaildraftsharing/archive/refs/tags/mozmeao-0.2.0.tar.gz#egg=wagtaildraftsharing \
+    --hash=sha256:7a3399c4d621628e6458f1ee3236b7d1d5fdd9577ee385d52e69edf7379f7562
     # via -r requirements/prod.in
 wand==0.6.13 \
     --hash=sha256:e5dda0ac2204a40c29ef5c4cb310770c95d3d05c37b1379e69c94ea79d7d19c0 \