Disallow item deletion after submission and support item abort

Assigning to me.

Initial patch.

Updated patch, removed resubmit action and fixed test fails.

+++ b/src/Entity/ListBuilder/JobItemListBuilder.php
@@ -32,6 +33,16 @@ class JobItemListBuilder extends EntityListBuilder {
+      $operations['cancel'] = array(
+        'url' => $entity->urlInfo('abort-form')->setOption('query', array('destination' => Url::fromRoute('<current>')->getInternalPath())),
+        'title' => t('Abort'),
+        'weight' => 10,
+      );

Should not be this operations['abort'] ?

I was looking for some example in JobListBuilder.php and there is 'cancel' for abort action, so I added 'cancel' too, dunno should I replace it with 'abort' ?

Updated patch.

Updated test a bit.

Updated patch.

1. +++ b/src/Entity/ListBuilder/JobItemListBuilder.php
   @@ -32,6 +33,16 @@ class JobItemListBuilder extends EntityListBuilder {
   +    if ($entity->getJob()->isActive() && $entity->isActive() && !$entity->isAborted()) {
   

   how can the entity be active and aborted at the same time? shouldn't be necessary to check both?

2. +++ b/src/Form/JobItemAbortForm.php
   @@ -0,0 +1,49 @@
   +  public function getDescription() {
   +    return $this->t('This is only an internal decision and the translator does not know about it. After the action the job item will be aborted and the only remaining action will be deleting it.');
   +  }
   

   I don't understand why we would allow to delete then (other than deleting as part of the whole job). Since we don't notify the provider anyway, this extra step would be kinda pointless if we then can delete anyway?

3. +++ b/src/Form/JobItemForm.php
   @@ -149,7 +148,7 @@ class JobItemForm extends TmgmtFormBase {
   -      '#access' => $item->isNeedsReview(),
   +      '#access' => $item->isNeedsReview() && !$item->isAborted(),
          '#validate' => array('::validateForm', '::validateJobItem'),
   

   same, aborted is a different step, it can't be in needs review and aborted at the same time?

4. +++ b/src/Form/JobItemForm.php
   @@ -186,6 +185,15 @@ class JobItemForm extends TmgmtFormBase {
   +      // Don't run validations, so the user can always delete the job item.
   +      '#limit_validation_errors' => array(),
   +    );
   

   This is why delete in many cases is a link now, not a button.

5. +++ b/src/JobItemInterface.php
   @@ -161,6 +161,18 @@ interface JobItemInterface extends ContentEntityInterface {
   +   * Attempts to abort the translation job item.
   +   *
   +   * Already accepted job items can not be aborted, submitted job items only if
   +   * supported by the translator plugin. Always use this method if you want to
   +   * abort a translation job item.
   +   *
   +   * @return bool
   +   *   TRUE if the translation job item was aborted, FALSE otherwise.
   

   copied from job, this is a lie! :)

   we don't ask the plugin, we don't really return true/false but throw an exception, ..

6. +++ b/tmgmt.module
   @@ -686,13 +687,13 @@ function tmgmt_job_request_translation(JobInterface $job) {
     */
   -function tmgmt_write_request_messages(JobInterface $job) {
   +function tmgmt_write_request_messages($job) {
   

   Yes, removing this is problematic. At least we should have a specific interface for the methods we need here. Seems like that could be a separate issue, we can just duplicate the few lines of code for now.

7. +++ b/tmgmt.module
   @@ -703,7 +704,7 @@ function tmgmt_write_request_messages(JobInterface $job) {
        if ($text = $message->getMessage()) {
   -      drupal_set_message($text, $message->getType());
   +      drupal_set_message(new FormattableMarkup($text, []), $message->getType());
        }
   

   this is wrong, getMessage() already returns a translated string.

Updated patch and created follow-up issue for code refactoring - #2689025: Refactor code where getMessagesSince() is used.

1. +++ b/src/Entity/ListBuilder/JobItemListBuilder.php
   @@ -32,6 +33,20 @@ class JobItemListBuilder extends EntityListBuilder {
   +    // Do not display delete on active job items, display abort.
   +    if ($entity->getJob()->isActive() && $entity->isActive()) {
   +      unset($operations['delete']);
   

   what happens for continuous jobs for this?

   We might be able to just rely on the job item status now. what about needs review? I think you can abort then too.

2. +++ b/src/Entity/ListBuilder/JobItemListBuilder.php
   @@ -32,6 +33,20 @@ class JobItemListBuilder extends EntityListBuilder {
   +    // Do not display delete on aborted job items also.
   +    if ($entity->isAborted()) {
   +      unset($operations['delete']);
   

   I think we can simplify this by only allowing access if the job item is inactive, if not, always unset. Then the other if doesn't have to duplicate the unset.

   And actually, this logic belongs in JobItemAccessControlHandler I'd say.

3. +++ b/src/Form/JobItemAbortForm.php
   @@ -0,0 +1,61 @@
   +    return $this->t('This is only an internal decision and the translator does not know about it. After the action the job item will be aborted.');
   

   Aborted job items can no longer be accepted. The provider will not be notified.

4. +++ b/src/JobItemInterface.php
   @@ -161,6 +161,20 @@ interface JobItemInterface extends ContentEntityInterface {
   +   *
   +   * @return int
   +   *   The updated state of the job item if it could be set.
   +   *
   

   that doesn't seem very useful.

5. +++ b/src/Tests/TMGMTUiTest.php
   @@ -753,7 +753,7 @@ class TMGMTUiTest extends EntityTestBase {
       * @todo This will in future check that delete is not allowed.
   

   this todo can be removed now.

6. +++ b/src/Tests/TMGMTUiTest.php
   @@ -772,21 +772,48 @@ class TMGMTUiTest extends EntityTestBase {
   +    // Reload job and check its state and state of its item.
   +    \Drupal::entityManager()->getStorage('tmgmt_job')->resetCache();
   +    $job = Job::load($job->id());
   +    $this->assertTrue($job->isActive());
   

   We need to check that a job changes to finished if all job items are either accepted or aborted. See tmgmt_job_check_finished()

Fixed bugs from #19.

Almost there now I think.

1. +++ b/src/Entity/ListBuilder/JobItemListBuilder.php
   @@ -33,17 +33,16 @@ class JobItemListBuilder extends EntityListBuilder {
   -    // Do not display delete on aborted job items also.
   -    if ($entity->isAborted()) {
   +    // Display delete only on inactive job items.
   +    if (!$entity->access('delete')) {
          unset($operations['delete']);
        }
   

   Now that yo uadded it to checkAccess(), this should just work and you can delete this. Check how the parent implementation checks delete access.

2. +++ b/src/JobItemInterface.php
   @@ -166,8 +166,8 @@ interface JobItemInterface extends ContentEntityInterface {
       *
   -   * @return int
   -   *   The updated state of the job item if it could be set.
   +   * @return bool
   +   *   TRUE if job item is successfully aborted.
       *
   

   Sorry for not being clear. I think we can remove the @return completely (also in the implementation). If it doesn't work then we throw an exception, so we don't have to return anything on success.

Fixed bugs from #21 too.

1. +++ b/src/Form/JobItemAbortForm.php
   @@ -0,0 +1,61 @@
   +  public function getQuestion() {
   +    return $this->t('Abort this job item?', array('%title' => $this->entity->label()));
   +  }
   

   Title is not used here.

   Maybe we want to update message similary to one for deleting jobs: "Are you sure you want to abort the job item %title?"

2. +++ b/src/Form/JobItemAbortForm.php
   @@ -0,0 +1,61 @@
   +    $entity = $this->entity;
   +    if (!$entity->abortTranslation()) {
   +      foreach ($entity->getMessagesSince() as $message) {
   

   This can throw an exception, I think we want to handle it?

3. +++ b/tmgmt.routing.yml
   @@ -96,6 +96,13 @@ entity.tmgmt_job_item.delete_form:
   +entity.tmgmt_job_item.abort_form:
   ...
   +  requirements:
   +    _entity_access: tmgmt_job_item.update
   

   This will require a custom access check. We should check if a job item is not already in any state other than "Active" (which gives an Exception right now)

Updated patch.

+++ b/src/Form/JobItemAbortForm.php
@@ -42,19 +43,26 @@ class JobItemAbortForm extends ContentEntityConfirmFormBase {
     }
+    catch(TMGMTException $e) {
+      drupal_set_message(t('Job item cannot be aborted: %error.', array(
+        '%error' => $e->getMessage(),
+      )), 'error');
+    }
     $form_state->setRedirectUrl($this->entity->toUrl());

I guess this couldn't really happen since you wouldn't be able to access that page anymore. But doesn't hurt to have it.

Thanks, committed.