Improve job owner

I remember that we discussed a week ago and came to the conclusion:
- On submission we will switch the job owner.
- All interactions, also cron, will use the job owner.
- For continuous jobs we will allow to switch the user when editing the job.

Please update the job summary properly.

Yay! Near to a well defined job ownership situation.

1. +++ b/src/Form/JobForm.php
   @@ -173,6 +174,36 @@ class JobForm extends TmgmtFormBase {
   +      $roles = user_roles(TRUE, 'submit translation jobs');
   ...
   +        $filter = [];
   ...
   +        $filter = [
   ...
   +          'filter' => $filter,
   

   That's quite some workaround and not a single comment about what you do. There's no better helper?!
   You could check for if(in_array('authenticated', $roles))
   And only do the array_keys and _combine if specific roles need to be filtered for.

2. +++ b/src/Form/JobForm.php
   @@ -486,6 +517,12 @@ class JobForm extends TmgmtFormBase {
   +      $id = explode(')', explode('(', $form_state->getUserInput()['uid'])[1])[0];
   

   Uh, this takes the first bracket, not the last.

   Checkout EntityAutocomplete::extractEntityIdFromAutocompleteInput() how Drupal core properly matches this situation.

3. +++ b/src/Entity/Job.php
   @@ -615,6 +615,9 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
          $message = 'The translation job has been submitted.';
   ...
   +    if (!$this->isContinuous()) {
   ...
        $this->setState(static::STATE_ACTIVE, $message, $variables, $type);
   

   That reads like a strange position right in the middle of $message handling?

4. And we need tests for it. ;-)

+++ b/src/Form/JobForm.php
@@ -486,6 +517,12 @@ class JobForm extends TmgmtFormBase {
    */
   public function save(array $form, FormStateInterface $form_state) {
+    if ($this->entity->isContinuous()) {
+      $id = explode(')', explode('(', $form_state->getUserInput()['uid'])[1])[0];
+      if ($this->entity->getOwnerId() != $id) {
+        $this->entity->setOwnerId($id);
+      }
+    }

The function that miro mentioned is called automatically, just use the value in form state values, which is already checked and validated for you?

It's also quite likely that the value is already set and works without this because the form element is named after the field.

As discussed, check setting a breakpoint inside validateEntityAutocomplete and figure out why the id extraction is not properly called.

+++ b/src/Form/JobForm.php
@@ -173,6 +175,35 @@ class JobForm extends TmgmtFormBase {
+        '#default_value' => $job->getOwnerId() == 0 || $job->getOwnerId() == NULL ? User::load(\Drupal::currentUser()->id()) : $job->getOwner(),

If you really need this, then use empty($job->getOwnerId()) ? ...

Still, a User::load(0) should be possible for anonymous. Why as a special case?

User::load(NULL) wouldn't and the code explicitly avoids loading 0 and defaults to the current user instead. Which we could also do as a default value for that field, see how Node does it. Could still be 0 for existing jobs, though.

But the double check is indeed not needed because 0 == NULL, it will never go into the second condition.

You are implementing the feature that the job owner is switched on submission.
This needs specific test coverage.

Not sure if the current simple assert is enough for the continuous case or if we need to test submissions also.

1. +++ b/src/Entity/Job.php
   @@ -639,6 +639,9 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
      public function submitted($message = NULL, $variables = array(), $type = 'status') {
   +    if (!$this->isContinuous()) {
   +      $this->setOwnerId(\Drupal::currentUser()->id());
   +    }
        if (!isset($message)) {
   

   This is too late. submitted() is called by the translator, *after* it was submitted. if the translator used the owner, then it used the old one.

   Instead, set it in requestTranslation(), before calling the translator.

   We could test this by adjusting the message in the test translator to include the owner. This might break some other tests though.

2. +++ b/src/Tests/TMGMTTestBase.php
   @@ -88,6 +88,7 @@ abstract class TMGMTTestBase extends WebTestBase {
          'administer tmgmt',
   +      'submit translation jobs',
   

   this is the wrong fix I think. Users that have administer tmgmt can also submit content. You will have to get roles that have either this or the other permission.

Thanks, committed.

We might want to open an issue to consider actually showing the owner somewhere now that the value makes sense?

Job overview maybe?