Verify stability with existing jobs when deleting a translator

As I can see, we have a check that makes sure that translators can't be removed if there are active jobs using it.

I will extend the tests.

Uploading patch with current progress. Added the functions hasTranslator and hasPlugin. There will probably be some test fails.

Also, removed the condition that is checking the active job attached to the translator. So now, we can't delete a translator if it has any job connected with it. In a followup, we should create some kind of automatization that deletes all connections between the translator and the (active) job.

Fixed the test fails...

Also, changed TranslatorTest for the error message, because now we can't delete translators that are connected with a job.

Created a followup for the discussion about dropping getTranslatorPlugin().

Added the fixes.

+++ b/src/Tests/TranslatorTest.php
@@ -62,8 +62,12 @@ class TranslatorTest extends TMGMTTestBase {
+    // With the new behaviour from #2538360, we cannot delete a translator if it
+    // is used by a node of any state.
+    $this->assertText(t('This translator cannot be deleted as long as there are active jobs using it.'));

About the last comment about the test, we have a check for the STATE_FINISHED. Should we also test all/some other states?

1. +++ b/src/Entity/Job.php
   @@ -403,9 +401,24 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
   +    else if (!$this->translator->target_id) {
   +      throw new TMGMTException('Cannot get the translator. The translator is missing.');
   +    }
   +    else if (!$this->translator->entity->hasPlugin()) {
   +      throw new TMGMTException('Cannot get the translator. The plugin is missing.');
   +    }
   +  }
   

   I think we can improve those messages a bit. What about...

   The job has no translator assigned.

   and

   The translator assigned to this job is missing the plugin.

   This method might be easier to follow have the if/elseif first and actually return it at the end.

2. +++ b/src/Entity/Job.php
   @@ -403,9 +401,24 @@ class Job extends ContentEntityBase implements EntityOwnerInterface, JobInterfac
   +  public function hasTranslator() {
   +    if ($this->translator->target_id && $this->translator->entity->hasPlugin()) {
   +      return TRUE;
   +    }
        return FALSE;
   

   The check that you are doing is already returning a boolean, so you can write that as return $this->translator->target_id && ...

   Also, to be sure, you want to check ->entity, since it is possible that it is referencing a non-existing translator, so an ID would be set but couldn't be loaded.

3. +++ b/src/JobInterface.php
   @@ -244,11 +244,22 @@ interface JobInterface extends ContentEntityInterface, EntityOwnerInterface {
   +   * @throws TMGMTException
   +   *   Throws an exception.
   

   that description is very helpful I would never have guessed that without that ;)

   Instead, decribe when an exception is thrown. Also, should have full namespace.

4. +++ b/src/Plugin/views/field/Translator.php
   @@ -18,10 +18,15 @@ use Drupal\views\ResultRow;
    
   +  /**
   +   * @param \Drupal\views\ResultRow $values
   +   * @return string
   +   */
      function render(ResultRow $values) {
   

   This is an implementation of a base class/interface, so just @inheritdoc.

5. +++ b/src/Plugin/views/field/Translator.php
   @@ -18,10 +18,15 @@ use Drupal\views\ResultRow;
   +      if ($job->hasTranslator()) {
   +        return $job->getTranslator() ? $job->getTranslator()->label() : t('Missing translator');
   +      }
   

   This logic doesn't make sense anymore, it will never go into the Missing Translator case anymore. That needs to become the else case or you need to remove the if again and use hasTranslator() on he first check.

6. +++ b/src/Tests/TranslatorTest.php
   @@ -62,8 +62,12 @@ class TranslatorTest extends TMGMTTestBase {
        $this->drupalPostForm(NULL, array(), 'Delete');
   -    $this->assertText(t('Add translator'));
   -    $this->assertNoText($translator->label());
   +    // With the new behaviour from #2538360, we cannot delete a translator if it
   +    // is used by a node of any state.
   +    $this->assertText(t('This translator cannot be deleted as long as there are active jobs using it.'));
   +
   +    $this->drupalGet('admin/config/regional/tmgmt_translator');
   +    $this->assertText($translator->label());
   

   Hm. AFAIK it was a feature that you can delete a translator that is only used on completed jobs, why is this changed?

Ok fixed the issues above, reverted and extended the test.

Also, after talking with @berdir, reverted back the condition for deleting translators only when the job is not in active state as it was at the beginning.

Uhm, uploaded the wrong patch.

+++ b/src/JobInterface.php
@@ -464,6 +475,9 @@ interface JobInterface extends ContentEntityInterface, EntityOwnerInterface {
    *   The translator plugin instance.
+   *
+   * @throws TMGMTException
+   *   Throws an exception.
    */
   public function getTranslatorPlugin();

This other method also needs more explanation about when it throws the exception.
The rest looks fine for me.

1. +++ b/src/Entity/Translator.php
   @@ -219,17 +219,22 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
   + * {@inheritdoc}
   + */
   +  public function getPlugin() {
   

   2 extra spaces needed for doc block.

2. +++ b/src/Entity/Translator.php
   @@ -219,17 +219,22 @@ class Translator extends ConfigEntityBase implements TranslatorInterface {
   +  public function hasPlugin() {
   +    if (!empty($this->plugin) && \Drupal::service('plugin.manager.tmgmt.translator')->hasDefinition($this->plugin)) {
   +      return TRUE;
        }
   -    catch (PluginException $e) {
   +    else {
   +      return FALSE;
   

   Else block can be removed and only return false after if condition.

3. +++ b/src/JobInterface.php
   @@ -244,11 +244,22 @@ interface JobInterface extends ContentEntityInterface, EntityOwnerInterface {
   +   *   Throws an exception when there is no translator assigned or when the translator is missing the plugin.
   

   Comment is longer than 80 characters.

4. +++ b/src/JobInterface.php
   @@ -464,6 +475,9 @@ interface JobInterface extends ContentEntityInterface, EntityOwnerInterface {
   +   *
   +   * @throws TMGMTException
   +   *   Throws an exception.
   

   Berdir's comment (#12/3).

Fixed other points above.

Added hasTranslator to the JobItemInterface and changed JobItemForm.

#2504639: Fatal error when clicking 'in progress' without a translator broke the tests. This patch should fix it.

+++ b/tmgmt.module
@@ -401,7 +401,7 @@ function tmgmt_translator_load_available($job) {
  * be modified or deleted. A translator is considered 'busy' if there are jobs
- * attached to it that are in an active state.
+ * attached to it that are in any state.
  *

left-over change, should be reverted too.

Looks good to me, we could hold on and try to find more possibly broken cases. One that we should possibly try is deleting a translator of a completed job and then viewing the job and job items again. Can you try that and extend the existing test for that?

But other than that, I think we should go ahead and get this in asap.

Here is the extended test with the fixes.

+++ b/tmgmt.module
@@ -401,7 +401,7 @@ function tmgmt_translator_load_available($job) {
+ * attached to it that are in any active state.

Still there?

Job messages config file changes for the test fails.

Nitpick, removed the "y" from any (#24).

Rebased.

+++ b/src/Tests/TranslatorTest.php
@@ -63,7 +65,18 @@ class TranslatorTest extends TMGMTTestBase {
+    $this->assertText($translator->label() . ' has been deleted.');

Revert the change here, this isn't translatable anymore.
After that I'll give my blessing :P.

Reverted. :)

Looks fine!

I think so too. Committed.