Problem/Motivation

This is a follow-up to #1831530: Entity translation UI in core (part 2)

User interface changes

Only show source translation column if there are 2 or more source languages (not including "n/a").

Original report from phone call

see #1188388-181: Entity translation UI in core

Problem/Motivation

This is a follow-up to #1831530: Entity translation UI in core (part 2)

Proposed resolution

Only show source translation column if there are 2 or more source languages (not including "n/a").

On the "translate" tab of a node, users currently see a "Source translation" column regardless of whether they have more than one source language for the node. This column should not be shown unless there are more sources than just the original language and the "n/a" label.

Example of one source language shown before:

Example of two source languages shown before:

Example of one source language shown after:

Example of two source languages shown after:

Remaining tasks

  1. done - add screenshot of current #5
  2. done - agree on new UI
  3. done - create patch after ui is agreed on
  4. (novice) test patch, see steps to test in #23. Contributor Task for manual testing: http://drupal.org/node/1489010
  5. (novice) review patch. Contributor Task for review: http://drupal.org/node/1488992
  6. (novice) write automated tests. Contributor Task for tests: http://drupal.org/node/1468170

User interface changes

Only show source translation column if there are 2 or more source languages (not including "n/a").

API changes

no expected API changes

Original report from phone call

see #1188388-181: Entity translation UI in core

CommentFileSizeAuthor
#107 1832870-107.patch5.23 KBrobertdbailey
#107 1832870-107-TESTS-ONLY-SHOULD-FAIL.patch2.86 KBrobertdbailey
#107 interdiff-1832870-100-107.txt1.68 KBrobertdbailey
#104 interdiff-1832870-100-104.txt1.65 KBrobertdbailey
#104 1832870-104-TESTS-ONLY-SHOULD-FAIL.patch2.86 KBrobertdbailey
#104 1832870-104.patch5.19 KBrobertdbailey
#2 ss1-1832870.png161.28 KBmaryedith
#5 drupal-only_display-source_languae_for_multiple_sources-1832870-5.patch1.73 KB-enzo-
#5 translation_entity_with_several_sources.png40.97 KB-enzo-
#5 traslation_entity_same_translation_source.png38.46 KB-enzo-
#11 drupal-only_display-source_languae_for_multiple_sources-1832870-11.patch1.74 KB-enzo-
#15 drupal-only_display-source_languae_for_multiple_sources-1832870-15.patch1.65 KB-enzo-
#17 interdiff-15-17.txt2.29 KBYesCT
#17 drupal-only_display-source_languae_for_multiple_sources-1832870-17.patch2 KBYesCT
#17 onlyshow-s01-na-2012-11-13_1645.png51.77 KBYesCT
#20 interdiff-17-19.txt3.92 KBYesCT
#20 drupal-only_display-source_languae_for_multiple_sources-1832870-19.patch2.88 KBYesCT
#26 interdiff-20-26.txt716 bytesshnark
#26 drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch2.61 KBshnark
#32 drupal-only_display-source_languae_for_multiple_sources_more_than_2.jpg29.93 KBdocker
#32 drupal-only_display-source_languae_for_multiple_sources_less_than_3.jpg19.66 KBdocker
#39 drupal-only_display-source_languae_for_multiple_sources-1832870-39.patch2.63 KBPinolo
#40 1832870-multiple-translation-multiple-source.png39.94 KBsaltednut
#40 1832870-multiple-translation-single-source.png37.37 KBsaltednut
#40 1832870-single-translation.png36.22 KBsaltednut
#46 drupal8.content-translation.module.1832870-46.patch2.64 KBdlu
#47 drupal8.content-translation.module.1832870-47.interdiff.txt1.06 KBdlu
#47 drupal8.content-translation.module.1832870-47.patch2.57 KBdlu
#51 drupal8.content-translation.module.1832870-51.interdiff.txt647 bytesdlu
#51 drupal8.content-translation.module.1832870-51.patch2.56 KBdlu
#53 drupal8.content-translation.module.1832870-53.interdiff.txt675 bytesdlu
#53 drupal8.content-translation.module.1832870-53.patch2.56 KBdlu
#55 Screen Shot 2013-08-16 at 12.31.32 PM.png60.06 KBCMS Dude
#62 drupal8.content-translation.module.1832870.patch2.56 KBseranooo5
#66 1832870-66-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.57 KBivanchaer
#68 1832870-68-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.58 KBrobertdbailey
#71 one_source_lang_before.png21.55 KBrobertdbailey
#71 two_source_langs_before.png23.46 KBrobertdbailey
#71 one_source_lang_after.png20.83 KBrobertdbailey
#71 two_source_langs_after.png23.47 KBrobertdbailey
#73 1832870-73-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.27 KBrobertdbailey
#74 1832870-74-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.3 KBrobertdbailey
#78 1832870-78-TESTS-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch3.76 KBmauzeh
#78 1832870-78-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch6.06 KBmauzeh
#82 1832870-82-TESTS-ONLY-SHOULD-FAIL.patch2.87 KBmauzeh
#82 1832870-82.patch5.18 KBmauzeh
#86 1832870-86-TESTS-ONLY-SHOULD-FAIL.patch2.9 KBmauzeh
#86 1832870-86.patch5.2 KBmauzeh
#94 1832870-94.patch5.21 KBfilijonka
#102 1832870-100.patch5.22 KBrobertdbailey
#102 interdiff-94-100.txt4.49 KBrobertdbailey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maryedith’s picture

