Support pending revisions and accepting translation as a specific moderation state

+++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -205,6 +208,58 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
+
+    // If the storage is revisionable load the latest revision.
+    if ($storage instanceof TranslatableRevisionableStorageInterface) {
+      $langcode = \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId();
+      foreach ($entities as $key => $entity) {
+        $revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $langcode);

What you are missing is a check for $entity_type->isRevisioanble.

We also need to respect the language code from the job, not the current language, so the job/langcode is a requirement argument here.

1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -264,6 +308,18 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
   +          $entity->setRevisionLogMessage(sprintf('Created by TMGMT. See %s for more details.', (string) $job_item->getJob()->toLink()->toString()));
   

   this should use t(), not sprintf().

2. +++ b/sources/content/tests/src/Kernel/ContentEntitySourceUnitTest.php
   @@ -248,7 +248,7 @@ class ContentEntitySourceUnitTest extends ContentEntityTestBase {
    
        // Check that the translations were saved correctly.
   -    $entity_test = entity_load($this->entityTypeId, $entity_test->id());
   +    $entity_test = \Drupal::service('tmgmt.entity_loader')->load($this->entityTypeId, $entity_test->id(), $job->getSourceLangcode());
        $translation = $entity_test->getTranslation('de');
    
        $this->assertEqual($translation->name->value, $data['name'][0]['value']['#translation']['#text']);
   

   not sure if we want to use this service in kernel tests. We also have lots and lots of load calls, those just use $class::load().

   Also with the content moderation specific tests, I'd say we want to explicitly load by revision ID, to make sure our service does what it should.

3. +++ b/src/Entity/JobItem.php
   @@ -489,7 +488,7 @@ class JobItem extends ContentEntityBase implements JobItemInterface {
          try {
   -        $translation = entity_load($this->getItemType(), $this->getItemId());
   +        $translation = \Drupal::service('tmgmt.entity_loader')->load($this->getItemType(), $this->getItemId(), $this->getJob()->getSourceLangcode());
          }
          catch (PluginNotFoundException $e) {
   

   woah, this is weird, looks like we ported that along for some time.

   We only care about the label and url, we have generic methods to get that from the source. Lets open a separate issue to clean that up and don't touch it here.

4. +++ b/src/EntityLoader.php
   @@ -0,0 +1,68 @@
   +class EntityLoader implements EntityLoaderInterface  {
   

   This is content entity specific, so it should be in tmgmt_content, as mentioned above, the only case that is not above is a bug and code that shouldn't exist (and seems mostly unused outside of tests)

5. +++ b/src/EntityLoader.php
   @@ -0,0 +1,68 @@
   +    try {
   +      /** @var \Drupal\Core\Entity\EntityStorageInterface $storage */
   +      $storage = $this->entityTypeManager->getStorage($entity_type_id);
   +    }
   +    catch (PluginException $e) {
   +      return [];
   +    }
   

   I don't think we should catch/support that, with the case above removed, this should never happen.

6. +++ b/src/EntityLoader.php
   @@ -0,0 +1,68 @@
   +        $entity_langcode = $langcode === LanguageInterface::LANGCODE_DEFAULT ? $entity->language()->getId() : $langcode;
   

   I'd prefer using NULL for this instead of DEFAULT. Most cases that use NULL then fall back to the current language and while I agree that the original language of the entity does make sense here, I think I prefer NULL :)

In general, this looks good so far, but we're going to need a lot of test coverage, basically for a lot of features we have like basic jobs, continuous, preview and so on.

Started looking into that test fail/workaround. The change fixes this test fail, but it won't be enough if we start to have more than one language.

If you look at \Drupal\content_translation\Controller\ContentTranslationController::overview(), then you can see that it loads the latest affected revision id for each translation and I think that's actually the right thing to do. If we start mixing it with drafts, then we will have a situations where there is a translation that doesn't exist in either the default revision or the source revision, so we can't rely on $entity->getTranslationLanguages() anymore.

I think we should actually look into displaying an icon in case we detect a pending revision (as well as link to it), but that's definitely a follow-up. We should have test coverage for the situation with saving two translations as a draft and making sure both show up, though.

Will continue to review later.

1. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -341,11 +435,32 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
   +      if ($storage instanceof TranslatableRevisionableStorageInterface) {
   +        // Always create a new revision of the translation.
   +        $entity = $storage->createRevision($entity, $entity->isDefaultRevision());
   +        // @todo: Investigate why is this needed?
   +        $entity->setRevisionTranslationAffected(NULL);
   +
   

   The order here is wrong, createTranslation() should be called with $translation, as this is very much about a specific translation.

   With that change, I'm down to 3 different fails about the revision log message, and when I move that above the createRevision() call, it passes without the translation affected hack. Maybe this should even be at the end, just before the save in fact.

   I think we also need better test coverage for that, specifically, we want to test the following situation:

   create a source entity translation, then create two different translation jobs, one for each language, then accept both, then make sure that the final revision contains all translations.

2. +++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
   @@ -381,12 +496,24 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
    
   +    if (isset($data['#moderation_state'])) {
   +      $moderation_state = reset($data['#moderation_state']);
   +      // If the entity is moderated, set the moderation state for translation.
   +      if (static::isModeratedEntity($translation)) {
   +        $translation->set('moderation_state', $moderation_state);
   +      }
   +      // Otherwise, try to set a published status.
   +      elseif ($translation instanceof EntityPublishedInterface) {
   +        $translation->setPublished((bool) $moderation_state);
   +      }
   

   I'd prefer to use two different keys for this, published aka true/false is *not* a moderation state and reading it like this is confusing.

3. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -0,0 +1,323 @@
   +    // The spanish translation is not published. English content is displayed.
   +    $this->assertText($title);
   

   The thing is that this would also be true if we would see the spanish site as $title is contained within $draft_title.

   Not easy with the default prefixes that we ave with our translations, but maybe we you could at least add an explicit assertNotext() for the translation as well?

4. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -0,0 +1,323 @@
   +    $this->clickLink('Revisions');
   +    $this->assertText('Created by TMGMT. See [Draft] Moderated node (2) for more details.');
   +    $this->assertText('Created by TMGMT. See [Draft] Moderated node for more details.');
   +  }
   

   don't quite understand the see thing, is that a link? the job title? not sure if that is very useful, at least the wording is strange.. maybe something like "Created by translation job @label"?

   Also, looks like at this point, we're already pretty well prepared to improve the test coverage of the source overview page, to make sure that both the german as well as the spanish translation are shown as translated which I'm pretty sure isn't working yet, as you didn't publish the spanish translation yet.

5. +++ b/sources/content/src/Tests/ContentEntitySourceUiTest.php
   @@ -529,6 +529,35 @@ class ContentEntitySourceUiTest extends EntityTestBase {
   +    $edit = [
   +      'moderation_state[new_state]' => FALSE,
   +    ];
   +    $this->drupalPostForm(NULL, $edit, 'Save as completed');
   +    $this->drupalGet('it/comment/' . $comment->id());
   

   does this happen because the content is unpublished and this user isn't allowed to view that?

   I'd prefer giving him bypass node access or so so we can check it's there and then 403 as anonymous user.

6. +++ b/sources/content/tmgmt_content.info.yml
   @@ -12,4 +12,4 @@ dependencies:
    
    test_dependencies:
     - pathauto
   -
   + - drupal:content_moderation
   

   this doesn't do anything, test_dependencies is just to test the testbot to fetch additional modules and require-dev in a composer.json is actually the better approach for that because it works immediately.

   Since this is in core, we don't need either.

1. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -511,4 +547,151 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
   +      'current' => [
   +        '#type' => 'item',
   +        '#title' => $this->t('Current state'),
   +        '#markup' => $default->label(),
   +        '#wrapper_attributes' => [
   +          'class' => ['container-inline'],
   +        ],
   +      ],
   +      'new_state' => [
   +        '#type' => 'select',
   +        '#title' => $this->t('Change to'),
   +        '#options' => $transition_labels,
   

   The labels are a bit confusing.

   This is taken from the widget, but the widget is when you edit an existing thing and just resave it.

   We're often creating a new translation, or updating it, from another language, so we have more than one "thing" and need to be more specific.

   Maybe "Current source state" and "Translation state"?

   Also the @see above is technically wrong, @see is only allowed in docblocks, in inline comments, just use "See .. " without @

2. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -161,6 +159,29 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
   +          // Make sure we do not list removed translations, i.e. translations
   +          // that have been part of a default revision but no longer are.
   +          if (!$latest_revision->wasDefaultRevision() || $default_revision->hasTranslation($langcode)) {
   +            $entity = $latest_revision;
   +            // Update the label if we are dealing with the source language.
   +            if ($langcode === $source_lang) {
   +              $entity_label = $entity->label();
   +            }
   +          }
   +        }
   

   Do we have test coverage for this stuff?

   I think that would for example mean having an entity created in a different language and having a draft for that in that language.

3. +++ b/sources/content/src/ContentEntitySourcePluginUi.php
   @@ -401,7 +425,8 @@ class ContentEntitySourcePluginUi extends SourcePluginUiBase {
        $langcode_table_alias = 'e';
   -    if ($data_table = $entity_type->getDataTable()) {
   +    $data_table = $entity_type->isRevisionable() ? $entity_type->getRevisionDataTable() : $entity_type->getDataTable();
   +    if ($data_table) {
          $langcode_table_alias = $query->innerJoin($data_table, 'data_table', '%alias.' . $id_key . ' = e.' . $id_key . ' AND %alias.default_langcode = 1');
        }
   

   Still not quite sure about this.

   Example scenario:

   I have a node with title "First node", then I create a draft of that with title "FOO node".

   That gives me this:

   I can't quite decide if that's a bug or a feature :)

   But lets keep it for now and open a follow-up to discuss if the search should work on the latest, default or all revisions. And add a @todo here for that.

4. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -185,15 +175,62 @@ class ContentEntitySourceContentModerationTest extends EntityTestBase {
   +    // Create a draft revision in italian using core translation.
   +    $edit = [
   +      'title[0][value]' => "it: $second_draft_title",
   +      'moderation_state[0][state]' => 'draft',
   +    ];
   +    $this->drupalPostForm('it/node/' . $node->id() . '/translations/add/en/it', $edit, 'Save (this translation)');
   +    // New German revision has been created when Spanish translation was added.
   +    $this->assertNodeTranslationsRevisionsCount($node->id(), 'de', 4);
   

   what is it now, spanish or italian? ;)

5. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -185,15 +175,62 @@ class ContentEntitySourceContentModerationTest extends EntityTestBase {
   +    $this->assertTextByXpath('//tbody/tr[1]/td[6]/a/img/@title', 'Translation up to date');
   +    // There is a spanish translation (saved as a draft).
   +    $this->assertTextByXpath('//tbody/tr[1]/td[7]/@class', 'langstatus-it');
   +    $this->assertTextByXpath('//tbody/tr[1]/td[7]/a/img/@title', 'Translation up to date');
   +    // There is a spanish translation (saved as a draft).
   +    $this->assertTextByXpath('//tbody/tr[1]/td[8]/@class', 'langstatus-es');
   

   again.

6. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -323,4 +429,84 @@ class ContentEntitySourceContentModerationTest extends EntityTestBase {
   +   */
   +  protected function createEditorialWorkflow() {
   +    if ($workflow = Workflow::load('editorial')) {
   +      return $workflow;
   +    }
   +
   +    $workflow = Workflow::create([
   +      'type' => 'content_moderation',
   +      'id' => 'editorial',
   +      'label' => 'Editorial',
   

   What about just always creating a workflow with a different ID? Then we're not tied to whether it exists by default or not and we also ensure that it doesn't just work with the default.

Forgot to add the screenshot above for the search thing:

1. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -33,9 +40,9 @@ class ContentEntitySourceContentModerationTest extends EntityTestBase {
   -    $workflow->save();
   +    $this->workflow = $this->createEditorialWorkflow();
   +    $this->workflow->getTypePlugin()->addEntityTypeAndBundle('node', 'article');
   +    $this->workflow->save();
   

   maybe just make that part of the create method now? you could pass in the bundle to the method or so.

2. +++ b/sources/content/src/Tests/ContentEntitySourceContentModerationTest.php
   @@ -430,19 +456,16 @@ class ContentEntitySourceContentModerationTest extends EntityTestBase {
       * @return \Drupal\workflows\Entity\Workflow
   -   *   The editorial workflow entity.
   +   *   The workflow entity.
   +   * @throws
       */
   

   weird @throws

That's afaik a new PhpStorm feature that I usually disable :)

Rebasing patch. :)

This should be good for 8.x-1.6 :)

Rerolled.

Updated the patch.

Reroll.

Converted the test to phpunit.

@Berdir

Is there a timeline on this item? We could really use this in our 8.7.1 instance and it would be nice to set expectations.

Thanks for the hard work!
Mike

Hi everyone.

We have some similar requirements as the ones in this patch but we need more things. My intent is to extend the patch and build what we miss upon it. I was wondering however if it was a good idea to do it in this issue or to create a separate issue for it. What I am planning to add is the following:

• Being able to select for translation an entity in a specific version state, and not just the latest draft like it is doing currently.
• Being able to match the translation version state to the entity version state, so if a translation is approved with the draft state, it will translate the draft version of the original entity.

I believe that's not something that you can achieve in the UI, so I'm not sure about supporting that out of the box. Also, content_moderation enforces new revisions every time you save, so I'm not sure what the use cases are for translating specific old revisions?

@Berdir - Use Case: You have draft content that is not ready to publish. An example might be a new product announcement. You need to translate that draft. When the translations come back, you don't want to auto publish them because the product has not yet launched. You want the translated content to exist in the same state as the source. When the time is right, you would publish all.

In my brief testing, the way this patch works is that it allows draft (moderation state) translation, but the returned translations are immediately published.

See comment #12, there a UI to select the target status/moderation state and it should default to what the source does.

That should work, but there might be bugs.

What doesn't work is translating a published version while there is a draft/forward revision.

That makes sense. I only ran a quick test and must have missed that setting.

Will do some more testing and report back if we run into any issues. Thanks!

What is the expected behavior when the provider is set to "auto accept" translations? Should we update the provider settings form to add an option that auto accepts based on moderation state?

Scenario:
1) Create a basic page (published)
2) Create a French translation (published)
3) Update the source page and save as a draft
4) Initiate translation for the draft state using a provider set to auto-accept translations
5) Return the translation and download the file.

In the above scenario the imported translation was auto published for the French node even though the source was in a draft state.

Ok, looks like it was mentioned in #12 "The auto-accept use-case is not yet supported. As per #14 we will do this in a follow-up."

Is anyone working on this item?

I tried to apply the patch against 1.10 and failed. Here. Not hiding the previous one -- it might be good against dev, not sure.

Patch at #53 is correct over 1.x-dev#c90a895a20fc2e52bf74fab60ec1b7af8e9aac3
I made my own from #44 and I have the same result.
Now I will proceed to some test about translation with content moderation

I have just applied the patch from #53 on 1.10, it is working well :)

Also, I have exactly the same scenario as @keats76 on #51. Is anyone working on this or is it maybe planned?
This feature is really needed by our contributors, I have some time to work on it. Maybe @mbovan or @berdir can help defining a to-do list/checklist in order to provide this feature the right way?

Thanks

Since this is quite a big patch already I think automated workflows should be a follow-up.

The reason this got stuck is that has been implemented in sync with content moderation support in paragraphs, but it is assuming an earlier implementation. Basically, it is trying to do #3004099: Allow to translate paragraphs from pending revisions but that doesn't work and I don't know how to solve that without changing core.

Specifically, if you add a new paragraph in a new draft, then translating that in the UI will still be based on the default revision, but the implementation in TMGMT will not, and as a result, it will conflict when trying to accept the translation. I'm not sure what will happen exactly, but it won't be pretty.

So to make it possible to commit this, we probably need to do something similar as the paragraph UI, only support paragraphs that exist in the default revision. And we need tests for that, probably best starting with a failing test for such a scenario,

Thx @berdir for your answer.

In my case, I am not using the Paragraphs module. If I understand well, this removes few difficulties.
Can you help me defining the different steps I should follow please? I will create a new issue as you have suggested.

This patch doesn't work if use Providers (ex: memsource).

We don't use review interface

Step to reproduce

1) Create Node with draft state in EN
2) Add translation FR (draft status) and push translation to memsource service
3) Pull translation in job page
4) this node in FR have a published status.

It's not about which provider but having auto-accept enabled or not, as written above, that is not supported yet and would be better done in a separate issue.

Thank you for the great work on this issue!
I did some manual testing and both cases are working well (with and without content moderation enabled).

Probably not the scope of this issue but adding some thoughts about the translation state to be selected during the Job review:
The next translation state is fetched from the source and not the translation.
Would it make sense to use the next translation state instead of the source there and also display the current one in this case?
E.g. like this, or perhaps remove the current source state then?

This applies only for existing translations though, newly created translations might inherit the state from the source.

Thinking about several translations (e.g. with Trados)
- The source is in the 'published' state
- The translations are in 'draft' state that can then e.g. transition to, say, 'draft' or 'for approval' state
So it might allow to keep restrictions to the transitions then (e.g. not being able to transition from 'published' to 'for approval').

Then it still requires to check how it behaves with auto-accept expectations (#51).

Also, I left some notes here related to the integration with the Xliff File provider if we want to include the latest revision translation
https://www.drupal.org/project/tmgmt/issues/3130344#comment-13571671

After some more testing with various state transitions it looks good so RTBC+1

Not sure that we want to tackle this here, but spotted an issue related to asymmetric translations (while using this setup https://www.drupal.org/project/paragraphs_asymmetric_translation_widgets).

#3134922: Integration with paragraphs asymmetric translation

About the moderation state fallback to the published status. On ContentEntitySourcePluginUi::buildPublishStateElement() we fetch the source published status to set it by default to the translation. Then, we allow to change it, which is fine if the status is translatable.
In some cases, it could be synced (e.g. MenuLinkContent "enabled", set as synced by the site builder for other non moderated entity types, ...).

We should probably add the "all languages" hint there if necessary by checking the field definition, to prevent unexpected changes to the published status.

+++ b/sources/content/src/Plugin/tmgmt/Source/ContentEntitySource.php
@@ -687,12 +687,20 @@ class ContentEntitySource extends SourcePluginBase implements SourcePreviewInter
       foreach ($entity->getFieldDefinitions() as $definition) {
-        if ($definition->getType() === 'entity_reference_revisions' && !$definition->isTranslatable()) {
-          return $entity;
+        if (in_array($definition->getType(), ['entity_reference', 'entity_reference_revisions']) && !$definition->isTranslatable()) {
+          $target_type_id = $definition->getSetting('target_type');

not sure if we need to check ER here as well, it wouldn't actually support the composite stuff, but if we do it like that already elsewhere then fine.

Awesome, very happy to finally being able to commit this as an acceptable solution. For more advanced use cases that were discussed, please create separate issues, there's also the parent issue still open.