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

Do not commit changes for a translation that needs approval from a reviewer #3745

Open
goetas opened this issue Apr 17, 2020 · 21 comments
Open
Labels
backlog This is not on the Weblate roadmap for now. Can be prioritized by sponsorship. enhancement Adding or requesting a new feature.

Comments

@goetas
Copy link

goetas commented Apr 17, 2020

Is your feature request related to a problem? Please describe.

We are using a workflow there translations are reviewed by other translators before going online.

Current status:

  1. Translator saves a translation.
  2. Weblate updates the file (XLIFF)
  3. Weblate or an user commits and pushes the changes
  4. App/project gets the new translation (since the changes have been already committed)
  5. Reviewer rejects the translation because quality was low
  6. go to step 1

A this stage the app was already deployed with a translation that was not approved.

Of course it is possible to leverage the approved XLIFF attribute, but no framework does it....

Describe the solution you'd like

Do not commit translations unless they are approved when using a workflow with Translation reviews.

Weblate should keep translations in its own database and write them to FS only when approved.

Describe alternatives you've considered
I've tried to use the "suggest" feature, but is less powerful than doing actual translations (all the translation progress gets lost).

This applies only when using a workflow with Translation reviews.

@goetas goetas changed the title Do not commit changes for a translation that needs review Do not commit changes for a translation that needs approval from a reviewer Apr 17, 2020
@nijel nijel added enhancement Adding or requesting a new feature. undecided These features might not be implemented. Can be prioritized by sponsorship. labels Apr 18, 2020
@github-actions
Copy link

This issue has been put aside. Currently, it is unclear whether it will be ever implemented as it seems to cover too narrow use case or doesn't seem to fit into Weblate. Please try to clarify the use case or consider proposing something more generic to make it useful to more users.

@goetas
Copy link
Author

goetas commented Apr 20, 2020

The issue is that the XLIFF files have a single state, and as soon a file has been written, there is a potential that those changes might land in production.

This makes kinda useless the review process...

I saw the automatic comment added by the bot, is there something more that I can provide?

@nijel
Copy link
Member

nijel commented Apr 20, 2020

The review status is exposed in the XLIFF files exactly for this reason, but I can understand that it might be complicated to change other tooling to understand that.

@goetas
Copy link
Author

goetas commented Apr 20, 2020

Even if changing the other tooling, there will be a moment when a wrong translation is on production.

Case:

  1. dev uploads translation
  2. translator translates
  3. reviewer approves the translation
  4. correct translation goes on production
  5. translator updates the translation (status goes back to approved=no)
  6. system deploys the translations, since approved=no the tooling framework will have to fallback to another language as the translation looks not approved 😟
  7. reviewer approves the translation, approved is set to yes, on the next deploy the text is updated.

With this, at step 6, even if the tolling supports the approved flag (or the other statuses), there will be always a moment when some wrong translation goes online.

Having tens of languages, it is hard to coordinate with translators and reviewers what is ready and what is not.

I'm not sure if XLIFF supports this, what seems needed here is to be able to have two xliff entries with the same key, where one could be approved, and the other no, I this way the frameworks should prefer the xliff translation with approved=yes.

@nijel nijel added backlog This is not on the Weblate roadmap for now. Can be prioritized by sponsorship. and removed undecided These features might not be implemented. Can be prioritized by sponsorship. labels Apr 21, 2020
@nijel
Copy link
Member

nijel commented Apr 21, 2020

So far, we tried to have all translations stored in the VCS. Only the additional content (for example suggestions) is not stored there (mostly because it doesn't make sense). But maybe the time has come to change this paradigm...

What probably would make sense is to introduce something like "write policy" which would define at which level strings would be written to the file. The side effect of this would be that authorship of the string would be hidden from VCS (because it would be written when approved and that would be credited to reviewer).

@github-actions
Copy link

This issue has been added to backlog. It is not scheduled on our road map, but it might be eventually implemented. In case you desperately need this feature, please consider helping or funding the development.

@goetas
Copy link
Author

goetas commented Apr 21, 2020

A write policy sounds a good idea. Regarding the authorship I do not see the issue, git allows to define multiple people as authors for one commit, and if you store in the database the translator that edited the translation and the one that reviewed it, you can use those information in the commit (using the Co-authored-by git header).

I can immagine that such change is not small, but it will allow to weblate to have translation workflows independent from the underlying VCS storage system (as example it will be possible to have a review-based workflow for android translations or any other format). Many of the gotchas caused by translation file formats could be workaround-ed by weblate)

