Check for source change on review and show diff

Could this (and 1686578) even be avoided earlier in the process? For example, if a user changes source content which is source for translation job items in progress/review, they should have the option to cancel the translation job item. (Is it even possible to cancel a job item without cancelling its parent job?)

Definitely related to #1686578: Merge with existing translation on review / accept, and #1623402: Option to cancel pending translation jobs when its source content is edited.

Here is the first approach:

Also added tests.

In a followup we can improve the UI and in another integrate it with the diff module.

1. +++ b/src/Controller/DataItemDiffController.php
   @@ -0,0 +1,84 @@
   +    $diff_rows = array();
   +    $build = array(
   +      '#title' => $this->t('Revisions for %title', array('%title' => $item->label())),
   +    );
   

   diff left-over?

2. +++ b/src/Controller/DataItemDiffController.php
   @@ -0,0 +1,84 @@
   +    $diff_header = ['Current text', 'New text'];
   

   missing t()

3. +++ b/src/Controller/DataItemDiffController.php
   @@ -0,0 +1,84 @@
   +      '#url' => Url::fromRoute('entity.tmgmt_job_item.canonical', ['tmgmt_job_item' => $item->id()]),
   

   why not $item->toUrl()?

4. +++ b/src/Form/JobItemForm.php
   @@ -176,6 +178,13 @@ class JobItemForm extends TmgmtFormBase {
   +    $actions['has_changed_source'] = array(
   +      '#type' => 'submit',
   +      '#value' => t('Has changed'),
   +      '#access' => !$item->isAccepted(),
   +      '#validate' => ['::hasChangedSource'],
   +      '#submit' => ['::submitForm'],
   +    );
   

   i don't think we want this as a a button.

   We should just always check it when looking a review-form of a not yet accepted job item.

5. +++ b/src/Form/JobItemForm.php
   @@ -670,4 +687,36 @@ class JobItemForm extends TmgmtFormBase {
   +    foreach ($form_state->getValues() as $field => $value) {
   +      if (is_array($value) && isset($value['translation'])) {
   +        $keys = \Drupal::service('tmgmt.data')->ensureArrayKey($field);
   +        $new_data = \Drupal::service('tmgmt.data')->flatten($item->getSourceData());
   +        $current_data = $item->getData($keys);
   +        $target_key = str_replace('|', '][', $field);
   +        if ($current_data['#text'] != $new_data[$target_key]['#text']) {
   +          $source_changed[$field] = 'The source has changed. ' . Link::fromTextAndUrl('View diff', Url::fromRoute('tmgmt.data_item_diff', [
   +            'tmgmt_job_item' => $item->id(),
   

   I don't think we need to involve the translation or values in the form structure here. Just compare the data of the job item with what the source returns.

Done.

Here the new screenshots:

On click "Show diff":

Fixed tests.

Now should be fixed, added the case when the field is removed.

I'll open a followup for when a field is added, to add also a message, in this case the user probably should abort the job if it's possible and create a new translation.

Found a new case, when you directly remove all the source. I just add the message for each element as was removed.
Not sure if is the best solution, but we already have a message that the source was removed, but I think just appears when you try to save as completed.

Referring to #2690037: Check if source url of a job item exists on JobItemForm #10 : We should cover the case when source doesn't exist at all. (e.g. it was deleted or lost)

Done. Here some screenshots:

Source modified.

Field deleted.

Resolved the conflict.

Removed the the whole source.

Done the following changes:

I'll create a followup for the case where we add a new field.
And open an issue in core to fix that CSS.

Fixed the first thing, now I'm keeping the structure, and as I'm just comparing the translatable texts I thing is not possible that in case of a change in the structure we can not resolve a change, so should work fine.

I could not reproduce the second thing.

I did this and also created this followups:
#2693671: Add missing tags and text format in tmgmt_demo
#2693673: Check for added fields in the source

Is still missing the core issue for the CSS as I don't know how to see the diff.

1. +++ b/src/Form/JobItemForm.php
   @@ -725,4 +811,71 @@ class JobItemForm extends TmgmtFormBase {
   +          $source_changed[$field] = 'This data item has been removed from the source.';
   +          $form_state->setValue('removed' . $field, TRUE);
   +        }
   

   using setValue() seems wrong for this, that's for form values. Should just use set/get? also don't really like the combined key, maybe use an underscore? or an array of removed keys, but that would be more complicated to work with.

2. +++ b/src/Form/JobItemForm.php
   @@ -725,4 +811,71 @@ class JobItemForm extends TmgmtFormBase {
   +    $form_state->setValue('show_diff' . $target_key, TRUE);
   

   same here.

3. +++ b/src/Form/JobItemForm.php
   @@ -725,4 +811,71 @@ class JobItemForm extends TmgmtFormBase {
   +    $tmgmt_data_item = str_replace('][', '|', $target_key);
   +    $key = \Drupal::service('tmgmt.data')->ensureArrayKey($tmgmt_data_item);
   +    $first_key = reset($key);
   +    $source_data = $item->getSourceData();
   +    $new_data = \Drupal::service('tmgmt.data')->flatten($source_data)[$target_key];
   

   Quite a bit of repeated code here, but we can try to improve that later, I've been thinking before about a shared trait between the two translate/review forms.

4. +++ b/src/Form/JobItemForm.php
   @@ -725,4 +811,71 @@ class JobItemForm extends TmgmtFormBase {
   +    $form_state->setValue('show_diff' . $target_key, FALSE);
   

   and more setValue()

5. +++ b/tmgmt.theme.inc
   @@ -265,6 +266,18 @@ function theme_tmgmt_translator_review_form($variables) {
   +      if (isset($element[$key]['source_changed']['message'])) {
   +        $source_changed = '<div class="tmgmt_source_changed"><div class="messages messages--warning"><span class="tmgmt-review-message-inline"><span>' . $element[$key]['source_changed']['message']['#value'] . '</span>' . \Drupal::service('renderer')
   +            ->render($element[$key]['source_changed']['diff_button']) . '</span></div></div>';
   +        $diff_actions = '';
   

   Is the message here safe? could be a problem if we embed it into HTML like this, we don't have twig autoescape support in here. might look into finally refactoring this into a twig template when this is in.

Improved that, added more missing t() calls.

reviewFormElement() is now 300 lines of code, with enough conditions to let every complexity code analyze tool implode :(

Also, we don't have to do that here but i think we should also have a Resolve button for deleted items, which would then remove the data item? (Probably named Remove, not Resolve).

I think we can look at improving the data item removed scenario in a follow-up. Committed.