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

Apply locale ID mappings consistently #16

Merged
merged 7 commits into from
Aug 1, 2024

Conversation

bcdickinson
Copy link

Fixes #15

@stevejalim take this with a pinch of salt, I have given it as thorough a shakedown as it needs, but I think this should fix the issue you showed me earlier. If you're able to give it a spin and let me know if it fixes your problems, that'd be great.

Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

Needs a small tidy up.

LGTM otherwise

src/wagtail_localize_smartling/settings.py Outdated Show resolved Hide resolved
src/wagtail_localize_smartling/utils.py Show resolved Hide resolved
@stevejalim
Copy link
Collaborator

stevejalim commented Jul 30, 2024

Checked out the branch locally and ran it. First time it clashed with existing matching job names (not sure if that's something being tweaked on a separate branch?)

wagtail_localize_smartling.api.client.FailedResponse: VALIDATION_ERROR
  [ { 'details': {'field': 'jobName'},
      'key': 'error.validation.job.alreadyExists',
      'message': 'There is already a job with that name, please provide a unique '
                 'name'}]

But after clearing down those jobs on the Smartling side, sync up to Smartling seemed fine:

INFO 2024-07-30 03:25:10,112 client Smartling API request: POST https://api.smartling.com/auth-api/v2/authenticate
INFO 2024-07-30 03:25:10,555 client Smartling API response: 200 0.366277s
INFO 2024-07-30 03:25:10,556 client Smartling API request: GET https://api.smartling.com/projects-api/v2/projects/REDACTED
INFO 2024-07-30 03:25:11,032 client Smartling API response: 200 0.473288s
INFO 2024-07-30 03:25:11,174 models Synced project Mozilla.org CMS (Dev and Staging) (ID REDACTED)
INFO 2024-07-30 03:25:11,186 sync Performing initial sync for job 975b993f-152c-4f4c-8ecb-49e9e9de13dc:5:de:es-ES:fr:it:ja:nl:pl:pt-BR:ru:zh-CN:2024-07-29T15:44:19+00:00
INFO 2024-07-30 03:25:11,197 client Smartling API request: POST https://api.smartling.com/jobs-api/v3/projects/REDACTED/jobs
INFO 2024-07-30 03:25:11,633 client Smartling API response: 200 0.43198s
INFO 2024-07-30 03:25:11,683 client Smartling API request: POST https://api.smartling.com/files-api/v2/projects/REDACTED/file
INFO 2024-07-30 03:25:12,530 client Smartling API response: 200 0.844413s
INFO 2024-07-30 03:25:12,554 client Smartling API request: POST https://api.smartling.com/jobs-api/v3/projects/REDACTED/jobs/yqc1mctgoviq/file/add
INFO 2024-07-30 03:25:13,563 client Smartling API response: 200 1.005362s

Copy link
Collaborator

@stevejalim stevejalim left a comment

Choose a reason for hiding this comment

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

Worksforme!

@bcdickinson
Copy link
Author

Thanks both. I'll tidy up those unused imports and add a sprinkling of unit tests for the util functions and then get this merged.

@stevejalim
Copy link
Collaborator

@bcdickinson Apols, I forgot I had targetted my add-option-to-disable-downcasing-of-locales branch to merge into this one. Hopefully not a pain! I can back it out if need be

@bcdickinson
Copy link
Author

@bcdickinson Apols, I forgot I had targetted my add-option-to-disable-downcasing-of-locales branch to merge into this one. Hopefully not a pain! I can back it out if need be

Not to worry, I'll review those changes here, there were a couple of bits I wanted to mention.

src/wagtail_localize_smartling/utils.py Outdated Show resolved Hide resolved
src/wagtail_localize_smartling/utils.py Outdated Show resolved Hide resolved
@bcdickinson bcdickinson marked this pull request as ready for review August 1, 2024 09:23
Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

A few cosmetic tweaks needed.
Kudos for the smartling settings fixture!

README.md Outdated Show resolved Hide resolved
src/wagtail_localize_smartling/utils.py Outdated Show resolved Hide resolved
tests/conftest.py Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@stevejalim stevejalim left a comment

Choose a reason for hiding this comment

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

Lovely lovely lovely. Thanks for this, Ben!

PS: I agree with all of Dan's tweaks

README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@zerolab zerolab left a comment

Choose a reason for hiding this comment

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

:shipit: with that small typo fix.
Great work, @bcdickinson

@zerolab zerolab merged commit 7b12cb4 into main Aug 1, 2024
15 checks passed
@zerolab zerolab deleted the bugfix/consistent-locale-id-mapping branch August 1, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API calls to add files to translation jobs don't apply language code mapping
3 participants