Support Explicitly Excluding Locales From Smartling Sync#65
Support Explicitly Excluding Locales From Smartling Sync#65stevejalim merged 1 commit intomozilla:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new WAGTAIL_LOCALIZE_SMARTLING["EXCLUDE_LOCALES"] setting to prevent specific Wagtail locales from entering the Smartling job pipeline, and validates/uses it during job creation.
Changes:
- Introduces
EXCLUDE_LOCALESonSmartlingSettingswith validation againstWAGTAIL_CONTENT_LANGUAGES. - Filters excluded-locale translations inside
Job.get_or_create_from_source_and_translation_data()(including a no-op return when all translations are excluded). - Adds unit tests and README documentation for the new setting.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/wagtail_localize_smartling/settings.py |
Adds EXCLUDE_LOCALES setting + validation during settings initialization. |
src/wagtail_localize_smartling/models.py |
Filters out excluded locales before job lookup/creation. |
tests/test_settings.py |
Adds settings validation tests for EXCLUDE_LOCALES. |
tests/test_models.py |
Adds job-creation behavior tests for excluded locales. |
README.md |
Documents how to configure EXCLUDE_LOCALES and what it does. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| f"{setting_name}['EXCLUDE_LOCALES'] must be a list, tuple, or set " | ||
| f"of locale code strings" |
There was a problem hiding this comment.
The error message here says EXCLUDE_LOCALES must be a "list, tuple, or set", but the code also accepts a frozenset. Update the message to match the accepted types (and consider saying it must be an iterable of locale code strings).
| f"{setting_name}['EXCLUDE_LOCALES'] must be a list, tuple, or set " | |
| f"of locale code strings" | |
| f"{setting_name}['EXCLUDE_LOCALES'] must be an iterable " | |
| f"(list, tuple, set, or frozenset) of locale code strings" |
| valid_locale_codes = { | ||
| code | ||
| for code, _ in getattr( | ||
| django_settings, "WAGTAIL_CONTENT_LANGUAGES", [] | ||
| ) | ||
| } | ||
| invalid_codes = set(exclude) - valid_locale_codes | ||
| if invalid_codes: | ||
| raise ImproperlyConfigured( | ||
| f"{setting_name}['EXCLUDE_LOCALES'] contains locale codes not in " | ||
| f"WAGTAIL_CONTENT_LANGUAGES: {sorted(invalid_codes)}" | ||
| ) | ||
| settings_kwargs["EXCLUDE_LOCALES"] = frozenset(exclude) |
There was a problem hiding this comment.
invalid_codes = set(exclude) - valid_locale_codes will raise a TypeError if EXCLUDE_LOCALES contains any unhashable items (e.g. a nested list) and currently doesn't validate that each entry is a string. Consider explicitly validating each element is a str (and rejecting non-hashable values) so misconfiguration consistently raises ImproperlyConfigured with a clear message instead of crashing during settings init.
| valid_locale_codes = { | |
| code | |
| for code, _ in getattr( | |
| django_settings, "WAGTAIL_CONTENT_LANGUAGES", [] | |
| ) | |
| } | |
| invalid_codes = set(exclude) - valid_locale_codes | |
| if invalid_codes: | |
| raise ImproperlyConfigured( | |
| f"{setting_name}['EXCLUDE_LOCALES'] contains locale codes not in " | |
| f"WAGTAIL_CONTENT_LANGUAGES: {sorted(invalid_codes)}" | |
| ) | |
| settings_kwargs["EXCLUDE_LOCALES"] = frozenset(exclude) | |
| # Validate that each entry is a string so we can safely use it in a set | |
| exclude_codes: list[str] = [] | |
| for value in exclude: | |
| if not isinstance(value, str): | |
| raise ImproperlyConfigured( | |
| f"{setting_name}['EXCLUDE_LOCALES'] must contain only strings; " | |
| f"got {value!r} (type {type(value).__name__})" | |
| ) | |
| exclude_codes.append(value) | |
| valid_locale_codes = { | |
| code | |
| for code, _ in getattr( | |
| django_settings, "WAGTAIL_CONTENT_LANGUAGES", [] | |
| ) | |
| } | |
| invalid_codes = set(exclude_codes) - valid_locale_codes | |
| if invalid_codes: | |
| raise ImproperlyConfigured( | |
| f"{setting_name}['EXCLUDE_LOCALES'] contains locale codes not in " | |
| f"WAGTAIL_CONTENT_LANGUAGES: {sorted(invalid_codes)}" | |
| ) | |
| settings_kwargs["EXCLUDE_LOCALES"] = frozenset(exclude_codes) |
| excluded_translations = [t for t in translations_list if t.target_locale.language_code in excluded] | ||
| if excluded_translations: | ||
| logger.info( | ||
| "Excluding %d translation(s) for locales: %s", | ||
| len(excluded_translations), | ||
| ", ".join(t.target_locale.language_code for t in excluded_translations), | ||
| ) | ||
| translations_list = [t for t in translations_list if t.target_locale.language_code not in excluded] |
There was a problem hiding this comment.
This filtering/logging uses t.target_locale.language_code, which will trigger an extra DB query per Translation if translations is a queryset without select_related('target_locale') (e.g. job.translations.all() in the resubmit view). Consider ensuring locales are fetched in bulk (e.g. select_related when possible) or restructuring to use target_locale_id plus a single Locale lookup, to avoid N+1 queries when excluding locales.
| excluded_translations = [t for t in translations_list if t.target_locale.language_code in excluded] | |
| if excluded_translations: | |
| logger.info( | |
| "Excluding %d translation(s) for locales: %s", | |
| len(excluded_translations), | |
| ", ".join(t.target_locale.language_code for t in excluded_translations), | |
| ) | |
| translations_list = [t for t in translations_list if t.target_locale.language_code not in excluded] | |
| # Bulk-load locales to avoid N+1 queries on t.target_locale | |
| target_locale_ids = { | |
| t.target_locale_id for t in translations_list if getattr(t, "target_locale_id", None) | |
| } | |
| locales_by_id = Locale.objects.in_bulk(target_locale_ids) if target_locale_ids else {} | |
| def _is_excluded(translation: Translation) -> bool: | |
| locale = locales_by_id.get(getattr(translation, "target_locale_id", None)) | |
| return bool(locale and locale.language_code in excluded) | |
| excluded_translations = [t for t in translations_list if _is_excluded(t)] | |
| if excluded_translations: | |
| excluded_language_codes = sorted( | |
| { | |
| locales_by_id[t.target_locale_id].language_code | |
| for t in excluded_translations | |
| if getattr(t, "target_locale_id", None) in locales_by_id | |
| } | |
| ) | |
| logger.info( | |
| "Excluding %d translation(s) for locales: %s", | |
| len(excluded_translations), | |
| ", ".join(excluded_language_codes), | |
| ) | |
| translations_list = [t for t in translations_list if not _is_excluded(t)] |
stevejalim
left a comment
There was a problem hiding this comment.
Approved with comments (r+wc)
@dchukhin I'm going to merge this now, to get it out sooner rather than later, but take a look at the optimisation comments from Copilot and if you agree they're worth it, feel free to follow up with another PR.
Thanks for sorting this!
| len(excluded_translations), | ||
| ", ".join(t.target_locale.language_code for t in excluded_translations), | ||
| ) | ||
| translations_list = [t for t in translations_list if t.target_locale.language_code not in excluded] |
There was a problem hiding this comment.
Optimisation nit, nonblocking: could this be done more efficiently with a set.difference, I'm wondering
This pull request makes it possible to explicitly exclude locales from syncing to Smartling by setting
EXCLUDE_LOCALESin theWAGTAIL_LOCALIZE_SMARTLINGsetting.Any locales defined in
Job.get_or_create_from_source_and_translation_data()are excluded prior to looking for existing jobs or creating a new job.