-
Notifications
You must be signed in to change notification settings - Fork 918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wagtail/Smartling robustness improvements #15699
Changes from 7 commits
7a07b2c
931cea2
e531f14
7a3a15b
440df9f
1140549
c456d65
e30f569
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 test3@mozilla.com 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!") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💣 |
||
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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is very broad, but it's important to cast a wide net for the sentry logging |
||
# 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.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not seeing where this variable is used? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you mean |
||
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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 _ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this not use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, exactly that. |
||
|
||
# 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" | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change in settings config to a dict is a breaking change introduced in Wagtaildraftsharing 0.8.0 |
||
|
||
# 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
# Custom settings, not a core Wagtail ones, to scope out RichText options | ||
WAGTAIL_RICHTEXT_FEATURES_FULL = [ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure we get as much info as possible if the sync isn't happy for some reason