Autocomplete for local translator assignment broken if no role has the "provide translation services" permission

Fixing title.

This problem also exists if *no* role has the permission, that will still fail?

Yes, that is a situation where it won't work and won't find anything, but we still need to make sure that we don't throw an exception that's as unhelpful as this.

You can also improve the UI to show a warning in that case, but the API function should be able to deal with that and return an empty array.

+++ b/translators/tmgmt_local/tmgmt_local.module
@@ -844,8 +844,15 @@ function tmgmt_local_capabilities_query($source_language = NULL, $target_languag
   // Only consider users that have the 'provide translation services' perm.
-  $roles = user_roles(TRUE, 'provide translation services');
-  $query->leftJoin('users_roles', 'ur', "ur.uid = u.uid AND ur.rid IN (:roles)", array(':roles' => implode(', ', array_keys($roles))));
+  $query->leftJoin('users_roles', 'ur', 'ur.uid = u.uid');
+
+  $roles = tmgmt_local_translator_roles();
+  if (!in_array('authenticated user', $roles)) {
+    $rids = array_keys($roles);
+    $rids = !empty($rids) ? $rids : NULL;
+    $query->condition('ur.rid', $rids);
+    $query->isNotNull('ur.rid');
+  }

that's a crazy trick ;)

It's very simple, here's a suggested interdiff for that function (against your latest patch). We can almost go back to the original join, just need an initial check for empty(), then return array() and wrap the whole join in an if () for the authenticated role (which is a performance optimization that isn't related to this bug, but lets' fix it while we are here.

The test is also very weird, but I didn't fix that. If you look, you'll notice that you visit that page without task id, because there *are none*. $tsid is empty (also not sure what a tsid is ;). And the array_keys() on EFQ is also bogus, because if there would be one, then it would contain the entity type on the first array level.

The bug is also visible on the checkout form.

So, instead of all this back and forth with permission, why not refactor the test to take the drupalCreateRole() out of setUp(), move in the test and before that, access the checkout page. There you should see the error on HEAD.

Then, create user, check again, verify it's still empty.

Then, assign languages, then visit the checkout page again, confirm that you can see the user. Then submit.

You might need to add it to the other test methods too. but it's just a single line.

1. +++ b/translators/tmgmt_local/tmgmt_local.module
   @@ -588,8 +588,11 @@ function tmgmt_local_get_translators_for_tasks($tasks) {
        return call_user_func_array('array_intersect', $translators);
      }
   +  elseif (count($translators) == 1) {
   +    return array_shift($translators);
   +  }
    
   -  return array_shift($translators);
   +  return $translators;
    }
   

   Was a bit confused by this, the final return means it's empty, right? Can we hardcode that to return array(); with a quick comment?

2. +++ b/translators/tmgmt_local/tmgmt_local.module
   @@ -841,14 +854,27 @@ function tmgmt_local_capabilities_query($source_language = NULL, $target_languag
   +  // If authenticated user is has the required permission we do not have to do
   +  // the role check.
   +  if (!in_array('authenticated user', $roles)) {
   +    $query->leftJoin('users_roles', 'ur', 'ur.uid = u.uid');
   +    $query->leftJoin('users_roles', 'ur', "ur.uid = u.uid AND ur.rid IN (:roles)", array(':roles' => implode(', ', array_keys($roles))));
   +  }
   

   Now there are two joins? :) No idea what it will do exactly but it should interesting, because the second join is aliased, so the second ON adds conditions on the first join :p

3. +++ b/translators/tmgmt_local/tmgmt_local.test
   @@ -151,13 +141,30 @@ class TMGMTLocalTestCase extends TMGMTBaseTestCase {
   -    $job->addItem('test_source', 'test', '2');
   +    $job->save();
   +    // Request translation directly on controller to bypass the language checks.
   +    tmgmt_translator_load('local')->getController()->requestTranslation($job);
   ...
   +    $query = new EntityFieldQuery();
   +    $result = $query->entityCondition('entity_type', 'tmgmt_local_task')->propertyCondition('tjid', $job->tjid)->execute();
   +    $tasks_ids = array_keys($result['tmgmt_local_task']);
   +    $task_id = reset($tasks_ids);
   ...
   +      array('@url' => url('admin/people/permissions'))));
   ...
    
   

   This is not what I meant :)

   What I'm interested in is test coverage for the job checkout form, not the re-assign local task pages. You will never even get that far, right now with this bug as you can't submit any jobs through the UI in the first place.

   The suggested workflow of testing before/after creating a user with the necessary permissions and capabilities was meant for that, not when you already have a job.

   I can see that we already have test coverage for non-existing capabilities, so all you need to add IMHO is a drupalGet() to the job checkout page before you create the translator and it should fail.

Ok... so apparently, the originally reported bug didn't exist anymore, because the query did a manual implode(), so the empty array case wasn't empty but an empty string, resulting in a IN ('') condition which apparently didn't fail.

However, this is wrong, as the comma separated list of the values is then passed as a single argument, I'm not even sure why it does work, I guess MySQL is drunk again, but I'm pretty sure that PostgreSQL wouldn't allow you to compare integers with strings like that. Instead, IN (:something) needs to be passed an array and will then internally split it up into multiple placeholders.

So what we need to do is remove that implode() and then the attached test fails as expected. and works with the fix.

The patch now tests the case of 0 roles with the permission, 1 role and 2 roles, which I expect would fail a more strict database like PostgreSQL.

I also wanted to test to submit the job through the UI, but that doesn't work because the UI doesn't have a comment field, so we can't submit a comment in the UI for the local translator (WTF?)

Improved the comment, committed and pushed.