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

Files: 
CommentFileSizeAuthor
#94 1832870-94.patch5.21 KBfilijonka
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,195 pass(es).
[ View ]
#86 1832870-86.patch5.2 KBmauzeh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1832870-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 1832870-86-TESTS-ONLY-SHOULD-FAIL.patch2.9 KBmauzeh
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,892 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#71 one_source_lang_after.png20.83 KBrobertdbailey
#71 two_source_langs_before.png23.46 KBrobertdbailey
#71 one_source_lang_before.png21.55 KBrobertdbailey
#68 1832870-68-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.58 KBrobertdbailey
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,342 pass(es).
[ View ]
#66 1832870-66-only-show-source-translation-column-if-there-are-2-or-more-source-languages.patch2.57 KBivanchaer
FAILED: [[SimpleTest]]: [MySQL] 63,151 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#62 drupal8.content-translation.module.1832870.patch2.56 KBseranooo5
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/content_translation/content_translation.pages.inc.
[ View ]
#55 Screen Shot 2013-08-16 at 12.31.32 PM.png60.06 KBCMS Dude
#53 drupal8.content-translation.module.1832870-53.patch2.56 KBdlu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.content-translation.module.1832870-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 drupal8.content-translation.module.1832870-53.interdiff.txt675 bytesdlu
#51 drupal8.content-translation.module.1832870-51.patch2.56 KBdlu
FAILED: [[SimpleTest]]: [MySQL] 38,142 pass(es), 648 fail(s), and 347 exception(s).
[ View ]
#51 drupal8.content-translation.module.1832870-51.interdiff.txt647 bytesdlu
#47 drupal8.content-translation.module.1832870-47.patch2.57 KBdlu
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 0 fail(s), and 112 exception(s).
[ View ]
#47 drupal8.content-translation.module.1832870-47.interdiff.txt1.06 KBdlu
#46 drupal8.content-translation.module.1832870-46.patch2.64 KBdlu
FAILED: [[SimpleTest]]: [MySQL] 57,992 pass(es), 0 fail(s), and 48 exception(s).
[ View ]
#40 1832870-single-translation.png36.22 KBbrantwynn
#40 1832870-multiple-translation-single-source.png37.37 KBbrantwynn
#40 1832870-multiple-translation-multiple-source.png39.94 KBbrantwynn
#39 drupal-only_display-source_languae_for_multiple_sources-1832870-39.patch2.63 KBPinolo
PASSED: [[SimpleTest]]: [MySQL] 52,819 pass(es).
[ View ]
#32 drupal-only_display-source_languae_for_multiple_sources_less_than_3.jpg19.66 KBdocker
#32 drupal-only_display-source_languae_for_multiple_sources_more_than_2.jpg29.93 KBdocker
#26 drupal-only_display-source_languae_for_multiple_sources-1832870-26.patch2.61 KBEllaTheHarpy
PASSED: [[SimpleTest]]: [MySQL] 52,454 pass(es).
[ View ]
#26 interdiff-20-26.txt716 bytesEllaTheHarpy
#20 drupal-only_display-source_languae_for_multiple_sources-1832870-19.patch2.88 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 48,066 pass(es).
[ View ]
#20 interdiff-17-19.txt3.92 KBYesCT
#17 onlyshow-s01-na-2012-11-13_1645.png51.77 KBYesCT
#17 drupal-only_display-source_languae_for_multiple_sources-1832870-17.patch2 KBYesCT
PASSED: [[SimpleTest]]: [MySQL] 48,062 pass(es).
[ View ]
#17 interdiff-15-17.txt2.29 KBYesCT
#15 drupal-only_display-source_languae_for_multiple_sources-1832870-15.patch1.65 KB-enzo-
PASSED: [[SimpleTest]]: [MySQL] 48,057 pass(es).
[ View ]
#11 drupal-only_display-source_languae_for_multiple_sources-1832870-11.patch1.74 KB-enzo-
PASSED: [[SimpleTest]]: [MySQL] 48,059 pass(es).
[ View ]
#5 traslation_entity_same_translation_source.png38.46 KB-enzo-
#5 translation_entity_with_several_sources.png40.97 KB-enzo-
#5 drupal-only_display-source_languae_for_multiple_sources-1832870-5.patch1.73 KB-enzo-
PASSED: [[SimpleTest]]: [MySQL] 48,063 pass(es).
[ View ]
#2 ss1-1832870.png161.28 KBmaryedith

Comments

Category:feature» task
Issue tags:+Usability, +translatable fields, +D8MI

Issue summary:View changes

updated with comment #181 info