Category: feature » task
Issue tags: +Usability, +translatable fields, +D8MI
maryedith’s picture

Issue summary: View changes

updated with comment #181 info

maryedith’s picture

Issue summary: View changes

updated summary with task list

maryedith’s picture

Issue summary: View changes

Updated issue summary. Fixed typo

maryedith’s picture

Issue summary: View changes

Updated issue summary. Fixed typo

maryedith’s picture

FileSize
161.28 KB

ss1-1832870.png

YesCT’s picture

@maryedith Thanks for posting this. A good issue summary is really helpful.

Next steps:
I think it would help to have two screen shots to clarify this.
One screenshot would illustrate when the column would be not shown, along with 1 or two sentences explaining why it's better to hide it.
Another screenshot would be an example of when it would be shown.

-enzo-’s picture

Assigned: Unassigned » -enzo-

Working in this issue

-enzo-’s picture

Hello folks

I did a patch to hide the column if the entity only have some source language

Entity with one unique language source: English

traslation_entity_same_translation_source.png

Entity with two language source : English & Spanish

translation_entity_with_several_sources.png

-enzo-’s picture

Issue summary: View changes

Updated issue summary. Fixed typo

Status: Needs review » Needs work
Issue tags: -Usability, -translatable fields, -D8MI
-enzo-’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +translatable fields, +D8MI
Bojhan’s picture

smart usability++

plach’s picture

Overall the patch looks good, thanks!

