-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: put comments transifex api v3 script #2
Conversation
aeb8347
to
9091736
Compare
in the directory indicated by inputFileDirectory. The file which contains the translation strings instructions, | ||
formatted as <hash>|<instructions>. |
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.
nit: update this line.
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.
LGTM. 👍
How is this being tested? There is risk of deleting all translations if API is called incorrectly, right? |
1 similar comment
How is this being tested? There is risk of deleting all translations if API is called incorrectly, right? |
I used studio-frontend 1-2 uploaded strings to verify the workings. With Transifex api v3(and the design shift to DRF), to delete the translation string, a delete request would be required against the endpoint(https://transifex.github.io/openapi/index.html#tag/Resource-Strings/paths/~1resource_strings~1{resource_string_id}/delete). The api call in this script requires PATCH and only updates instructions, tags, and character limit(https://transifex.github.io/openapi/index.html#tag/Resource-Strings/paths/~1resource_strings~1{resource_string_id}/patch). I can't think of any other way where this api call can result in translation string deletion. If you have any past experience or any thoughts, please do let me know. |
@DawoudSheraz OK, that sounds good. I've seen in the past problems with API calls if not been tested on real situations so glad to see you used studio-frontend to test this. |
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.
Approved - as we talked about, we should carefully test this on a single repo prior to making the new code widely available. Worst case scenario with changes like this, we accidentally delete all our translations, which is difficult to recover from.
PROD-2485
Description
Adds JS script to add translation strings instructions using Transifex API v3. The difference between v2 and v3 api has been documented in this internal document. The old script that utilizes API v2 has been written as the bash script. With API v3, the bash script choice was not taken because:
application/vnd.api+json
This PR is a follow-up to #1, where the script for fetching translation string hashes was introduced.