Wagtail/Smartling robustness improvements #15699

stevejalim · 2024-12-09T15:22:45Z

One-line summary

This changeset makes our Smartling interactions more robust and provides more logging to help us diagnose any further teething troubles with its use in production.

Significant changes and points to review

Update wagtaildraftsharing to 0.2.0
This remedies an issue where it was possible to ask for a sharing link for a page, only to be given one that has expired (and which then blows up the code that's suppose to provide a HTML dump of the page to Smartling).
We now use a new method create_for_revision, which never gets a duplicate. We also make that sharing link one that never expires, rather than second-guess how long a translator might need to translate a page. (These links can still be deleted, so we could add a cleanup job every few months)
Update to wagtail-localize-smartling 0.80
This remedies the other main error in the export run that failed: we got a File locked error from the Smartling API because we were trying to make a call related to a file that hadn't completed processing. The solution to this was to refactor the file-upload part of wagtail-localize-smartling to use a "Job Batch", which handles file uploads asynchronously and transparently. I've tested this locally against our Smartling Sandbox and it definitely works.
Extra logging added

Issue / Bugzilla link

Resolves #15629

Testing

This is tricky to test without local setup. I've tested it a fair amount myself as part of developing the new release of wagtaildraftsharing and wagtail-localize-smartling, and I'll test-drive it again on Dev before we take it to prod. So, code-level eyes and sense checking would be great, but no need to test drive I think.

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

codecov · 2024-12-09T16:22:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.06%. Comparing base (3d32a7e) to head (e30f569).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15699      +/-   ##
==========================================
+ Coverage   79.04%   79.06%   +0.01%     
==========================================
  Files         159      160       +1     
  Lines        8329     8355      +26     
==========================================
+ Hits         6584     6606      +22     
- Misses       1745     1749       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…Sentry It already has SENTRY_DSN available to it

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

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

stevejalim · 2024-12-09T15:35:29Z

bedrock/settings/base.py

-# 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


This now defaults to 28 days for our fork of wagtaildraftsharing https://github.com/mozmeao/wagtaildraftsharing/blob/768b989adc304c43476f2ca55b655b7940378999/wagtaildraftsharing/settings.py#L7

...while creating links for using Smartling's visual context now has a max_ttl of -1, meaning those links never expire (see the changes in base.py above)

stevejalim · 2024-12-09T16:15:37Z

bedrock/settings/base.py

+    # VERBOSE_NAME: "Internal Share"
+    # VERBOSE_NAME_PLURAL: "Internal Shares"
+    # MENU_ITEM_LABEL: "Create internal sharing link"
+}


This change in settings config to a dict is a breaking change introduced in Wagtaildraftsharing 0.8.0

stevejalim · 2024-12-09T16:18:13Z

bedrock/cms/management/commands/run_smartling_sync.py

@@ -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)


Making sure we get as much info as possible if the sync isn't happy for some reason

stevejalim · 2024-12-09T16:23:40Z

bedrock/cms/wagtail_localize_smartling/callbacks.py

+            key=sharing_link.key,
+        )
+        return resp.content.decode("utf-8")
+    except Exception as ex:


I know this is very broad, but it's important to cast a wide net for the sentry logging

stevejalim · 2024-12-09T16:38:56Z

Tagging you @alexgibson just for awareness - I know you're too busy to review, and wanted to keep you in the loop.

robhudson · 2024-12-09T19:40:14Z

bedrock/cms/tests/test_commands.py

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


robhudson · 2024-12-09T19:42:30Z

bedrock/cms/wagtail_localize_smartling/callbacks.py

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


I'm not seeing where this variable is used?

If you mean content_obj, that's the Wagtail Page or Snippet that's being translated - we get it up on line 51 (just above this diff, folded away)

robhudson · 2024-12-09T19:43:36Z

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 _


Why does this not use the _ but the one below does? Separate libraries?

Yeah, exactly that. wagtail-localize uses WAGTAILLOCALIZE_FOO (and my eyes cross when seeing those LL together), while wagtail-localize-smartling uses WAGTAIL_LOCALIZE_SMARTLING

robhudson

code level review looks good. I asked a few questions just out of curiosity.

stevejalim added 3 commits December 9, 2024 15:11

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

7a07b2c

Upgrade to 0.2.0 release of our fork of wagtaildraftsharing

931cea2

stevejalim force-pushed the 15629--smartling-related-fixups branch from 5f58938 to 47eedf9 Compare December 9, 2024 16:14

stevejalim added 4 commits December 9, 2024 16:22

Ensure our wrapper for the sync_smartling management command logs to …

7a3a15b

…Sentry It already has SENTRY_DSN available to it

Add more Sentry logging to our visual context callback

440df9f

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

1140549

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

c456d65

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

stevejalim force-pushed the 15629--smartling-related-fixups branch from 47eedf9 to c456d65 Compare December 9, 2024 16:22

stevejalim marked this pull request as ready for review December 9, 2024 16:32

stevejalim requested a review from a team as a code owner December 9, 2024 16:32

stevejalim commented Dec 9, 2024

View reviewed changes

robhudson reviewed Dec 9, 2024

View reviewed changes

robhudson approved these changes Dec 9, 2024

View reviewed changes

Expand test coverage

e30f569

stevejalim merged commit 99c0ca2 into main Dec 9, 2024
5 checks passed

stevejalim deleted the 15629--smartling-related-fixups branch December 9, 2024 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wagtail/Smartling robustness improvements #15699

Wagtail/Smartling robustness improvements #15699

stevejalim commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading

stevejalim Dec 9, 2024

stevejalim Dec 9, 2024

stevejalim Dec 9, 2024

stevejalim Dec 9, 2024

stevejalim commented Dec 9, 2024

robhudson Dec 9, 2024

robhudson Dec 9, 2024

stevejalim Dec 9, 2024

robhudson Dec 9, 2024

stevejalim Dec 9, 2024

robhudson left a comment

Wagtail/Smartling robustness improvements #15699

Wagtail/Smartling robustness improvements #15699

Conversation

stevejalim commented Dec 9, 2024 • edited Loading

One-line summary

Significant changes and points to review

Issue / Bugzilla link

Testing

codecov bot commented Dec 9, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevejalim commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robhudson left a comment

Choose a reason for hiding this comment

stevejalim commented Dec 9, 2024 •

edited

Loading

codecov bot commented Dec 9, 2024 •

edited

Loading