Change cancel to no longer allow re-submit, allow to create a new job with the same job items instead

1. +++ b/entity/tmgmt.entity.job.inc
   @@ -102,6 +102,17 @@ class TMGMTJob extends Entity {
   +    $clone->tjid = $clone->uid = $clone->changed = $clone->reference = NULL;
   

   I think this would be less confusing if you write separate lines. Feels weird to set completely different things in a single multi-assignment line.

   Also, uid should probably be set to the current user?

2. +++ b/ui/includes/tmgmt_ui.pages.inc
   @@ -550,6 +550,20 @@ function tmgmt_job_form($form, &$form_state, TMGMTJob $job, $op = 'edit') {
   +      '#attributes' => array('onclick' => 'return confirm("' . t('This action will duplicate current job and submit to the translator. Do you which to continue?') . '")'),
   ...
   +      '#attributes' => array('onclick' => 'return confirm("' . t('This action will cancel the job at the translator. Do you which to continue?') . '")'),
   

   onclick confirmations seem like a completely new behavior, Drupal usually uses confirm forms?

3. +++ b/ui/includes/tmgmt_ui.pages.inc
   @@ -575,6 +589,45 @@ function tmgmt_job_form($form, &$form_state, TMGMTJob $job, $op = 'edit') {
    /**
   + * Submit handler of the job translation cancel action.
   + */
   +function tmgmt_ui_job_cancel_translation(array $form, array &$form_state) {
   +  /** @var TMGMTJob $job */
   +  $job = entity_ui_form_submit_build_entity($form, $form_state);
   +  if (!$job->cancelTranslation()) {
   +    // This is the case when a translator does not support the cancel operation.
   +    // It would make more sense to not display the button for the action,
   +    // however we do not know if the translator is able to cancel a job until
   +    // we trigger the action.
   +    drupal_set_message(t('The translation could not be cancelled, the selected translator does not support this action.'), 'warning');
   +  }
   +}
   

   We should use the get messages to display the message that the translator might have added, not just invent something that might not be correct.

4. +++ b/ui/includes/tmgmt_ui.pages.inc
   @@ -575,6 +589,45 @@ function tmgmt_job_form($form, &$form_state, TMGMTJob $job, $op = 'edit') {
   +/**
   + * Submit handler to duplicate a job and submit it to a translator.
   + */
   +function tmgmt_ui_job_resubmit(array $form, array &$form_state) {
   +  /** @var TMGMTJob $job */
   +  $job = entity_ui_form_submit_build_entity($form, $form_state);
   +  $job_to_resubmit = $job->cloneAsUnprocessed();
   +  $job_to_resubmit->save();
   +  /** @var TMGMTJobItem $item */
   +  foreach ($job->getItems() as $item) {
   +    $item_to_resubmit = $item->cloneAsActive();
   +    $job_to_resubmit->addExistingItem($item_to_resubmit);
   +  }
   +
   +  tmgmt_ui_job_request_translation($job_to_resubmit);
   +
   +  $job->addMessage('Job has been resubmitted to the translator as a new job <a href="@url">#@id</a>.',
   +    array('@url' => url('admin/tmgmt/jobs/' . $job_to_resubmit->tjid), '@id' => $job_to_resubmit->tjid));
   +  $job_to_resubmit->addMessage('This job is a resubmission of the previously cancelled job <a href="@url">#@id</a>',
   +    array('@url' => url('admin/tmgmt/jobs/' . $job->tjid), '@id' => $job->tjid));
   +
   +  $form_state['redirect'] = 'admin/tmgmt/jobs/' . $job_to_resubmit->tjid;
   +}
   

   Why is this a single action?

   I'd expect a Duplicate, that *just* duplicates and then lets me go through the checkout process again?

   Chances are that I *don't* want to use the same translator again, otherwise I probably wouldn't have cancelled?

5. +++ b/ui/tmgmt_ui.test
   @@ -370,4 +370,67 @@ class TMGMTUITestCase extends TMGMTBaseTestCase {
   +      $this->assertEqual($_item->word_count, $item->word_count);
   

   word_count and the other statistics have to be re-calculated. They're not re-set, so we're not sure they are.

   I think there are two main reasons to cancel a job:

   a) not being happy with the translator. at all. That's where re-submit doesn't work.

   b) The source might have changed, e.g. the nodes were changed and need to be translated again. So we need to make sure that everything is properly re-calculated based on what exists right now. Here a retranslate could be useful, but there might be some options, the different text results in a different quote and so on, so it's ok to click twice.

Shouldn't resubmit_translation now be renamed to clone or duplicate or something like that?

Ok, yes, resubmit is fine, but I don't get the _transaction suffix, that makes no sense to me. Neither in the machine name, nor the button label (maybe Cancel/Re-submit job instead?).

Even with that change, the buttons don't work anymore, we have too many of them:

We can't have two cancel buttons. Ideas? We could make cancel a link, similar to the confirm forms, but that only helps a bit...

The menu items and operation handling should be in TMGMTJobUIController.

Oh, also, the resubmit messages are no longer correct ("has been resubmitted", for example).

Still not convinced about the button labels (especially the translation part, we don't do that anywhere else). Will try to sit together with Miro and discuss it through.

Recoverable fatal error: Argument 3 passed to tmgmt_ui_job_cancel() must be an instance of TMGMTJob, none given

No test for cancel?

I still *really* would like to see this as a link, we have too many buttons there. Have a look at how confirm_form() does this, it also supports a ?destination= override. I don't think we need to worry about the redirect queue, we don't have cancel yet on checkout anyway (although that's where we should have it and where it should probably be a button that deletes the job again and instead don't display a delete on that page, separate issue).

Also, we discussed this quite a bit here, and this is what we agreed on:
- Abort translation => Abort job.
- We discussed Abort vs. Cancel and agreed on Abort, that however means we need to do it consistently. The job state label needs to be renamed to Aborted, and we need a job item state for aborted as well. Aborted job items should not display their progress information in the view (it's still ok to view them and e.g. copy out translations in there, but it shouldn't be possible to accept or do anything with them, that right now possible, you can add translations, review it and save it as completely. Maybe only display cancel there for aborted job items and nothing else)
- We will not yet rename the constants and methods (but use abort for the new ones for job items), but open a follow-up issue for that part.
- Resubmit to translator => Resubmit. Instead, explain on the confirm form what this means more detailed, have a short but correct title/question (Resubmit as a new job?) and replace the stupid this can not be undone description (because it can) with something that explains what it does: This creates a new job with the same items which can then be submitted again.
- The abort confirm page also needs some work, it shouldn't use abort *and* cancel. Instead, "Abort this job?" and a description that explains a request to abort the job will be sent to the translator and it won't be possible to do anything with it if that succeeds
- The confirm forms are also not yet part of the operationForm() method, see delete for example. Also, looking at that, we do have a dead implementation for cancel there, which means we can replace that one.

1. +++ b/entity/tmgmt.entity.job.inc
   @@ -424,10 +438,12 @@ class TMGMTJob extends Entity {
      /**
   -   * Returns whether the state of this job is 'cancelled'.
   +   * Returns whether the state of this job is 'aborted'.
       *
       * @return boolean
   -   *   TRUE if the state is 'cancelled', FALSE otherwise.
   +   *   TRUE if the state is 'aborted', FALSE otherwise.
   +   *
   +   * @todo - we need to refactor this method to use the "abort" term.
       */
      public function isCancelled() {
        return $this->isState(TMGMT_JOB_STATE_CANCELLED);
   @@ -479,13 +495,15 @@ class TMGMTJob extends Entity {
   
   @@ -479,13 +495,15 @@ class TMGMTJob extends Entity {
      }
   

   Not sure what to do with the comments :( The mix with abort/cancel is a bit weird. Maybe clarify the @todo, because it now partially already uses the term "abort" ;) Just not in the method name, so it should say, @todo Rename method to isAborted() in http://drupal.org/node/....

2. +++ b/tmgmt.module
   @@ -51,6 +51,9 @@ define('TMGMT_JOB_STATE_ACCEPTED', 3);
     * to confirm it by returning TRUE in cancelTranslation().
   + *
   + * @todo - as of #2106637#comment-7993257 there will be a followup to rename
   + * the constant and methods that deal with cancel operation to "abort".
     */
   

   References don't work like this. We need to create a new issue and reference to that.

3. +++ b/ui/includes/tmgmt_ui.controller.job.inc
   @@ -73,6 +95,44 @@ class TMGMTJobUIController extends EntityDefaultUIController {
          case 'cancel':
            $entity->cancelTranslation();
            return t('Cancelled the translation job %label.', array('%label' => $entity->label()));
   +      case 'abort':
   +        if (!$entity->cancelTranslation()) {
   +          // This is the case when a translator does not support the cancel operation.
   +          // It would make more sense to not display the button for the action,
   +          // however we do not know if the translator is able to cancel a job until
   +          // we trigger the action.
   +          foreach ($entity->getMessagesSince() as $message) {
   

   We still have cancel and abort here now. We should only have one, the old cancel must be some left-over that was changed a long time ago.

4. +++ b/ui/includes/tmgmt_ui.controller.job.inc
   @@ -73,6 +95,44 @@ class TMGMTJobUIController extends EntityDefaultUIController {
   +
   +        // We want to redirect to the new job checkout page, could not find any
   +        // nicer way how to control a redirect from here.
   +        $this->path = $this->path . '/' . $job_to_resubmit->tjid;
   

   Yeah, that's a hack :( $this->path is the the overview path, this could theoretically lead to crazy side effects.

   We might have to override the submit callback that calls this function, where it uses $this->path for the redirect and do something differently if operation is resubmit.

Ok, testbot happy (had to kick it a bit, first testbot committed suicide while testing the patched) and I'm happy.

Removed cancel menu callback and confirm form in ui controller before commit.

Committed and pushed!