Problem/Motivation

We should have the ability to choose the status of new translations when they are downloaded into Drupal 8. The status should be Same as source by default (that's what we do now), but we should be able to set up Published or Unpublished for now, and in 8.3.x we may need to expand that if content moderation goes into core.

Proposed resolution

Add a setting on the LingotekSettingsTabPreferencesForm that allows the ability to choose what status translations will be downloaded in.

The logic for doing this is added into the saveTargetData() in the LingotekContentTranslationService. This can only occur if the Publishing Status is a translatable field and so that is checked first and if it is then it continues and checks the new setting. If it is set to "1"=>"Published" then it will download the new translation and set the status to published. If set to "0"=>"Unpublished" then the status will be set to unpublished and if set to "2"=>"Same as source" then it will check the status of the source and set it to that.

Remaining tasks

None

User interface changes

Added a preference on the LingotekSettingsTabPreferencesForm that has a dropdown for the options and a description to accompany the dropdown.

API changes

Added:

LingotekConfigurationServiceInterface::getPreference($preference_id)
LingotekConfigurationServiceInterface::setPreference($preference_id, $value);

Data model changes

None

CommentFileSizeAuthor
#61 2832516-published-setting-61.patch14.21 KBpenyaskito
#61 2832516-published-setting-59-61.interdiff.txt2.1 KBpenyaskito
#59 2832516-published-setting-59.patch14.35 KBpenyaskito
#59 interdiff.txt13 KBpenyaskito
#57 2832516-published-setting-57.patch11.36 KBpenyaskito
#57 interdiff.txt4.46 KBpenyaskito
#55 add_a_setting_to_set-2832516-55.patch10.15 KBt.murphy
#53 add_a_setting_to_set-2832516-53.patch10.14 KBt.murphy
#50 add_a_setting_to_set-2832516-51.patch7.89 KBt.murphy
#49 add_a_setting_to_set-2832516-49.patch7.89 KBt.murphy
#47 add_a_setting_to_set-2832516-47.patch7.82 KBt.murphy
#45 add_a_setting_to_set-2832516-45.patch7.81 KBt.murphy
#43 add_a_setting_to_set-2832516-43.patch7.79 KBt.murphy
#41 add_a_setting_to_set-2832516-41.patch7.86 KBt.murphy
#39 add_a_setting_to_set-2832516-39.patch8.14 KBt.murphy
#37 add_a_setting_to_set-2832516-37.patch8.15 KBt.murphy
#35 add_a_setting_to_set-2832516-35.patch8.14 KBt.murphy
#33 add_a_setting_to_set-2832516-33.patch8.15 KBt.murphy
#31 add_a_setting_to_set-2832516-31.patch8.14 KBt.murphy
#28 add_a_setting_to_set-2832516-28.patch8.33 KBt.murphy
#25 add_a_setting_to_set-2832516-25.patch8.1 KBt.murphy
#22 add_a_setting_to_set-2832516-22.patch8.12 KBt.murphy
#19 add_a_setting_to_set-2832516-19-new.patch8.55 KBt.murphy
#14 add_a_setting_to_set-2832516-14.patch6.11 KBt.murphy
#11 add_a_setting_to_set-2832516-11.patch6.41 KBt.murphy
#8 add_a_setting_to_set-2832516-8.patch6.41 KBt.murphy
#5 add_a_setting_to_set-2832516-5.patch6.28 KBt.murphy
#2 add_a_setting_to_set-2832516-2.patch5.86 KBt.murphy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

t.murphy created an issue. See original summary.

t.murphy’s picture

Here is the patch that contains the suggested changes.

t.murphy’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: add_a_setting_to_set-2832516-2.patch, failed testing.

t.murphy’s picture

Fixed the two errors that occurred during the test of the last patch. The preference needed to be added to the lingotek.settings and it appeared that there was a problem with the description for the new preference.

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: add_a_setting_to_set-2832516-5.patch, failed testing.

t.murphy’s picture

The new preference did not have a default value and so it was causing errors in the test. I added a default value.

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: add_a_setting_to_set-2832516-8.patch, failed testing.

t.murphy’s picture

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 11: add_a_setting_to_set-2832516-11.patch, failed testing.

t.murphy’s picture

A new patch to attempt the fix for the Undefined Index.

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: add_a_setting_to_set-2832516-14.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review

Re-testing, one of the failures is related to #2840990: Fix LingotekDashboardTest::testTranslationsAvailable() for compatibility with 8.3.x. The other one maybe is gone?

penyaskito’s picture

Status: Needs review » Needs work

This is the same failures as #2840990: Fix LingotekDashboardTest::testTranslationsAvailable() for compatibility with 8.3.x, so we are good.

However, we should add new tests for the new behaviours. Also, some improvements we should probably make:

  1. +++ b/config/schema/lingotek.schema.yml
    @@ -32,6 +32,8 @@ lingotek.settings:
    +        node_status:
    

    This should be a setting available for any content that have a status, not only nodes. Could we use a different name that doesn't tie us to nodes? Maybe have a settings per entity type (even bundle?)?

  2. +++ b/src/Form/LingotekSettingsTabPreferencesForm.php
    @@ -125,7 +125,20 @@ class LingotekSettingsTabPreferencesForm extends LingotekConfigFormBase {
    +    2 => t('Same as source node'),
    +    1 => t('Published'),
    +    0 => t('Unpublished')
    

    Let's use moderation states if content_moderation is enabled. For that, we should be using strings and get the available states from the content_moderation module data.

  3. +++ b/src/Form/LingotekSettingsTabPreferencesForm.php
    @@ -264,20 +278,20 @@ class LingotekSettingsTabPreferencesForm extends LingotekConfigFormBase {
    -    
    +
    ...
    -              
    +
    ...
    -              'label' => 'above', // Can be above, inline, hidden, or visually_hidden (These are hard coded in core) 
    +              'label' => 'above', // Can be above, inline, hidden, or visually_hidden (These are hard coded in core)
    
    +++ b/src/LingotekContentTranslationService.php
    @@ -792,7 +792,7 @@ class LingotekContentTranslationService implements LingotekContentTranslationSer
    -      
    +
    

    These changes are unneeded?

t.murphy’s picture

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: add_a_setting_to_set-2832516-19-new.patch, failed testing.

t.murphy’s picture

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: add_a_setting_to_set-2832516-22.patch, failed testing.

t.murphy’s picture

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 25: add_a_setting_to_set-2832516-25.patch, failed testing.

t.murphy’s picture

t.murphy’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 28: add_a_setting_to_set-2832516-28.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

Status: Needs review » Needs work

The last submitted patch, 31: add_a_setting_to_set-2832516-31.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

Status: Needs review » Needs work

The last submitted patch, 33: add_a_setting_to_set-2832516-33.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

Status: Needs review » Needs work

The last submitted patch, 35: add_a_setting_to_set-2832516-35.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

Status: Needs review » Needs work

The last submitted patch, 37: add_a_setting_to_set-2832516-37.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
8.14 KB

Status: Needs review » Needs work

The last submitted patch, 39: add_a_setting_to_set-2832516-39.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
7.86 KB

Status: Needs review » Needs work

The last submitted patch, 41: add_a_setting_to_set-2832516-41.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
7.79 KB

Status: Needs review » Needs work

The last submitted patch, 43: add_a_setting_to_set-2832516-43.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
7.81 KB

Status: Needs review » Needs work

The last submitted patch, 45: add_a_setting_to_set-2832516-45.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
7.82 KB

Status: Needs review » Needs work

The last submitted patch, 47: add_a_setting_to_set-2832516-47.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
7.89 KB
t.murphy’s picture

The last submitted patch, 49: add_a_setting_to_set-2832516-49.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 50: add_a_setting_to_set-2832516-51.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
10.14 KB

Status: Needs review » Needs work

The last submitted patch, 53: add_a_setting_to_set-2832516-53.patch, failed testing.

t.murphy’s picture

Status: Needs work » Needs review
FileSize
10.15 KB

Status: Needs review » Needs work

The last submitted patch, 55: add_a_setting_to_set-2832516-55.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
4.46 KB
11.36 KB

Fixed syntax error.

Status: Needs review » Needs work

The last submitted patch, 57: 2832516-published-setting-57.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
13 KB
14.35 KB
+++ b/src/LingotekContentTranslationService.php
@@ -929,6 +929,13 @@ class LingotekContentTranslationService implements LingotekContentTranslationSer
+        $status = $this->lingotek->get('preference.target_download_status');

The problem is that we are using the lingotek service for accessing the configuration, but we have that mocked in the tests. So the feature was working, the test wasn't.
I'm changing that getting rid of the $this->lingotek->get call, which we should stop using unless actually calling the service, but that it's a cleanup that should happen in other issue.

This patch is green and has more scenarios coverage, but I want to still clean it a bit.

penyaskito’s picture

  1. +++ b/src/Form/LingotekSettingsTabPreferencesForm.php
    @@ -126,6 +126,34 @@ class LingotekSettingsTabPreferencesForm extends LingotekConfigFormBase {
    +    // @todo find out how to get the moderation states from both the content_moderation
    +    // and workbench_moderation modules in a form that is akin to the following:
    +    //  array(
    +    //    machine_name_1 => Name_1,
    +    //    machine_name_2 => Name_2,
    +    //    ... ,
    +    //    machine_name_n => Name_n
    +    //  )
    +    // if (\Drupal::moduleHandler()->moduleExists('content_moderation')){
    +    //   $states = array(); @todo see above comment
    +    // }
    +    // elseif (\Drupal::moduleHandler()->moduleExists('workbench_moderation')){
    +    //   $states = array(); @todo see above comment
    +    // }
    

    Let's remove this commented piece of code.

  2. +++ b/src/Form/LingotekSettingsTabPreferencesForm.php
    @@ -126,6 +126,34 @@ class LingotekSettingsTabPreferencesForm extends LingotekConfigFormBase {
    +    drupal_set_message($this->lingotek->get('preference.target_download_status'));
    

    Oops, I forgot this while debugging. Let's remove it.

  3. +++ b/src/Form/LingotekSettingsTabPreferencesForm.php
    @@ -126,6 +126,34 @@ class LingotekSettingsTabPreferencesForm extends LingotekConfigFormBase {
    +    $form['prefs']['target_download_status'] = array(
    +      '#type' => 'select',
    +      '#description' => t('Translations Download Status: the default status is the status of the source node.'),
    +      '#options' => $states,
    +      '#default_value' => $lingotek_config->getPreference('target_download_status') ?: 'same-as-source',
    +    );
    

    We need a #title for this, looks ugly.

penyaskito’s picture

Attended feedback at #60. Also added a default value for the new setting in our config for new installations.

penyaskito’s picture

Issue summary: View changes

Updated issue summary with API addition.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community

Discussed with @t.murphy and this is RTBC. Tests passed as expected now :-)

  • penyaskito committed 639868d on 8.x-1.x authored by t.murphy
    Issue #2832516 by t.murphy, penyaskito: Add a setting to set the status...
penyaskito’s picture

Status: Reviewed & tested by the community » Fixed

Committed 639868d and pushed to 8.x-1.x. Thanks!

Status: Fixed » Closed (fixed)

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