Skip to content
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

Add wagtail-localize-smartling to the project, for Smartling L10N support #14794

Merged
merged 5 commits into from
Jul 22, 2024

Conversation

stevejalim
Copy link
Collaborator

@stevejalim stevejalim commented Jul 8, 2024

One-line summary

This changeset drops in wagtail-localize-smartling, which will flow strings from Wagtail to Smartling for L10N and then back again.

Significant changes and points to review

0.2.3 is an early-ish, Minimum Lovable Product version, and there may well be some fast-follow changes from the vendor - those are in discussion at the mo, and we can pick up anything else we want, as it's "our" codebase now.

I've tested it manually, locally, for now, with sandbox-y API keys and it works for the core cases. There are some rough edges but we'll sort them out.

The infra PR needed to accompany these changes will need to:

  1. Set the appropriate env vars as documented
  2. Set a cron job that runs ./manage.py sync_smartling every X minutes (15?). Doing it as a separate job on a cron means we don't risk timeouts in the web UI.

Issue / Bugzilla link

Resolves #14774

Testing

N/A for now - I'll record a video demo, but waiting to see if there'll be a 0.2.4 soon (or not yet)

@stevejalim stevejalim requested review from robhudson, alexgibson and pmac and removed request for alexgibson July 8, 2024 12:43
@stevejalim
Copy link
Collaborator Author

Review tagging is more about awareness than close reviewing right now. Good to get this merged (when the infra is ready)

Copy link

codecov bot commented Jul 8, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 77.38%. Comparing base (fe9144d) to head (ed6edbb).
Report is 5 commits behind head on main.

Files Patch % Lines
bedrock/settings/base.py 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14794      +/-   ##
==========================================
+ Coverage   77.29%   77.38%   +0.09%     
==========================================
  Files         160      161       +1     
  Lines        8281     8306      +25     
==========================================
+ Hits         6401     6428      +27     
+ Misses       1880     1878       -2     

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

bedrock/settings/base.py Outdated Show resolved Hide resolved
bedrock/settings/base.py Outdated Show resolved Hide resolved
@stevejalim stevejalim added Do Not Merge ⚠️ Backend Server stuff yo Wagtail Development related to our use of Wagtail CMS labels Jul 8, 2024
@stevejalim stevejalim force-pushed the 14774-set-up-wagtail-localize-smartling branch from 887fe4d to 7931a8b Compare July 10, 2024 12:51
@stevejalim stevejalim requested a review from robhudson July 10, 2024 12:52
@stevejalim
Copy link
Collaborator Author

Secrets deployed to dev and will not crash when code reaches Stage or Prod because we have non-exploding (but deliberately invalid) defaults set of setme

bedrock/settings/base.py Outdated Show resolved Hide resolved
Copy link
Member

@pmac pmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+wc

everything other than the question about env vars looks good.


.. note::

It's worth investing 15 mins in watching the `Wagtail Localize original demo`_ to get a good feel of how it can work.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticing the above text looks like it's wrapped whereas this new text isn't.

@stevejalim stevejalim force-pushed the 14774-set-up-wagtail-localize-smartling branch from c0dc2ab to 6962f4d Compare July 15, 2024 16:32
@stevejalim stevejalim force-pushed the 14774-set-up-wagtail-localize-smartling branch from ed6edbb to 51254b6 Compare July 22, 2024 22:02
@stevejalim stevejalim merged commit 1829904 into main Jul 22, 2024
3 checks passed
@stevejalim stevejalim deleted the 14774-set-up-wagtail-localize-smartling branch July 22, 2024 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backend Server stuff yo Wagtail Development related to our use of Wagtail CMS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate and evaluate wagtail-localize-smartling
3 participants