Replace custom mapping table with tmgmt remote mapping API

+++ b/tmgmt_mygengo.installundefined
@@ -64,3 +64,23 @@ function tmgmt_mygengo_update_7002() {
+  $select = db_select('tmgmt_mygengo_mappings', 'm')->fields('m');
+  $old_mappings = $select->execute()->fetchAll();

fetchAll() is not necessary, you can loop over the return value of execute().

Also, this can easily be written as a db_query('select * from {tmgmt_mygengo_mappings}') as it's not a dynamic query. Or then formatted correctly.

$result = db_select()
  ->fields()
  ->execute()

+++ b/tmgmt_mygengo.installundefined
@@ -64,3 +64,23 @@ function tmgmt_mygengo_update_7002() {
+  foreach ($old_mappings as $mapping) {
+    $data = array(
+      'tjid' => $mapping->tjid,
+      'data_item_key' => $mapping->data_item_key,
+      'remote_identifier_1' => $mapping->gorder_id,
+      'remote_identifier_2' => $mapping->gjid,
+    );
+    drupal_write_record('tmgmt_remote', $data);

Use a multi-value insert here. $insert = db_insert()->fields(), then just $insert->values() and after the loop, $insert->execute().

More later.

Raising this to major.

The current mapping is inflexible and I've seen various weird behaviors and bugs. Single mappings getting dropped suddenly, completely wrong mappings with wrong tjid's that didn't map the data item keys.

The form alter code is broken and doesn't check the job item id, so you end up with a random job id if you have multiple job items with the same data item keys.

If possible in anyway, we need to refactor the code to switch to the core mapping API for the new release.

Started working on this, will provide a patch tomorrow morning.

Here's a first patch. Tests are passing, manual tests were working as well. Will comment on a few changes with dreditor in the next comment.

+++ b/tmgmt_mygengo.installundefined
@@ -6,61 +6,46 @@
 function tmgmt_mygengo_update_7001() {
-  drupal_install_schema('tmgmt_mygengo');
+  // Removed as it relied on the schema.
...
 function tmgmt_mygengo_update_7002() {
-  $schema = tmgmt_mygengo_schema();
+  // Removed as it relied on the schema.

Never rely on hook_schema() in an update hook, you must copy the table definition. You already worked around the problem in 7002 but it gets more and more complicated as you change the original structure. Decided to just remove that and make the removal conditional.

+++ b/tmgmt_mygengo.installundefined
@@ -6,61 +6,46 @@
+    // Extract job item id and data item key.
+    list(, $tjiid, $data_item_key) = explode('][', $mapping->data_item_key, 3);

That's a trick that I'm using a few places now, making it a function is tricky as we still need the list() part.

One thing that we could consider for gengo jobs is something like a GengoJob($data) class, that allows us to do getJob(), getJobItem(), getDataItemKey() but that's stuff for later, if at all.

+++ b/tmgmt_mygengo.moduleundefined
@@ -139,9 +139,8 @@ function tmgmt_mygengo_add_revision_form_submit($form, &$form_state) {
-    $data = $item->getData($form_state['values'][$gengo_job_id . '_key_array']);
-    $data['#status'] = TMGMT_DATA_ITEM_STATE_PENDING;
-    $item->updateData($form_state['values'][$gengo_job_id . '_key_array'], $data);
+    $data_item_key = tmgmt_ensure_keys_array($form_state['values'][$gengo_job_id . '_data_item_key']);;
+    $item->updateData($data_item_key, array('#status' => TMGMT_DATA_ITEM_STATE_PENDING));

You can update specific things with updateData(), it's pretty neat. the tmgmt_ensure_keys_array() also wouldn't be necessary, it accepts both.

+++ b/tmgmt_mygengo.moduleundefined
@@ -222,54 +221,56 @@ function _tmgmt_mygengo_poll_submit($form, &$form_state) {
-  // Try to map jobs that have not yet been mapped.
-  $controller->mapGengoJobs($job);
-
-  // Get all available mappings.
-  $gengo_mappings = $controller->getMappedGengoData($job->tjid);
-
-  $connector = new TMGMTGengoConnector($job->getTranslator());
-  $response = NULL;
-  // Get Translations for mapped data.
-  if (empty($gengo_mappings)) {
-    drupal_set_message(t('Gengo service has not yet queued the jobs. Please try again later.'));
-    return;
-  }
-
-  try {
-    $response = $connector->getJobs(array_keys($gengo_mappings));
-    if (!empty($response->jobs)) {
-      foreach ($response->jobs as $response_job) {
-        $controller->saveTranslation($job, $gengo_mappings[$response_job->job_id]->data_item_key, $response_job);
-      }
-
-      drupal_set_message(t('All available translations from Gengo have been polled.'));
-    }
-  }
-  catch (TMGMTException $e) {
-    watchdog_exception('tmgmt_mygengo', $e);
-    drupal_set_message($e->getMessage(), 'error');
-  }
-
+  // Fetch everything for this job, this updates the mapping and saves
+  // translations if there are any.
+  $controller->fetchGengoJobs($job);
+  drupal_set_message(t('All available translations from Gengo have been polled.'));

This was actually partially done twice in the old code, as mapGengoJobs() also did saveTranslation() for existing mappings.

Also, we should imho use "fetch" or so in the UI for this (button + message), not poll.

+++ b/tmgmt_mygengo.moduleundefined
@@ -222,54 +221,56 @@ function _tmgmt_mygengo_poll_submit($form, &$form_state) {
   $job = tmgmt_job_load($tjid);
+  if (!$job) {
+    watchdog('tmgmt_mygengo', 'Failed to load translation job for @data', array('@data' => var_export($data, TRUE)), WATCHDOG_WARNING);
+    return;

Noticed fatal errors in the log when gengo for some reason calls the callback for jobs that got deleted or so.

+++ b/tmgmt_mygengo.moduleundefined
@@ -222,54 +221,56 @@ function _tmgmt_mygengo_poll_submit($form, &$form_state) {
+  $remotes = entity_get_controller('tmgmt_remote')->loadByLocalData($tjid, $tjiid, $data_item_key);
+  $remote = reset($remotes);
+  // Create a mapping for this job if we don't have one yet. Should not happen
+  // as we pre-create mappings with the order id..
+  if (!$remote) {
+    $item = tmgmt_job_item_load($tjiid);
+    $item->addRemoteMapping($data_item_key, NULL, array(
+      'remote_identifier_2' => $data->job_id,
+      'word_count' => $data->unit_count,
+      'remote_data' => array(
+        'credits' => $data->credits,
+        'tier' => $data->tier,
+      ),
+      // @todo: Add remote_url.
+    ));
+  }
+  elseif (empty($remote->remote_identifier_2)) {
+    $remote->remote_identifier_2 = $data->job_id;
+    $remote->word_count = $data->unit_count;
+    $remote->remote_data = array(
+      'credits' => $data->credits,
+      'tier' => $data->tier,
+    );
+    $remote->save();

The first case should actually never happen, as we always create a mapping for each data item key with the order id below. But I left it in for now.

This is also done three times now, in slightly different variations, not yet sure how to consolidate it.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -18,6 +18,9 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+  const GENGO_ORDER_ID_FIELD = 'remote_identifier_1';
+  const GENGO_JOB_ID_FIELD = 'remote_identifier_2';

I think I don't use those anymore atm.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -77,15 +80,7 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
-      $response = $this->sendJob($job);
-
-      // If we already receive jobs, process them.
-      if (!empty($response->jobs)) {
-        $this->processGengoJobsUponTranslationRequest($job, $response->jobs);
-      }
-      elseif(isset($response->order_id)) {
-        $this->initGengoMapping($job, $response->order_id);
-      }
+       $this->sendJob($job);

@@ -216,7 +211,15 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
-      return $connector->submitJob($translations);
+      $response = $connector->submitJob($translations);
+      // If we already receive jobs, process them.
+      if (!empty($response->jobs)) {
+        $this->processGengoJobsUponTranslationRequest($job, $response->jobs);
+      }
+      elseif (isset($response->order_id)) {
+        $this->initGengoMapping($job, $response->order_id, $translations);
+      }
+      return $response;

Moved this into sendJob() so that I have access to $translations in initGengoMapping.

As said before, we should imho have something like $translations = $this->buildTranslations($job) and then call $connector->submitJob() directly in requestTranslation() but that's right now a bit ugly due to the duplicate handling.

Which should be reworked now to have two mappings to the same gengo job. Then the UI (comment/revision) will be a lot easier to fix for duplicates.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -225,14 +228,19 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+      // If the status is approved or reviewable, we expect a body_tgt property,
+      // abort and log if it doesn't exist.
+      if (!isset($data->body_tgt)) {
+        $job->addMessage('Callback called for @key and status @status without translation.', array('@key' => $data->custom_data, '@status' => $data->status));
+        return;

Seeing this currently on the sandbox, just added a message for now so it's easier to debug. Will need to check with them why that is happening.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -296,50 +301,27 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+      $item->addRemoteMapping($data_item_key, NULL, array(

This is the immediate process case, where we don't have an order_id.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -363,77 +348,70 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
-  public function mapGengoJobs(TMGMTJob $job) {
...
+  public function fetchGengoJobs(TMGMTJob $job) {

This works quite a bit different now and is only called for the manual poll/fetch action. It basically does this:

1. Load remote mappings
2. Load unique order ids
3. Load those and check for not yet mapped job ids. This again is something that should not happen but let's make sure.
4. Then load all existing mapped and new jobs.
5. Create mappings if they don't exist yet based on custom_data.
6. Save translations if there are any.

+++ b/tmgmt_mygengo.plugin.incundefined
@@ -363,77 +348,70 @@ class TMGMTMyGengoTranslatorPluginController extends TMGMTDefaultTranslatorPlugi
+        // Collect job ids of that job. Use array_merge() as we have numerical
+        // array keys.
+        $order_job_ids = array();
+        if (!empty($response->order->jobs_available)) {
+          $order_job_ids = array_merge($order_job_ids, $response->order->jobs_available);
...
-        $gengo_job_ids += $response->order->jobs_available;

That's the += bug that I mentioned ;)

+++ b/tmgmt_mygengo.ui.incundefined
@@ -16,22 +16,14 @@ class TMGMTMyGengoTranslatorUIController extends TMGMTDefaultTranslatorUIControl
     $job = $item->getJob();
-    /* @var TMGMTMyGengoTranslatorPluginController $plugin */
-    $plugin = $job->getTranslatorController();
 
-    $mapping_data = array();
-    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);
-      // Store data item key for further use.
-      $info->key_array = $key_array;
+    $remotes = array();
+    $tmgmt_remote_controller = entity_get_controller('tmgmt_remote');
+    foreach ($tmgmt_remote_controller->loadByLocalData($item->tjid, $item->tjiid) as $remote) {
       // Create key which is used in the review form.
-      $key = str_replace('][', '|', implode('|', $key_array));
+      $key = str_replace('][', '|', $remote->data_item_key);
       // Add into mapping data array.
-      $mapping_data[$key] = $info;
+      $remotes[$key] = $remote;

I really like how this got a *lot* easier :)

+++ b/tmgmt_mygengo.moduleundefined
@@ -222,54 +221,56 @@ function _tmgmt_mygengo_poll_submit($form, &$form_state) {
-  $mygengo->mapGengoJobs($job);

One more thing, this call in the callback resulted in an exponentioally increasing time when callbacks were invoked for created gengo jobs. Every time the order was requested, all jobs were requested and all translations updated if possible. Now that method does even more but as mentioned above, it's no longer called here. On the callback, we just update the mapping for the provided job id.

A job with 10 items or so took multiple minutes, it's down to a few seconds now.

This one should apply.

Ok. This should fix the duplicate handling, they're now tracked as additional remote data of a mapping. Making them separate mappings to the same job might be nicer but is also quite a bit more work as we still need this temporary storage for at least the order case. We do have a separate issue open to improve this.

When revising data items with duplicates, then those are set to pending as well and are then correctly updated again.

Also partially fixed the broken review UI for multi-value fields. Comments still show up wrong, but I'd prefer to refactor that to use methods provided by core for this instead of making the mess here bigger.

Interdiff is big but most of that just adding another foreach to the review alter code.

Ok, this should fix the polling bugs. Poor job items, always they get forgotton.

Fixed a stupid condition that was the wrong way round in the update function and added a ton of tests for order mode callback calls + polling.

The mock callbacks will need to be improved, for example, right the jobs callback doesn't actually check what arguments he gets, he just returns the previously entered response object. That makes it hard to actually validate that it was called with the right ids. But that might affect other tests, so I'll wait with improving that.

Commited, updated changelog. While not perfect, this is certainly the way forward.