Issue summary:View changes

updated summary with task list

Issue summary:View changes

Updated issue summary. Fixed typo

Issue summary:View changes

Updated issue summary. Fixed typo

StatusFileSize
new161.28 KB

ss1-1832870.png

@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.

Assigned:Unassigned» -enzo-

Working in this issue

Assigned:-enzo-» Unassigned
Status:Active» Needs review
StatusFileSize
new1.73 KB
PASSED: [[SimpleTest]]: [MySQL] 48,063 pass(es).
[ View ]
new40.97 KB
new38.46 KB

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

Issue summary:View changes

Updated issue summary. Fixed typo

Status:Needs review» Needs work
Issue tags:-Usability, -translatable fields, -D8MI

Status:Needs work» Needs review
Issue tags:+Usability, +translatable fields, +D8MI

smart usability++

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 :)

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new1.74 KB
PASSED: [[SimpleTest]]: [MySQL] 48,059 pass(es).
[ View ]

Fixed patch to apply code standards

+++ 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? :(

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?

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 :)

StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 48,057 pass(es).
[ View ]

Changed applied

--- 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.

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).
StatusFileSize
new2.29 KB
new2 KB
PASSED: [[SimpleTest]]: [MySQL] 48,062 pass(es).
[ View ]
new51.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.]

Issue tags:+Needs tests

needs tests

Assigned:Unassigned» YesCT

StatusFileSize
new3.92 KB
new2.88 KB
PASSED: [[SimpleTest]]: [MySQL] 48,066 pass(es).
[ View ]

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)

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? :)

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)

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

...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.

@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...

Status:Needs work» Needs review
StatusFileSize
new716 bytes
new2.61 KB
PASSED: [[SimpleTest]]: [MySQL] 52,454 pass(es).
[ View ]

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.

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

Status:Needs review» Needs work

Status:Needs work» Needs review
Issue tags:+Usability, +Needs tests, +translatable fields, +D8MI

Issue summary:View changes

Updated issue summary to add link to screenshots

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)?

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

Status:Needs review» Needs work

still applies and passes testbot.

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

Subscribing to write tests

Status:Needs work» Needs review

Assigned:Unassigned» Pinolo

Issue tags:+#SprintWeekend

-

StatusFileSize
new2.63 KB
PASSED: [[SimpleTest]]: [MySQL] 52,819 pass(es).
[ View ]

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.

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

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

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.

@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. :)

Component:translation_entity.module» content_translation.module

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.

Issue tags:-Needs reroll
StatusFileSize
new2.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,992 pass(es), 0 fail(s), and 48 exception(s).
[ View ]

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

StatusFileSize
new1.06 KB
new2.57 KB
FAILED: [[SimpleTest]]: [MySQL] 58,026 pass(es), 0 fail(s), and 112 exception(s).
[ View ]

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

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

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.

Status:Needs work» Needs review
StatusFileSize
new647 bytes
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] 38,142 pass(es), 648 fail(s), and 347 exception(s).
[ View ]

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.

StatusFileSize
new675 bytes
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.content-translation.module.1832870-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Try it again without the typo…

Status:Needs work» Needs review

It's late.

StatusFileSize
new60.06 KB

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

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.

Issue summary:View changes

Updated issue summary remaining tasks.

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

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

Starting as part of DrupalSprintWeekend

Assigned:Unassigned» willieseabrook

Assigned:seranooo5» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/content_translation/content_translation.pages.inc.
[ View ]

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.

Assigned:Unassigned» ivanchaer

Assigned:ivanchaer» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.57 KB
FAILED: [[SimpleTest]]: [MySQL] 63,151 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

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

StatusFileSize
new2.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,342 pass(es).
[ View ]

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

Please review

Status:Needs work» Needs review

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.

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new21.55 KB
new23.46 KB
new20.83 KB
new23.47 KB

Issue summary updated.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new2.27 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,401 pass(es), 0 fail(s), and 43 exception(s).
[ View ]

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.

StatusFileSize
new2.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,332 pass(es).
[ View ]

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

Assigned:Unassigned» mauzeh

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

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.

StatusFileSize
new3.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,718 pass(es), 8 fail(s), and 2 exception(s).
[ View ]
new6.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,644 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Two patches:

1) Tests only. Should fail.
2) Patch and tests. Should pass.

Status:Needs review» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new2.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,767 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
new5.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,790 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Looks good, thanks!

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.

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,892 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
new5.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1832870-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Reviewed & tested by the community» Needs work

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new5.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,195 pass(es).
[ View ]

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.

Status:Needs work» Needs review

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

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) {