-
Notifications
You must be signed in to change notification settings - Fork 48
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: mark invalid entries as fuzzy | FC-0012 #1944
Conversation
Thanks for the pull request, @OmarIthawi! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
c621790
to
b0fa48c
Compare
@shadinaif @brian-smith-tcril this is ready for initial review. |
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.
@OmarIthawi before I finish reviewing the documentation in this one I'd like to discuss this decision.
The fact that
The workflow lacks a notification mechanism to notify the reviewers
that their translations are invalid.
Is rather concerning to me, as it could lead to a disconnect in understanding of the state of translations. A translator working in Transifex would think everything is fine, and the lack of error on the PRs could make it likely that we wouldn't notice a translated string isn't working until it came time to test the translations manually in-application.
I'm not opposed to utilizing fuzzy
, but I want to make sure we do this in a way that we aren't just "eating errors" so to speak.
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.
Left more comments with more thoughts. I like the fuzzy
plan. Looking forward to seeing what we can do to make it great!
Implement a GitHub Actions workflow that will mark the invalid entries as | ||
``fuzzy``. Then create a commit -- editing the pull request -- to branch | ||
of the bot pull request. |
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.
Implement a GitHub Actions workflow that will mark the invalid entries as | |
``fuzzy``. Then create a commit -- editing the pull request -- to branch | |
of the bot pull request. | |
A GitHub Actions workflow will be implemented to mark invalid entries in synchronized | |
``.po`` files as ``fuzzy``. This will update pull requests created by the Transifex GitHub | |
App. |
* The workflow lacks a notification mechanism to notify the reviewers | ||
that their translations are invalid. |
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.
I discussed this with Sarina and I think we can address this fairly elegantly by allowing this action to create issues on https://github.com/openedx/wg-translations/issues
It might take a bit of work to ensure we don't spam that repo with issues. I'd be happy to discuss how this could be implemented with you!
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.
You're right, we should implement a notification mechanism.
I think the following has the same cost and complexity, therefore we should implement the one with the best Translator Experience.
Option 1: WG Translations project GitHub issues
Create a list of people/teams to notify when things fail. One way is to utilize a CODEOWNER file to know who to nofity for which resource/language pair. Often the file has flexible enough rules to specific files belongs to which contributor.
If the CODEOWNER file format turns to be difficult, we can implement a YAML file to provide an easier to parse file in a similar manner.
Option 2: Slack comments in a new #translations-wg-issues
channel
Post to a message to a new Slack channel e.g. #translations-wg-issues
to announce the issue.
The Slack message may include a handle to the Slack CODEOWNER
with handles to assign Slack users handles and let the bot notify them.
Option 3: Email the contributors with their Transifex emails
Emails are not public, but definitely one way to tackle the issue
Option 4: Open Transifex issues for each entry, and mark them as "unreviewed"
Get the msgid
of the fuzzy
entry then look it up on Transifex and open an issue. This notifies Transifex admins with a direct link to the entry via email. However, those issues are not visible to translators as far as I know.
We can use one or more notification mechanism based on what we see fit.
Please let me know what do you think @brian-smith-tcril.
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.
Since goal is to implement the solution that provides translators with the best experience, I think this is a good question to ask in the #wg-translations slack channel.
7e33cd3
to
c4ed038
Compare
* The workflow lacks a notification mechanism to notify the reviewers | ||
that their translations are invalid. |
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.
Since goal is to implement the solution that provides translators with the best experience, I think this is a good question to ask in the #wg-translations slack channel.
95f7822
to
c5e667b
Compare
if: "${{ github.actor == env.TRANSIFEX_APP_ACTOR_NAME && github.actor_id == env.TRANSIFEX_APP_ACTOR_ID }}" | ||
run: | | ||
# Add the pull request to the merge queue with --rebase commit strategy | ||
gh pr merge ${{ github.head_ref }} --rebase --auto |
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.
Yes, this is sufficient and retry
isn't needed because this step will either succeed or keep failing.
The --rebase --auto
marks the pull request to be auto-merged by GitHub and GitHub will take care of the process.
963e088
to
b17018b
Compare
Status: Currently testing on my fork and there are some complications, but I'm hoping I could address them. Testing PR: |
@brian-smith-tcril this pull request is ready for review and merge. It lacks the notification mechanism, which we can implement in another pull request to make this one smaller. Please check it out and let me know what do you think. Once this is ready for merge, we will need your admin-forced merge. Then update the |
Closed for reasons mentioned in #2128. |
@OmarIthawi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
1 similar comment
@OmarIthawi Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
This is an feature that I'd like to make for Transifex pull requests to avoid having stale PRs at all.
Problem
The Transifex bot creates pull requests with invalid entries in the po-files. Pull requests with invalid translations gets stuck and needs manual intervention in Transifex to fix invalid entries manually.
This often creates a backlog of PRs that needs attention:
Current list of *15* stuck PRs
If Transifex had marked the invalid entries as fuzzy, then the django compilemessages command would have ignored them. However, that's not the case. Therefore, we need to perform this task ourselves.
Solution
Please review the included design document for this PR because it will modify Transifex bot pull requests before merging them.
TODO
Details
validate-po-files
totests
because it's testing Python as wellTest results on my fork