@emmapeel2
Copy link
Contributor

I like this proposal! we are facing the same problem in the Tails translation workflow, and as the OP says the suggestions are not so powerful.

@chrisandrewcl
Copy link

chrisandrewcl commented Oct 28, 2021

When I created #6731, I should have stated that while I think this issue may solve my workflow, it seems like a big undertaking and doesn't seem to be a priority at the moment, that is why I've opened the issue as a question, not a feature request.

It may be just wording, but just to make sure: for my workflow, I think it would be best to keep everything only in the database until they are approved, than saving to file but not committing. This should avoid mishaps with the VCS by just applying changes when they are ready.

I also don't really care about VCS commit ownership from Weblate translations, so I hope this could make things easier.

I am not versed at python, but I was hoping the Weblate addon system could make this possible somehow, so I was more looking for pointers in the right direction (e.g. something like checking state to prevent changing the file in a save hook) than anything else.

@nijel
Copy link
Member

nijel commented Nov 4, 2021

This really has to be implemented in the core. The add-ons do not get to flagging or processing pending entries.

@MAGICCC
Copy link

MAGICCC commented Nov 28, 2021

Would also love to see this! Then it's also possible to recheck translations added by MT sources and approve them when they are ok

@nilmerg
Copy link

nilmerg commented Dec 9, 2021

I'm about to setup Weblate for us but noticed this now. I'm really impressed about all the other stuff 🤩 , but this baffles me to be honest. We rely on gettext and sure have the possibility to not deploy fuzzy strings, but still it would be nice that also these don't get committed by Weblate. When updating the catalogs after a template update, a ton of fuzzy strings may get introduced after all.

But non-approved but otherwise valid strings are not fuzzy! So we have no way in our tooling to exclude them in deployment. 😞

Ideally I want Weblate to be the place where translators do their work without worrying something gets broken if they make mistakes. This is already possible somehow by enforcing some checks, which, if they fail, cause the entry to be fuzzy. (needs editing) The admin then also just commits from time to time without worrying something bad is introduced to the product. A write policy if you will, set to "only approved strings", would make this possible. 👍

