Problem/Motivation

Aborting a translation currently does not lead to any notification of Gengo.

Proposed resolution

Implement.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

miro_dietiker’s picture

This applies both to aboring on Drupal side and aborting on gengo side.
Either an immediate update should happen, or the state should be pulled on button press.
Both abort states should be in sync to avoid malfunction.

Bambell’s picture

Status: Active » Needs review
FileSize
3.54 KB

Here we go. Adds a bit of overhead, but personally I would also query again the API for the jobs' status, to double check, and would clearly return the ID of the jobs that were aborted to reassure the user.

miro_dietiker’s picture

Status: Needs review » Needs work

A quick review. No reason to double check. APIs do what they describe and you get a success/error message.

  1. +++ b/tmgmt_mygengo.module
    @@ -218,3 +218,48 @@ function tmgmt_mygengo_access_check() {
    +function tmgmt_mygengo_form_tmgmt_job_abort_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
    ...
    +  $form['actions']['submit']['#submit'][] = '_tmgmt_mygengo_abort_form_submit';
    

    This is wrong. It's added to every abort form and also applies if it's not a Gengo provider.
    There is the JobInterface::abortTranslation() and you don't need to attach through a form at all.
    For an example see: tmgmt_oht/src/Plugin/tmgmt/Translator/OhtTranslator.php

  2. +++ b/tmgmt_mygengo.module
    @@ -218,3 +218,48 @@ function tmgmt_mygengo_access_check() {
    +          drupal_set_message(t('Unable to abort job @gengo_job_id. It has already been translated or is in progress.', ['@gengo_job_id' => $gengo_job_id]), 'error');
    ...
    +        drupal_set_message(t('Unable to abort job @gengo_job_id. Error: @error', ['@gengo_job_id' => $gengo_job_id, '@error' => $e->getMessage()]), 'error');
    

    That's not really an error. Also we don't use drupal_set_message() in tmgmt. We attach messages to the job.
    The user can abort a job always. But we can not tell the user if it leads to a cancellation at the provider. Also many providers will not refund at all if you cancel.
    Thus, we simply try to cancel the jobs and create a SINGLE message on the job like X of Y messages have been aborted at Gengo.

Bambell’s picture

Assigned: Unassigned » Bambell
Status: Needs work » Needs review
FileSize
3.99 KB
5.78 KB
9.65 KB
9.57 KB

I have modified the patch in order to implement JobInterface::abortTranslation(), instead of hooking into a form, and no longer use use drupal_set_message(), but rather set messages to job items. With the current implementation, jobs can always be aborted on TMGMT's side. A single message is added to the job item ("X job out of Y has been aborted at Gengo" or "X jobs out of Y have been aborted at Gengo"). The message is no longer an error. Would it be appropriate to set it as a warning in case job(s) couldn't be aborted at Gengo?

Screenshot showing the message if all jobs are aborted.

Screenshot showing the message if 1 job couldn't be aborted.

miro_dietiker’s picture

Status: Needs review » Needs work

Yeah looks good now. And yeah, if not all can be stopped, warning would be the best level.
I think you should add to the warning "Gengo does not allow aborting jobs that are already in translation."

Bambell’s picture

Status: Needs work » Needs review
FileSize
18.03 KB
4.75 KB
1.96 KB

I re-rolled the patch, changed the wording in case job(s) couldn't be canceled and changed this message level to a warning.

Job abortion warning.

And yeah, if not all can be stopped, warning would be the best level.

We can also abort the order at Gengo. If there's a job in the order that can't be canceled, I believe that none of the order's jobs is canceled. We could then deny abortion on TMGMT's side.

miro_dietiker’s picture

Status: Needs review » Needs work

I like the presentation almost wanted to commit.

+++ b/src/Plugin/tmgmt/Translator/MyGengoTranslator.php
@@ -558,4 +561,59 @@ class MyGengoTranslator extends TranslatorPluginBase implements ContainerFactory
+          $job_item->addMessage('@count_aborted jobs out of @count_total have been aborted at Gengo. Gengo does not allow aborting jobs that are already in translation.', ['@count_aborted' => $count_aborted, '@count_total' => count($remotes)], 'warning');
...
+          $job_item->addMessage('@count_aborted jobs out of @count_total have been aborted at Gengo.', ['@count_aborted' => $count_aborted, '@count_total' => count($remotes)]);
...
+          $job_item->addMessage('@count_aborted job out of @count_total has been aborted at Gengo. Gengo does not allow aborting jobs that are already in translation.', ['@count_aborted' => $count_aborted, '@count_total' => count($remotes)], 'warning');
...
+          $job_item->addMessage('@count_aborted job out of @count_total has been aborted at Gengo.', ['@count_aborted' => $count_aborted, '@count_total' => count($remotes)]);

I feel blind.
I see only 2 real cases. Why 4?

Bambell’s picture

FileSize
2.03 KB
4.15 KB

As discussed, I changed the wording from "X job/jobs out of Y has/have been aborted at Gengo" to "Aborted X out of Y jobs at Gengo". I tried considering the singular form of "jobs" with formatPlural(), but strangely I couldn't get it to work :

$total_jobs = \Drupal::translation()->formatPlural($count_remotes, '1 job', '@count jobs');

With that wording, though, it's so very unlikely that a node will only have a single field that we can probably leave the singular form ("job") out?

Bambell’s picture

Status: Needs work » Needs review
miro_dietiker’s picture

Status: Needs review » Fixed

Works for me now. :-)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.