Support job comments

I tried to do something here. For the beginning only attached comments to dataitems in the review tool. Was also thinking how we can store it locally and not load it, however not sure how we can do this effectively as we would need to "sync" local comment storage with remote. See the attached screenshot for UI feedback, will provide patch after some cleanup.

Following functionality implemented:

- comments are displayed below each data item in the review form (only for tmgmt_ui_node_translation_review_form not sure how are other translatable objects handled)
- you can add new comment using the new comment field
- comments are cached locally for 15 minutes, so there should not be too much overhead

So for faster review here are a few mockups

mappings not yet received:
- gengo service has not yet provided mappings, so we are not able to post comments - not displaying the comment button

thread short no textarea:
- a few comments added, button to add new comment has not been clicked

thread long no textarea:
- just to show how looks longer thread

thread long with textarea:
- this is how it looks after clickin the comment btn. the textarea gets focus also
- clicking cancel will hide textarea
- saving the comment will hid textarea add the comment and scroll down to the latest comment

And here is the patch.

no other action then posting a comment is implemented. However at the current state implementing further workflow is not that difficult anymore.

Looks nice.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -198,7 +198,7 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
-    if ($response->code != 200) {
+    if ($response->code != 200 && $response->code != 201) {
       throw new TMGMTException('Unable to connect to Gengo service due to following error: @error at @url',

Looking forward to Guzzle in 8.x with sane error/exception handling :)

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -462,6 +462,44 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+   * @param TMGMTTranslator $translator
+   * @param int $gengo_job_id
+   * @param string $comment
+   *   Comment text.
...
+   * @param TMGMTTranslator $translator
+   * @param $gengo_job_id
+   * @param boolean $reload

Doesn't need to be elaborate, but should follow the minimal coding standards, which means a short sentence for each argument below the @param.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -462,6 +462,44 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+  function gengoPostComment(TMGMTTranslator $translator, $gengo_job_id, $comment) {
+    $this->requestFromGengo($translator, 'translate/job/' . $gengo_job_id . '/comment', 'POST', array('body' => $comment));

At some point we have to make sense of that method naming mess again with someMethod(), otherMethodGengo() and gengoYetAnotherMethod().

I like the way we're doing it in OHT now, with a separate class that's used internally. Not a problem that needs to be solved here.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -462,6 +462,44 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+    $cid = 'tmgmt_mygengo_comments_' . $gengo_job_id;
+    $cache = cache_get($cid, 'cache');

We have a cache_tmgmt bin that you can use here.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -462,6 +462,44 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+    if (isset($cache->data) && !$reload && ($cache->created + TMGMT_MYGENGO_COMMENTS_CACHE_EXPIRE > REQUEST_TIME)) {
...
+    cache_set($cid, $data, 'cache', CACHE_TEMPORARY);

Instead of CACHE_TEMPORARY, you can pass REQUEST_TIME + that constant to cache_set(), then $cache->expire equals that value.

The advantage is that certain backends can deal with that natively and won't even return expired times (database backend does)

+++ b/tmgmt_mygengo.ui.incundefined
@@ -15,6 +15,108 @@ class TMGMTMyGengoTranslatorUIController extends TMGMTDefaultTranslatorUIControl
   public function reviewForm($form, &$form_state, TMGMTJobItem $item) {
+    $job = $item->getJob();
+    /* @var TMGMTMyGengoTranslatorPluginController $plugin */

Wondering if and how we should add more generic support for this stuff to tmgmt core and e.g. just have a method that can contain comments or other information to be displayed, and display it if there is.

We can get this in and then maybe look into that later, as we want to have something to get started asap I think.

Exceptions - that is a nightmare currently...

Comments - added

Separate connector class - yes, for OHT I basically rewrote the plugin, so also created connector class, however that is not so much work, will create follow up for gengo

Cache - implemented as you suggested

Generic comments - behaviour is there different for different services. For OHT we add comments to job item as a whole, here we add comments for data items. But I think it is possible to support both in core.

Thanks, if possible please add an interdiff so that it's easier to review the changes :)

Yes, with core support, I'm not talking about commits specifically. Just that core provides a place to add per-data-item information that a plugin can just implement and return stuff and doesn't have to alter things like crazy :)

For example, we could use that (and implement as part of) translation revision support. The idea there is simply that we have a #revision property on the data item that is set (there's a patch for that already in the translation update issue) and display information about old translation revisions. And Gengo could just extend that method and also add the comments.

I just realised that the other issue has been pushed, so I made diff against current 7.x-1.x, so I am not able to make an interdiff anymore as it would include the other changes as well :(

Sure, there are cases where it doesn't work. I'm usually using feature branches that I rebase on top of 7.x-1.x, then I can almost always diff against the last commit to get an interdiff, even after a merge + changes. You can also use "interdiff" to create an interdiff of two patches files but that can't really handle some changes and is often convoluted.

Going to test and commit this asap :)

This isn't working for me yet, looks like we have a problem with those mappings.

The mappings weren't set up immediately for me, only on demand when the job was sent back and it looks like we have a different structure then, mapping key is job_id|item_id|data_item_key but it only seems to expect item_id|data_item_key, see attached screenshot.

+++ b/tmgmt_mygengo.ui.incundefined
@@ -15,6 +15,108 @@ class TMGMTMyGengoTranslatorUIController extends TMGMTDefaultTranslatorUIControl
+    foreach ($form['review'] as $key => $info) {
+      if (!is_array($info)) {
+        continue;

This should use foreach (element_children($form['review'] as $key) instead, that's how you correctly loop over form elements. Note that you get the key and then need to assign it with $form['review'][$key]

Also, ][ vs | data_item_key delimiters...

Not setting mappings immediately is gengo behaviour. It just places orders and actual jobs do not exist yet. As soon as jobs get created at gengo side we are able to map them and this in most cases does not happen upon job submission.

Your finding with data keys is interesting - it looks like we get different key when you poll the job than with one that is mapped immediately.

Fixed, please see the patch.

+++ b/tmgmt_mygengo.ui.incundefined
@@ -15,6 +15,111 @@ class TMGMTMyGengoTranslatorUIController extends TMGMTDefaultTranslatorUIControl
+    foreach ($plugin->getMappedGengoData($job->tjid) as $info) {
+      $key_array = tmgmt_ensure_keys_array($info->data_item_key);
+      // Remove tjid.
+      array_shift($key_array);
+      // Remove tjjid.
+      array_shift($key_array);
+      // Create key which is used in the review form.
+      $key = str_replace('][', '|', implode('|', $key_array));
+      // Add into mapping data array.
+      $mapping_data[$key] = $info;

Yes, this works fine now.

But don't we have to fix the fact that our mapping isn't consistent? It worked for you before, wouldn't it be broken now?

@Berdir - I was not able to reproduce such case anymore.

We determine data item key as array keys of following: $data = tmgmt_flatten_data($job->getData()) and then we prepend it with tjid. So the case I described above should not happen, and as I reloaded my previous installation I was not able to verify this by looking at data :|

@miro_dietiker - we provide a textarea for adding an initial comment at checkout. This same comment is then added to each job at gengo side, so we can have like dozen of jobs at gengo with the same comment.

About initial comments, My idea is still that you can look at and prepare job items during checkout and e.g. allow to ignore certain data items from being translated, or use a built-in glossary to select a pre-existing translation. At that point, it would also be possible to add specific comments, store them in #comments and use that for the initial comment. But that's something for follow-up issues :)

What we discussed is that we'd like to have identical test coverage against our mock module (for quick feedback in the issue queue and on d.o) and using the sandbox ( to make sure it works against their real API and with that also test our mock responses).

The way this would work is an abstract base class that defines the test methods but doesn't provide getInfo(), Those test methods call abstract methods that the mock and sandbox child classes will have to implement (e.g. configureTranslator($translator), onfigureTranslatorWrongKey($translator), invokeCallback($job)).

This obviously overlaps with #1966306: Automated testing. So we shouldn't go too far here, I'm not sure what's the best way to start, maybe focus on on the mock for now but keep the above things in mind so that we can easily refactor it in the other issue, by e.g. already move mock specific stuff into separate methods but without introducing a base class yet.

You will have to extend the mock callbacks so that they work correctly with the mapping, that might not be the case yet.

Having test coverage of this process will also allow us to replace the custom mapping table and API with the tmgmt core mapping API.

#1970570: Replace custom mapping table with tmgmt remote mapping API

The test coverage is within one method so can be easily moved/refactored later on.

Also I now reproduced the behaviour when custom data key was populated without job item id - processGengoJobsUponTranslationRequest()

and patch with merged in changes from 7.x-1.x

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -305,8 +305,7 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
-      $key = array_slice(tmgmt_ensure_keys_array($key), 1);
-      $this->saveTranslation($job, $key, $response_job);
+      $this->saveTranslation($job, array_slice(tmgmt_ensure_keys_array($key), 1), $response_job);

Not sure I can see a difference here? :)

Tests look like a good start. It's a bit strange that we do machine translation and then add a comment. Is that even possible with the real service?

The difference is that $key does not change and its value is used ad data_item_key for the mapping.

The machine tier is used to instantly receive translation, so we do not need to do translation submission. It would however work even with standard tier, but tmgmt core will produce warning as it is expecting #translation in the review form.

BTW i think the behaviour should be that the review form is either not accessible or unless the #translation is present the translation textarea would be disabled as well. But that is out of scope of this task.

Ah, right, now I can see the different with $key... nasty.

Commited and pushed.

Also opened some follow-ups/related tasks:
- #1970730: Undefined index #translation on job item page when translations are missing
- #1970740: Give plugins an easy way to add additional information/actions to a data item on the job item page
- #1970748: Improve job item page when there are no translations yet

When you notice bugs or so, make sure to create an issue if you can't find an existing one, then we don't forget it.

Setting the status would help.