A callback (containing the translation) can fail.

Customers should be able to pull the translation on click... or a translation update.

Add a button per job or/and per job item to pull the translation (again) from OHT.
Overwrite the text field if not #customized (future feature of tmgmt core).

CommentFileSizeAuthor
#12 add_pull_translation-1899670-12.patch4.57 KBmbovan
#9 add_pull_translation-1899670-9-interdiff.txt4.92 KBmbovan
#9 add_pull_translation-1899670-9.patch5.35 KBmbovan
#7 add_pull_translation-1899670-7-interdiff.txt4.28 KBmbovan
#7 add_pull_translation-1899670-7.patch4.08 KBmbovan
#5 add_pull_translation-1899670-5-interdiff.txt4.58 KBmbovan
#5 add_pull_translation-1899670-5.patch5.31 KBmbovan
#3 add_pull_translation-1899670-3.patch3.92 KBmbovan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented

Core related #1969434: Poll / Pull translator button

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Priority: Normal » Major
Issue summary: View changes

We defined that a pull button is a minimum requirement for QA in all TSP plugins.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Active » Needs review
FileSize
add_pull_translation-1899670-3.patch3.92 KB

Added a "Pull translations" button.

Not sure about error/status messages...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work 
+++ b/tmgmt_oht.plugin.inc
@@ -291,6 +291,40 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+        if (empty($remote->remote_identifier_1)) {
+          watchdog('tmgmt_oht', 'Could not retrieve project information about job item id (@tjiid).', array('@tjiid' => $job->tjiid), WATCHDOG_ERROR);
+          continue;

in here, use the addMessage() stuff, not wtachdog for those errors.

It would also be weird if we show a successfull message at the end when there were errors. this should possibly have a boolean return value or maybe we should just do the addMessage() on the job here if there are no errors.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
add_pull_translation-1899670-5.patch5.31 KB
add_pull_translation-1899670-5-interdiff.txt4.58 KB

Tried to improve it with this patch...

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker commented
Status: Needs review » Needs work 
  1. +++ b/tmgmt_oht.plugin.inc
@@ -291,9 +291,53 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+   * @return bool
+   *   Returns TRUE if there are error messages during the process of retrieving
...
+  public function fetchJobs(TMGMTJob $job) {
...
+          $job_item->addMessage('Could not retrieve translation resources about %label.', array('%label' => $job_item->label()), 'error');
...
+      $job->addMessage('Could not pull translation resources about %job.', ['%job' => $job->label()], 'error');

    Once array() once []. 

  2. +++ b/tmgmt_oht.plugin.inc
@@ -304,11 +348,13 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+          if ($show_message) {
...
+              $job_item->addMessage('The translation has been received.');

    It does not show the message. It adds a message to the item.. 

  3. +++ b/tmgmt_oht.plugin.inc
@@ -304,11 +348,13 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+              $job_item->addMessage('The translation has been updated.');

    Also if i remember right, we usually report some more data to the message such as data length.

    But anyway, why not logging the information to the item message if pull button updates it?
    The only thing is, we need to know if there was an update at all to avoid duplicate messages on multiple button press. All in all, the API reads a bit limited... 

  4. +++ b/tmgmt_oht.ui.inc
@@ -111,6 +111,24 @@ class TMGMTOhtTranslatorUIController extends TMGMTDefaultTranslatorUIController
+      $form['actions']['poll'] = array(
...
+        '#value' => t('Pull translations'),
+        '#submit' => array('_tmgmt_oht_poll_submit'),

    Poll or pull? :-) 

  5. +++ b/tmgmt_oht.plugin.inc
@@ -291,9 +291,53 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+          $job_item->addMessage('Could not retrieve project information about %label.', array('%label' => $job_item->label()), 'error');
...
+          $job_item->addMessage('Could not retrieve translation resources about %label.', array('%label' => $job_item->label()), 'error');

    The %label seems to be a duplicate information. Note that you add the message to the job item already! Anything else is of more value.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
add_pull_translation-1899670-7.patch4.08 KB
add_pull_translation-1899670-7-interdiff.txt4.28 KB

Re #6:

1. Changed to array() :)

2/3. These messages are improved in #1918168: When translation is updated the message created should be appropriate. We added $error variable just to know in what case to display messages in drupal_set_message(). It seems it adds additional overhead so I removed the variable, attaching the messages to job items (in any case). In drupal_set_message() we are displaying general messages and for more info there is "Messages" section.

4. Agreed with @Berdir that "Pull translations" is better UI, but changed to "Poll" to be consistent with other translators.

5. Removed unnecessary labels.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

I think we should consistently use pull, not poll. I've always found the usage of poll in mygengo confusing, no need to be consistent with something that's not good :)

+++ b/tmgmt_oht.plugin.inc
@@ -291,6 +291,50 @@ class TMGMTOhtPluginController extends TMGMTDefaultTranslatorPluginController {
+        $project_details = $this->getConnector($job->getTranslator())->getProjectDetails($remote->remote_identifier_1);
+        if (isset($project_details['resources']['translations'])) {
+          $this->retrieveTranslation($project_details['resources']['translations'], $job_item, $project_details['project_id']);
+        }
+        else {
+          $error = TRUE;
+          $job_item->addMessage('Could not retrieve translation resources.', array(), 'error');

Hm. I don't think the fact that there are no translations yet is an error. I simply wouldn't log anything in this case. This is an expected scenario.

If anything, then we should possibly provide a summary at the end, something like "Fetched translations for X job items, Y are not yet translated".

Which I think we could do as a message on the job, which we can then display with the function that I mentioned, instead of the true/false return, we wouldn't need that anymore I think.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Needs work » Needs review
FileSize
add_pull_translation-1899670-9.patch5.35 KB
add_pull_translation-1899670-9-interdiff.txt4.92 KB

Changed to "Pull" and improved a message based on comment #8.

  • Berdir committed 7e0afa4 on 7.x-1.x authored by mbovan 
    Issue #1899670 by mbovan: Add pull translation button
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

That works for me. We'll need to update the code in 8.x-1.x too, which is based on the previous patch.

mbovan’s picture

mbovan CreditAttribution: mbovan at MD Systems GmbH commented
Status: Patch (to be ported) » Needs review
FileSize
add_pull_translation-1899670-12.patch4.57 KB

Ported a patch to D8.

There are some improvements that we can make - e.g. use formatPlural() for the message, as it can be "Fetched translations for 1 job items.".
Should I add it here?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

No. format plural can't handle two numbers anyway and we do our own translation. That would translate it in the current interface language and then every user would see it in that when looking at the log.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Fixed

Committed, thanks.

  • Berdir committed 909302c on 8.x-1.x authored by mbovan 
    Issue #1899670 by mbovan: Add pull translation button

Status: Fixed » Closed (fixed)

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