However, there are many coding standard issues that need to be fixed before this can go in:

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -21,6 +21,8 @@ function translation_entity_overview(EntityInterface $entity) {
+  //Validate how many source languages are enabled

@@ -134,6 +142,16 @@ function translation_entity_overview(EntityInterface $entity) {
+  //Remove Source Language columns if there are less than two source languages
...
+    //Remove source language header
...
+      //remove source language in each row

Missing space after "//" and trailing dot.

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -124,7 +126,13 @@ function translation_entity_overview(EntityInterface $entity) {
+      if(isset($source_languages[$source_name])) {

@@ -134,6 +142,16 @@ function translation_entity_overview(EntityInterface $entity) {
+  if(count($source_languages) < 3) {

Missing space after "if".

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -124,7 +126,13 @@ function translation_entity_overview(EntityInterface $entity) {
+      } else {

The "else" keyword should be on its own line.

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -124,7 +126,13 @@ function translation_entity_overview(EntityInterface $entity) {
+      }  ¶
+      ¶
+      $rows[] = array($language_name, $row_title, $source_name, $status, $operations); ¶

@@ -134,6 +142,16 @@ function translation_entity_overview(EntityInterface $entity) {
+  }
+  ¶

Trailing whitespaces.

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -134,6 +142,16 @@ function translation_entity_overview(EntityInterface $entity) {
+    foreach($rows as $idx => $row) {

Missing space after "foreach". Also "$idx" is not a very common variable name for a numeric index: I'd use, well, "$index" or something similar :)

plach’s picture

Status: Needs review » Needs work
-enzo-’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Fixed patch to apply code standards

plach’s picture

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -124,7 +126,14 @@ function translation_entity_overview(EntityInterface $entity) {
+      $rows[] = array($language_name, $row_title, $source_name, $status, $operations); ¶

There's still a trailing whitespace left.

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -134,6 +143,16 @@ function translation_entity_overview(EntityInterface $entity) {
+    foreach ($rows as $idx => $row) {
+      // Remove source language in each row.
+      unset($rows[$idx][2]);

No chance to get a more readable name here? :(

-enzo-’s picture

Hello plach

I didn't the fix patch for that line following the idea of webchick to maintain the context http://webchick.net/please-stop-eating-baby-kittens.

For the name , you can give the your idea for a better name?

plach’s picture

I didn't the fix patch for that line following the idea of webchick to maintain the context

Well, that article speaks about focusing on fixing a single issue in a patch, but it refers to existing code. The trailing whitespace is being introduced by the patch so it needs to be fixed.

For the name , you can give the your idea for a better name?

$index :)

-enzo-’s picture

YesCT’s picture

--- a/core/modules/translation_entity/translation_entity.pages.inc
+++ b/core/modules/translation_entity/translation_entity.pages.incundefined

@@ -124,6 +126,13 @@ function translation_entity_overview(EntityInterface $entity) {
+      if (isset($source_languages[$source_name])) {
+        $source_languages[$source_name] += 1;
+      }
+      else {
+        $source_languages[$source_name] = 1;
+      }
+

Maybe it's more standard to use ++.

The variable $source_languages does not really contain the source languages, it's a count of them, which is made clear later.

I think adding a comment for this block would help like:
// Count the number of source languages.
or is it really:
// Record the number of times each language is used as a source for a translation.

and/or
change the variable name to number_of_ ...
would help too.

I looked for an example elsewhere in core, and in
core/modules/file/file.module

there is something like
$existing_files = entity_load_multiple_by_properties('file', array('uri' => $uri));
if (count($existing_files)) {

enzo is in irc. I'll ask there.

+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -134,6 +143,16 @@ function translation_entity_overview(EntityInterface $entity) {
 
+  // Remove Source Language columns if there are less than two source languages.
+  if (count($source_languages) < 3) {

comment says less than 2, but the code says 3.

Also, it's removing just one column so it should be:

Remove Source Language column if there are less than two source languages.

YesCT’s picture

Title: Only show show source translation column if there are > 2 source languages (more than n/a and the original language). » Only show source translation column if there are > 2 source languages (more than n/a and the original language).
FileSize
2.29 KB
2 KB
51.77 KB

background to explain counting the number of a source is used in translations

it's equivalent to the number of rows in which a source language would appear.

Its a little strange because in, for example, a table like the one below,

$count_lauguage_as_source['n/a'] would be 3 (there are 3 rows with n/a as the source)
$count_lauguage_as_source['English'] would be 1
$count_lauguage_as_source['Afrikaans'] would be 1
and
count($count_lauguage_as_source) is 3 (there are 3 values of source language in the table: n/a and two other languages

onlyshow-s01-na-2012-11-13_1645.png

pickiness about variable names

I changed the variable name to $count_language_as_source[] to attempt to accurately describe that it is number of translations/rows in the table for each value of the name of a source language. But I think a better var name could be picked if we redo this again.

We dont really need to know how many rows have English as a source.. we just need to know that English is used as a source. So we could do true false that it's used instead of counting them. We dont really care that there are 5 rows all with n/a. We just care that n/a is the only element in the array, and so there were less than two unique source languages and that means we dont need the source column. [edit: added this paragraph]

It's weird to have '2' in the if for the number of sources deciding when to remove the column, and then later again to unset($header[2]); because those are not the same in meaning, even though they are both '2'. Using a constant for the column might make more sense. The patch I'm uploading uses a variable for now to at least give '2' a meaning.

general strategy

dawehner recommended taking a look at not adding the column in the first place. And I think it would remove some of the awkwardness of later removing the "2th" column. So we should take a look at not adding the column instead of removing it.

tests needed

this also should have tests written.
test case a: hide it! 3 languages enabled, content just exists (table would have all rows with source = n/a
test case b: hide it! 3 languages enabled, content exists and two translation exists both using the same source
test case c: show it! 3 languages enabled, content exists and two translation exists each using a different source

rare weirdness that can be ignored

[we can ignore this because "it's not a common/sane workflow that translators would do": There is a very raw use case where content is created in Spanish, an english translation is added (using spanish as the source), then the content is edited and the language of it is changed to be german. That would result in a table with a german row (the content) and a english row (english translation). the source of the english translation is/was spanish, but the source column would be hidden.]

YesCT’s picture

Issue tags: +Needs tests

needs tests

YesCT’s picture

Assigned: Unassigned » YesCT
YesCT’s picture

corrected my spelling of lauguage to language.

Now it only adds the column if there is more than one unique source language.
This did clean up the need to explain the 2th column or that n/a plus a source meant a comparison against '2'.

relocates the 2 lines setting up the header and rows arrays until after we know if we want to show the source column or not.

It does mean an extra foreach loop through all the languages enabled on a site.

(still needs tests)

YesCT’s picture

Assigned: YesCT » Unassigned

unassigning myself.
Also: Enzo, thanks for starting on this, without your work I could not have gotten this far. Maybe since you are familiar with the issue, you could also give my approach a review? :)

YesCT’s picture

Status: Needs review » Needs work
+++ b/core/modules/translation_entity/translation_entity.pages.incundefined
@@ -21,14 +21,13 @@ function translation_entity_overview(EntityInterface $entity) {
+  $count_language_as_source = array();

@@ -47,6 +46,40 @@ function translation_entity_overview(EntityInterface $entity) {
+    if (isset($language_as_source) && count($language_as_source) > 1) {

ah, $count_language_as_source is not used...

I should have changed that to $language_as_source and if I had, then I probably would not have had to do the isset($language_as_source)

shnark’s picture

here are some instructions for testing the patch

clean drupal 8.
download patch
made new branch
applied patch
installed drupal 8
under extend enable entity translation
under strutre edit article content type
under language settings enable translation
edit aricle content type
go to manage fields
edit body
enabled translation
under configuation add a language
add article pice of content
fill out words for title and body
click on tranlations tab
notice there is no source column
add translation
notice there is no source column
add another language
add another translation, picking a different source
notice there is a source column

klonos’s picture

...coming a bit late in this, but what do you think about adding a "(source language)" or plain "(source)" text appended as some kind of indication when the column is hidden? I know that we already mark the language with bold text, but it kinda feels that it's not that obvious to people not familiar with this.

YesCT’s picture

@klonos yeah, there is an issue for that: #1833126: Having the original language in bold text in the node "TRANSLATIONS" tab is not semantic. make representation accessible. which also links to an issue to figure out what to call it "source" etc...

shnark’s picture

Status: Needs work » Needs review
FileSize
716 bytes
2.61 KB

I made the change recommended in comment #22
since the if condition has a isset, we didn't need the line at the beginning.
besides, it had the wrong variable name at the beginning.

klonos’s picture

@YesCT: Thanx for the pointer Cathy ;) Heading there...

YesCT’s picture

Status: Needs review » Needs work
shnark’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Needs tests, +translatable fields, +D8MI
shnark’s picture

Issue summary: View changes

Updated issue summary to add link to screenshots

YesCT’s picture

Issue tags: +Novice

Next steps here include writing tests. For a hint for someone new to tests... would web tests be needed here (instead of unit tests)?

docker’s picture

Tested as described #23
The source translation column only shown when there is more than n/a and the original language.

YesCT’s picture

YesCT’s picture

Status: Needs review » Needs work

still applies and passes testbot.

needs work to write tests. (Contributor Task for tests: http://drupal.org/node/1468170 )

nagwani’s picture

Subscribing to write tests

rli’s picture

Status: Needs work » Needs review
Pinolo’s picture

Assigned: Unassigned » Pinolo
Pinolo’s picture

Issue tags: +SprintWeekend2013

-

Pinolo’s picture

Patch updated (no longer worked with latest core 8.x: column was always hidden since the source property was moved as element of translation[$langcode]). Still needs tests.

saltednut’s picture

After applying the patch, during a quick manual test, I am only seeing the source translation column if there are more than 2 source languages.

Single translation (no source column)
1832870-single-translation.png

Multiple translations with single source (no source column)
1832870-multiple-translation-single-source.png

Multiple translations with multiple sources (source column)
1832870-multiple-translation-multiple-source.png

Pinolo’s picture

Brantwynn: it's actually visible when 2 or more source languages are present. I understand this was the desired behavior. Can anyone confirm?

plach’s picture

Yes, this is the desired behavior: not cluttering the translation overview with the source column unless there actually is meaningful data to display.

Code looks good to me, aside from:

+++ b/core/modules/translation_entity/translation_entity.pages.inc
@@ -47,6 +44,39 @@ function translation_entity_overview(EntityInterface $entity) {
+    // Only show the translations column if there is more than one source value.
+    if (isset($language_as_source) && count($language_as_source) > 1) {
+      $show_source_column = TRUE;
+    }
+    else {
+      $show_source_column = FALSE;

Can we have just $show_shource_column = count($language_as_source) > 1; and initialize $show_shource_column to an empty array before the loop above?

We should also add some test coverage for this, it shouldn't be a big deal.

saltednut’s picture

@Pinolo - sorry for not explaining further but basically I was saying in #40 that your patch worked - I wasn't ready to RTBC it though because my time at the sprint was ending and I didn't have time to do code review.

So, by "multiple" I meant "2 or more" - sorry if that was lost in translation. :)

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
star-szr’s picture

Assigned: Pinolo » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs reroll

CNW for tests. Needs a reroll first, the file doesn't exist anymore.

dlu’s picture

This is a reroll of #40 without the change suggested in #42.

dlu’s picture

Made change suggested in #42 assuming that the array to be initialized was really $language_as_source.

dlu’s picture

Made change suggested in #42 assuming that the array to be initialized was really $language_as_source.

dlu’s picture

Status: Needs work » Needs review

Hoping that this will make the test bot interested…

Status: Needs review » Needs work

The last submitted patch, drupal8.content-translation.module.1832870-47.patch, failed testing.

dlu’s picture

Testing failed due to a regression from $language->id back to $language->langcode introduced by the reroll.

Status: Needs review » Needs work

The last submitted patch, drupal8.content-translation.module.1832870-51.patch, failed testing.

dlu’s picture

dlu’s picture

Status: Needs work » Needs review

It's late.

CMS Dude’s picture

Testing: Walked through steps as outlined in #23

- Applied patch in #53
- Fresh drupal 8 install
- Created content
- Added a few new languages
- No source language column showing (as per expected)
- Created a few nodes and altered so we had more than one source language.
- "Source Language" Column showed up as expected. Woo-hoo!

Screen Shot 2013-08-16 at 12.31.32 PM.png

CMS Dude’s picture

Title: Only show source translation column if there are > 2 source languages (more than n/a and the original language). » Only show source translation column if there are 2 or more source languages (more than n/a and the original language).
Assigned: Unassigned » CMS Dude
Status: Needs review » Needs work

Going to write my first test.

CMS Dude’s picture

Issue summary: View changes

Updated issue summary remaining tasks.

willieseabrook’s picture

The last submitted patch, 53: drupal8.content-translation.module.1832870-53.patch, failed testing.

willieseabrook’s picture

Assigned: CMS Dude » Unassigned
Issue summary: View changes
Issue tags: -SprintWeekend2013 +SprintWeekend2014, +TUNIS_2014_JANUARY

Starting as part of DrupalSprintWeekend

willieseabrook’s picture

Assigned: Unassigned » willieseabrook
seranooo5’s picture

Assigned: willieseabrook » seranooo5
seranooo5’s picture

Assigned: seranooo5 » Unassigned
Status: Needs work » Needs review
FileSize
2.56 KB

Status: Needs review » Needs work

The last submitted patch, 62: drupal8.content-translation.module.1832870.patch, failed testing.

The last submitted patch, 62: drupal8.content-translation.module.1832870.patch, failed testing.

ivanchaer’s picture

Assigned: Unassigned » ivanchaer
ivanchaer’s picture

Assigned: ivanchaer » Unassigned
Status: Needs work » Needs review
FileSize
2.57 KB

It seems that @seranooo5 forgot to keep one line of code from the original in his patch, hence the syntax error.

I've gone through the test patch steps (#23), and it seems fine now.

Status: Needs review » Needs work
robertdbailey’s picture

Rerolled patch #66 from January.
fixed $header and $row variables to exist even when multilingual is not enabled.
Removed unused $links variable.

Please review

robertdbailey’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Thanks for working on this!

I think the code looks good.

The issue summary needs an update with before/after screenshots. Also the summary still says if there are 2 sources, they should still not be displayed, but that is not what the code does. It displays sources even if there are only 2 of them. So maybe not implementing the exact right thing yet?

Also still needs tests :)

Apart of that, only cosmetic remarks:

+++ b/core/modules/content_translation/content_translation.pages.inc
@@ -44,6 +43,31 @@ function content_translation_overview(EntityInterface $entity) {
+    // Determine which languages have been used as a source for other
+    // translations.
+    $language_as_source = array();
+    foreach ($languages as $language) {
+      $langcode = $language->id;
+
+      $is_original = $langcode == $original;
+      // Original has no source.
+      if (!$is_original && isset($translations[$langcode])) {
+        // Non-original existing translation in the translation set: keep track
+        // of the source language.
+        $source = isset($entity->translation[$langcode]['source']) ? $entity->translation[$langcode]['source'] : '';
+        // Note the source language has been used as a source.
+        $language_as_source[$source] = TRUE;
+      }
+    }
+
+    // Only show the translations column if there is more than one source value.
+    $show_source_column = count($language_as_source) > 1;
+
+    // Set up the translations table.
+    if ($show_source_column) {
+      $header = array(t('Language'), t('Translation'), t('Source language'), t('Status'), t('Operations'));
+    }
+

I think this could likely do with some comments removed :) Eg. the table header re-setup comment is misleading, it does not set up the table but the header, although its easy to tell from the code itself. Its not that complicated to need a comment :)

Also inside the foreach I think one line of comment would be enough, something like:

// Collect source languages for translations (non-originals).

Or something along those lines.

robertdbailey’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
21.55 KB
23.46 KB
20.83 KB
23.47 KB

Issue summary updated.

robertdbailey’s picture

Status: Needs review » Needs work
robertdbailey’s picture

Modified the patch to consolidate a little code and some comments. Let me know if this looks good, and I'll take a stab at the tests.

robertdbailey’s picture

oops, overzealous line consolidation caused some errors, patch resubmitted.

mauzeh’s picture

Assigned: Unassigned » mauzeh

My first novice issue, I'm at DDD@Szeget 2014, will write tests for this issue.

mauzeh’s picture

I will write the tests on the UN-patched version that will:

1) Fail if content is not yet translated (in which case all "source language" values are equal to "N/A"). Column should be hidden in this case.

2) Fail if content is written in different languages from scratch (in which case all "source language" values are also equal to "N/A"). Column should be hidden in this case.

3) Fail if at most one content item is translated from a version in a different language (in which case all but one "source language" is equal to "N/A"). Column should be hidden in this case.

4) Succeed if more than one content item is translation from a version in a different language (in which case more than one "source language" is equal to "N/A"). Column should be shown in this case.

mauzeh’s picture

Status: Needs review » Needs work

mauzeh’s picture

Seems like the tests failed because other modules (like Custom Block) inherit from my newly added test. I just ran the test locally (having applied the patch and test) and been able to reproduce the problem by running the test of the "Custom Block" module only.

mauzeh’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
5.18 KB

Fixed the test. The "TESTS-ONLY" file should fail, and the other patch should pass.

The last submitted patch, 82: 1832870-82-TESTS-ONLY-SHOULD-FAIL.patch, failed testing.

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Hey, nice. This is a sweet little UX improvement to make the screen look less cluttered for the 80%+ use case.

Unfortunately, though, it no longer applies, so needs a quick re-roll.

mauzeh’s picture

Ha! good to exercise my reroll muscles... There was one conflict introduced in #2111887: Regression: Only title (of base fields) on nodes is marked as translatable. The conflict was one line in the git context lines, I adopted the HEAD version.

The last submitted patch, 86: 1832870-86-TESTS-ONLY-SHOULD-FAIL.patch, failed testing.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

Talked to @mauzeh Looked at a diff of the two patches. It is good. Was a simple reroll, and patch with fix and test is green. We should be good to go here again.

mauzeh’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 86: 1832870-86.patch, failed testing.

robertdbailey’s picture

Status: Needs work » Needs review

86: 1832870-86.patch queued for re-testing. Previous result:

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.

filijonka’s picture

Status: Needs review » Reviewed & tested by the community

Can't see that anything happened since #88 and patch passed the test again so setting this to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 86: 1832870-86.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review
FileSize
5.21 KB

Hi

Ok didn't have anyone to ask about this but since the patch obviously wasn't appliable I made a reroll based on #86. There were no conflicts so it should be straight forward I hope.

Status: Needs review » Needs work

The last submitted patch, 94: 1832870-94.patch, failed testing.

filijonka’s picture

Status: Needs work » Needs review

94: 1832870-94.patch queued for re-testing.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community

I agree, only a change in context line
< foreach (field_info_instances($entity->getEntityTypeId(), $entity->bundle()) as $instance) {
---
> foreach (\Drupal::entityManager()->getFieldDefinitions($entity->getEntityTypeId(), $entity->bundle()) as $field_definition) {

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -30,11 +30,10 @@ function content_translation_overview(EntityInterface $entity) {
    -  $header = array(t('Language'), t('Translation'), t('Source language'), t('Status'), t('Operations'));
    +  $header = array(t('Language'), t('Translation'), t('Status'), t('Operations'));
    
    @@ -44,6 +43,24 @@ function content_translation_overview(EntityInterface $entity) {
    +    if ($show_source_column) {
    +      $header = array(t('Language'), t('Translation'), t('Source language'), t('Status'), t('Operations'));
    +    }
    

    I think that code should be moved closer to each other, so that it's easier to see that they are mutually exclusive. I.e. an if/else or something like that.

  2. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -44,6 +43,24 @@ function content_translation_overview(EntityInterface $entity) {
    +    $language_as_source = array();
    ...
    +        $source = isset($entity->translation[$langcode]['source']) ? $entity->translation[$langcode]['source'] : '';
    +        $language_as_source[$source] = TRUE;
    ...
    +    $show_source_column = count($language_as_source) > 1;
    

    I think this part is very hard to read.

    A: I think the isset() check should be moved into the if (). The empty string thing feelds weird.

    I think also there's something that doesn't work as expected as it seems i.e. if there's one translation that has 'English' as source and then one that doesn't have a source at all, that would still result in the source_column being shown as count(array('' => ..., 'en' => ...)) == 2. ?!

    B: I think instead of the current variable/array structure I would propose the following:

    $source_langcodes = array();
    ...
      $source_langcodes[] = $langcode;
    ...
    $show_source_column = count(array_unique($source_langcodes)) > 1;
    

    That has a performance offset incurred by the array_unique() but that's negligible.

  3. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -44,6 +43,24 @@ function content_translation_overview(EntityInterface $entity) {
    +    foreach ($languages as $language) {
    ...
         foreach ($languages as $language) {
    

    Is there someway we can avoid looping the list of languages twice? I don't think the performance loss is problematic but still would be nice. It might be that that is not possible or at least not easily possible.

  4. +++ b/core/modules/content_translation/lib/Drupal/content_translation/Tests/ContentTranslationUITest.php
    @@ -54,6 +54,8 @@ function testTranslationUI() {
    +    $this->assertNoText('Source language', format_string('Source language column correctly hidden.'));
    
    @@ -74,20 +76,27 @@ function testTranslationUI() {
    +    $this->assertNoText('Source language', format_string('Source language column correctly hidden.'));
    ...
    +    $this->assertText('Source language', format_string('Source language column correctly shown.'));
    

    There's no reason for format_string() here as there's nothing to format. In case you do want to stick to this please use String::format()

Moving back to needs review, to see if people agree with the points above.

vijaycs85’s picture

Issue tags: +sprint

Adding sprint tag.

YesCT’s picture

Assigned: mauzeh » Unassigned
Status: Needs review » Needs work

mauzeh, unassigning, just comment if you begin any work. :)

certainly at least 4. is actionable.

lets give a day or so to try the other things, and if they are not easy, let's re-rtbc and have them be follow-ups to investigate doing them.

Gábor Hojtsy’s picture

Issue tags: +language-content

Add D8MI topic tag.

robertdbailey’s picture

Status: Needs work » Needs review
Issue tags: -language-content
FileSize
4.49 KB
5.22 KB

This patch is an attempt at @tstoeckler's suggested changes.

Hopefully the hard-to-read passage is a little easier now. One observation: the size of the array of source languages should be at least one, not including the original language. This distinction seems important because a source language could be the only one and also not the original language at the same time. This is how it would happen:

a. create the original in English.
b. translate from English to French.
c. translate from French to German.
d. delete the French.

In this case, I think it is still important to show the source-language column, which would contain "n/a", "n/a", and "French", respectively. To account for this scenario, I just kept the collection of source languages as an associative array instead of the array to be unique'd per @tstoeckler's suggestion, so the original language may be easily removed if it exists (in the filter function). If this is the correct logic, then the issue summary should be updated to reflect this change.

tstoeckler’s picture

Status: Needs review » Needs work

Yay, thanks a lot @robertdbailey!! It might be because I like anonymous functions a lot, but I find the code a *lot* more readable now. Awesome!

I have a few minor suggestions and then I think we're done here:

  1. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -44,6 +42,12 @@ function content_translation_overview(EntityInterface $entity) {
    +    $additional_source_langcodes = array_filter($entity->translation, function ($a) use ($original) {
    

    We should avoid arbitrary variable names such as $a. It seems $translation might be appropriate here?

  2. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -44,6 +42,12 @@ function content_translation_overview(EntityInterface $entity) {
    +      return isset($a['source']) && $a['source'] && $a['source'] != $original;
    

    I think the first two conditions can be collapsed with !empty($a['source'])

  3. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -44,6 +42,12 @@ function content_translation_overview(EntityInterface $entity) {
    +    $show_source_column = count($additional_source_langcodes) > 0;
    

    It seems this is also equivalent to a !empty(). In that case maybe collapse the two statements into one?

  4. +++ b/core/modules/content_translation/content_translation.pages.inc
    @@ -135,6 +144,13 @@ function content_translation_overview(EntityInterface $entity) {
    +  if (isset($show_source_column) && $show_source_column == TRUE) {
    

    If we were to do a $show_source_column = FALSE before the big if {} then we can remove the isset().

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
5.19 KB
2.86 KB
1.65 KB

great catches @tstoeckler, changes made! I also bumped into a case where $entity->translation was not set because the content type was not enabled for translation, so I added one additional "!empty($entity->translation)" to prevent a warning in this case. Everything else appeared to work. Please review.

The last submitted patch, 104: 1832870-104.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 104: 1832870-104-TESTS-ONLY-SHOULD-FAIL.patch, failed testing.

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
1.68 KB
2.86 KB
5.23 KB

I think (PHP 5.3.x?) did not like the last edit, consolidating the two lines into one. Perhaps there is a better way to do that; but I kept them separate for now. Please review.

The last submitted patch, 107: 1832870-107-TESTS-ONLY-SHOULD-FAIL.patch, failed testing.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yay!!!

Thanks a lot @robertdbailey for enduring my nitpicking. :-) I really do find the code a lot more natural to read, though, now. I have nothing more to complain about. The tests-only patch beautifully fails on the correct assertions.

RTBC.

Gábor Hojtsy’s picture

Issue tags: +language-content

Add D8MI topic tag.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops. thought I had committed this a long time ago! Rectifying that now. :)

Committed and pushed to 8.x. Thanks!

  • Commit 0828fa9 on 8.x by webchick:
    Issue #1832870 by -enzo-, YesCT, EllaTheHarpy, Pinolo, dlu, seranooo5,...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks all! Finally :)

Status: Fixed » Closed (fixed)

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