The point on authorship is also no issue to me (without knowing the internals of Weblate). Unless the reviewer doesn't change the string, but only changes the state, why should not the translator get the credit? If that's already not the case (didn't test), I'd classify this as bug.

I was thinking of working around this by using the bulk edit addon: Lock the project. Add the flag variant:volatile to all state:translated strings while also setting the mode to "Needs editing". Commit. Remove the flag variant:volatile from all previously changed strings and set their mode to "Waiting for review". Unlock the project. Though this already fails since there seems to be no way of filtering strings with a given flag. 😐

@topialla
Copy link

This would help us a lot as well. Are there any workarounds for continuous translation in interplay with github available until this is possible? We already had some partial translations from the kind weblate community, but had to remove them again. We don't want to do that... ;-)

@nijel
Copy link
Member

nijel commented Feb 21, 2022

I know some projects are cherry-picking translation commits from Weblate on languages they want. This is far from being a nice solution, but should work reasonably well with rebasing.

@rhofer
Copy link
Contributor

rhofer commented Mar 4, 2022

In order that translators can finally decide on whether the translation is really good, they may want to see the result in the final application as a preview. Hence, we also use a preview build of the application which can be visited by translators and check on the real resulting screen on how this looks and feels.

Consequently, this requires state:=translated ("Waiting for review") to be back propagated to the VCS as well. If that would be strictly tight up to state:>translated ("Approved"), the tranlator would claim approval bevore having checked on preview build.

Nevertheless, I also see this requirement to only have approved stings in production. The preview issue has already be mentioned with #2982 and further issues. I could agree to an option to only commit approved strings in order to foster the production scenario, if the preview scenario can be solved.

Sidenote: Another big problem to us is that everything in state:>=translated is automatically added to the translation memory, what meanwhile heavily pollutes our TMs.

@antonkomarev
Copy link

antonkomarev commented May 15, 2022

@nijel If there will be extra attribute in XLIFF files saying real state in Weblate like weblate-state="untranslated|needs_editing|waiting_review|approved", then we will be able to parse file and build our own based on current one. For example we can compile translation without waiting_review and needs_editing. This does not seems to be breaking change.

Or send states in API calls which should be included to exported file:
GET /api/translations/(string: project)/(string: component)/(string: language)/file/?format=xliff&include_states[]=approved&include_states[]=waiting_review
This call will export XLIFF file with translations for staging in states: Waiting for review and Approved.
For production we can export only Approved translations:
GET /api/translations/(string: project)/(string: component)/(string: language)/file/?format=xliff&include_states[]=approved

2nd variant may be represented in Download translation UI page too... as checkboxes with states and you may check what states should be included in output file. But only adding API params will be completely enough for the automated tasks, UI is optionated feature.

Otherwise we cannot determine states now:

  • Needs editing (in PO files they have fuzzy flag, in XLIFF approved=no)
  • Waiting for review (in PO files they dont have fuzzy flag, in XLIFF approved=yes)
  • Approved (in PO files they dont have fuzzy flag, in XLIFF approved=yes)

Waiting for review looks completely the same as Approved from file perspective.

@nijel
Copy link
Member

nijel commented May 16, 2022

XLIFF already stores the real state, see https://docs.weblate.org/en/latest/formats.html#xliff. It does not seem to work well for the exporter though, I'll look into that.

@comradekingu
Copy link
Contributor

@goetas
Working purely off external tooling, you could go back to an earlier commit to find an approved version to use in case a later unreviewed translation has landed thereafter, following 6. here.
(This prevents the malice of fuzzifying all strings, leading to their non-use.)

On the assumption that strings are committed immediately,
I think a review freeze period (by un-checking regular translations) rather than final stringbase in VCS different from what the latest translation is.
This is based on my view that any single translator is the ultimate authority on quality, not upstream.

then it is still easy to put out beta versions with un-reviewed strings.
(This helps ultimate quality by way of eyeballs and translations looking as wrong as possible.
Hopefully this leads to removing translators entirely and opens the door to flagging all translations by a translator as needing review.)

If strings are committed once before a new version is put out, effectively this grants the same type of review window by looking at the PR/commit, but there is no easy way to tell what has gotten looked at by whom.

Dialling it back to having reviewers all hinges on reviewers being hand-picked,
(as opposed to just manually approved) as otherwise the "review" status is functionally meaningless,
allowing for anyone to make an extra account to be both editor and reviewer.

The "needs editing" status is too limited, as it doesn't allow for a bimodal user specified determination of whether it at the very least should go to upstream in any capacity or not.
(This is why I use it in nb_NO for just anything from my own problems to source strings needing work or ones I intend to fix,
as marking source strings can prevent anyone in any language from translating it. Luckily there aren't many nb_NO translators, so quality seldomly suffers from others having different ideas about it.)

What I want is a per-translator review status, since then nobody can effectively undo the binary review status of any given reviewer (me, and why I don't use it) by toggling the status.
(This allows people to issue reviews right away, where they aren't either meaningless as a commodity available to all, or subject to a mostly prior authority-by-position promotion to "overcome" maintainers
not being able to check actual strings.)

Even though multiple translations of the same string is possible, I don't think more than one reviewed string needs to be considered if there is rate-limiting on marking as needing edit (not sure if that undoes review, and how else can one be challenged without adding a translation or
resorting to the comment field), like there is with issuing translations.
Luckily there is very little malice, but automated attacks have a great amount of surface.

Other musings:
The regular (unreviewed) translations should have an option to add the the first 5 characters of the Weblate checksum to the actual string, possibly making it a link, or mouseover,
so it is easy to go an change the actual string that needs changing.
(As opposed to something with the same name, going into production merrily.)

TL;DR
I think it would be best to point regular translations to one set of files betas can be based off of, and one set of files to use in production. (Similar to the staging of production strings coming from devs.) Does anyone need anything else?

For consistency
Picking reviewers isn't any easier than gauging the quality of translations in languages one is unfamiliar with.
The number of reviews is largely meaningless, and can hold up translations.
The number of reviews by any one translator with full coverage of the stringbase isn't meaningless.
The selection process of which reviewers to trust should be done independent of their ability to give reviews (afterwards).

@zen-fu
Copy link

zen-fu commented Jul 14, 2022

@emmapeel2:

we are facing the same problem in the Tails translation workflow

To contribute with some context:

  • Tails currently has external tooling to automatically integrate:
    • changes from the Weblate Git repo into Tails' main Git repo. This is needed to avoid merge conflicts between updates made to each repo.
    • changes from the Tails Git repo into Weblate. This is needed because there are more languages in Weblate than in the production website and they need to have their source strings updated accordingly.
  • All the translations and tooling are based on .po files (there's no XLIFF in place in our case).
  • Translations need to be reviewed by reviewers before they go live. This is currently achieved by leveraging the "Suggestions" feature, but that is far from ideal as UX for translators and reviewers becomes weird. "Needs review" is not useful in this context.
  • Changing such tooling might be expensive as there are many corner cases that need to be handled.

This is the workflow we thought about and think would be better:

  1. A Translator (self-registered user) finds a translation that needs work ("Needs edit" or "Needs review").
  2. The Translator then translates and saves the translation as "Needs review". (Nothing is committed to Git at this point.)
  3. A Reviewer finds translations that need review, reviews, and saves the translation as "Approved". (The translation is now committed to Weblate's Git repo.)
  4. Tails integration tooling propagates that change automatically to Tails main Git repository, and the translation goes live.

Note: if a "write-policy" implementation will cause the state in Weblate backend to be different from the state stored in .po files, then when reloading changes from Tails main Git into Weblate we'd need some way to avoid losing translations that haven't been reviewed yet and only live in Weblate backend.

Note 2: we'd still need a way to extract "Needs review" strings to build our staging website, but we can adapt the code we currently have for suggestions for that.

@comradekingu

I think it would be best to point regular translations to one set of files betas can be based off of, and one set of files to use in production. (Similar to the staging of production strings coming from devs.) Does anyone need anything else?

I think this approach could address Tails' needs, including "solving" Note 2 above, but we'd still need to avoid losing translations as described in Note 1 above.

@henry-torproject
Copy link
Contributor

henry-torproject commented Oct 18, 2023

This would we really great improvement for formats that do not have metadata for the "fuzzy" or "translated" states. Such as Android String Resources, and Fluent (probably a lot more quickly looking through the translate toolkit storage files).

Right now, for these formats, a "Needs editing" (fuzzy) or "Waiting for review" string will be saved and will replace the old string in the file itself, with no indication in the file that the string should be ignored. So at the application level, there are no means to filter out these strings.

In principle, these formats without support for these states should never save incomplete strings. But, as mentioned in #9775 (comment), if we were to change translation toolkit to simply not serialize them, then they would be lost from weblate at the next sync. Moreover, we still have the problem as mentioned above: that a bad edit would replace any previous ok string.

Also, having a "database first" approach could help address the concern in #10106:

This makes it possible to recover from the situation, but the string can be lost as it is not serialized, the change will disappear on next upstream update.

@comradekingu
Copy link
Contributor

comradekingu commented Oct 18, 2023

@goetas

There is something I don't understand at all here, and it starts at the premise:

This isn't a special new status, but rather tries to make calls based on existing functionality (doesn't work),
and changes how things works for reviewed projects (messes with learnt behavior, and has unforeseen effects).

Do not commit translations unless they are approved when using a workflow with Translation reviews.

Could this be explained? This is exactly how Crowdin fails by default by requiring two people to agree.
Reviews in Weblate are useful how?

Weblate should keep translations in its own database and write them to FS only when approved.

For a given confidence level, that is the decision of the translator..
The translator can approve, or make another account to do it, see point 1.


"Needs editing" is used for anything that isn't 100%. I even use it in local languages to keep track of upstream strings that need work. Last I checked it locks all translations if done for the source string.

There is no way to assert confidence level when clicking "needs editing" on part of the translator,
and it isn't protected per translator.
Yes, it doesn't do damage by locking things down, but it is impossible to tell anything worthwhile about its existence just like with "reviewed" for the same reasons.

There are many reasons to run a database locally, like for example saving translations made in locked components.

When stuff isn't meant to go in, and some level of work or fleshing out of a problem needs to happen in a different manner, it is supposed to be a suggestion.

Quality is better maintained with more eyeballs, statuses that actually can be pinned to anything other than "true positive" (no flags), for any given translator, and automated suggested workflows AFAICT.

In my view, the thing that will be gained by changing the default idea of what goes upstream for every project in "reviewed" ones is that knowledge is lost, and that more importantly people will purposefully review stuff to get it in.
Thus making "reviewed" even more of a crux.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog This is not on the Weblate roadmap for now. Can be prioritized by sponsorship. enhancement Adding or requesting a new feature.
Projects
None yet
Development

No branches or pull requests