Bugs:

1. Original value display does not contain any newlines when needed. This may invalidate using the description field for the original value. We may need someone with more complex forms usability expertise to help devise a better UI here.

2. Original value display uses the same language as the translation, so it is in fact the value of the translation before any changes on the page. That is not as intended, it should be the original config value. This is due to the config access using the same context as the form itself.

Screenshot_3_17_13_3_05_PM.png

Files: 
CommentFileSizeAuthor
#19 1945184-original-value-display-19.patch3.13 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#19 1945184-original-value-display-19-test-only.patch2.22 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#19 1945184-diff-17-19.txt708 bytesvijaycs85
#17 1945184-original-value-display-17.patch3.13 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#17 1945184-original-value-display-17-test-only.patch2.21 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#16 1945184-original-value-display-16-test-only.patch2.22 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#16 1945184-original-value-display-16.patch3.14 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#14 1945184-original-value-display-14-test-only.patch2.23 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 1945184-original-value-display-14.patch3.14 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 1945184-original-value-display-11-test-only.patch745 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#11 1945184-original-value-display-11.patch1.64 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
#10 00001078.png189.2 KBGábor Hojtsy
#5 1945184-original-value-display-5.patch2.46 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#5 1945184-diff-3-5.txt1.06 KBvijaycs85
#3 1945184-original-value-display-3.patch2.49 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 1945184-diff-1-3.txt2.28 KBvijaycs85
#1 1945184-original-value-display-1.patch1.47 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 original-value-br.png15.24 KBvijaycs85
#1 original-value-source-language.png15.73 KBvijaycs85
Screenshot_3_17_13_3_05_PM.png137.51 KBGábor Hojtsy

Comments

vijaycs85’s picture

Status:Active» Needs review
StatusFileSize
new15.73 KB
new15.24 KB
new1.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch to fix both issue #1 and #2 in summary.
original-value-br.pngoriginal-value-source-language.png

Gábor Hojtsy’s picture

Status:Needs review» Needs work
+++ b/config_translation.admin.incundefined
@@ -177,7 +177,6 @@ function config_translation_item_translate_page($action, $base_path, $title, $na
-  config_translation_enter_context($language);
   return drupal_get_form('config_translation_form', $base_path, $title, $names, $language);
}

@@ -220,6 +219,7 @@ function config_translation_form($form, &$form_state, $base_path, $title, $names

@@ -220,6 +219,7 @@ function config_translation_form($form, &$form_state, $base_path, $title, $names
       '#tree' => TRUE,
     );
     $base_config = config($name)->get();
+    config_translation_enter_context($language);
     $form[$id] += config_translation_build_form(config_typed()->get($name), config($name)->get(), $base_config);

How does this solve the problem? The form has values and source intertwined, so I'd assumed entering / leaving contexts constantly would be what is needed here, or keeping a previously retrieved $config around, if that would work. Just putting the context entering later sounds like would not actually solve this? How does it do it?

+++ b/config_translation.admin.incundefined
@@ -299,12 +299,12 @@ function config_translation_build_form($schema, $config, $base_config, $collapse
-        '#description' => $description,
+        '#description' => $description

This trailing comma should be no issue, it should even be there according to the coding standards.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new2.28 KB
new2.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks for the review @Gábor Hojtsy. I have updated to get the base_config before form builder and added comma for $description.

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Starting to look good :)

+++ b/config_translation.admin.incundefined
@@ -176,9 +176,13 @@ function config_translation_item_translate_page($action, $base_path, $title, $na
+  // Get base language configuration, before form building.
+  $base_config = array();
+  foreach ($names as $name) {
+    $base_config[$name] = config($name)->get();

It would be great to add a comment here on why is this being done. :)

+++ b/config_translation.admin.incundefined
@@ -196,11 +200,13 @@ function config_translation_item_translate_page($action, $base_path, $title, $na
+ * @param array $base_config
+ *   An array of base language configuration data.

"keyed by configuration names" would be a good addition to the docs.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.06 KB
new2.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1945184-original-value-display-5.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updating doc changes in #4.

Gábor Hojtsy’s picture

Status:Needs review» Reviewed & tested by the community

Superb, thanks, let's get this committed :)

vijaycs85’s picture

Status:Reviewed & tested by the community» Fixed

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

Gábor Hojtsy’s picture

Superb, thanks!

Status:Fixed» Closed (fixed)

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

Gábor Hojtsy’s picture

Status:Closed (fixed)» Needs work
Issue tags:+Needs tests
StatusFileSize
new189.2 KB

As per YesCT, this is not yet fixed. We'll also need tests for this.

00001078.png

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new1.64 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
new745 bytes
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]

Updating fix with test.

Gábor Hojtsy’s picture

Good catch on the issue where the config page is visited in another language :)

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Test only should fail :D

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.23 KB
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Sure @Gábor Hojtsy. Here it will fail :)

Status:Needs review» Needs work

The last submitted patch, 1945184-original-value-display-14.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new3.14 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
new2.22 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]

updating URL...

vijaycs85’s picture

StatusFileSize
new2.21 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]
new3.13 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]

Minor URL fix...

Gábor Hojtsy’s picture

Status:Needs review» Needs work

Test-only patch should still fail :D I think I found the reason it does not.

+++ b/lib/Drupal/config_translation/Tests/ConfigTranslationUITest.phpundefined
@@ -99,13 +101,18 @@ class ConfigTranslationUITest extends WebTestBase {
+
+    // Visit French site to ensure base language string present as source.
+    $this->drupalGet($translation_base_url . '/edit/fr');
+    $this->assertText($site_name);
+    $this->assertText($site_slogan);

This just visits the French translation editing page, but the page is not in French. That would need a path prefix of 'fr'. $translation_base_url starts with 'admin/...'.

So drupalget('fr/' . $translation_base_url . '/edit/fr') I think :)

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new708 bytes
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] 245 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new3.13 KB
PASSED: [[SimpleTest]]: [MySQL] 247 pass(es).
[ View ]

yeah, my bad...

Status:Needs review» Needs work

The last submitted patch, 1945184-original-value-display-19-test-only.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests

This is ready for review now.

Gábor Hojtsy’s picture

Status:Needs review» Fixed

Thanks! Committed the fix with updated code comments. It is not the base context but the override free context + other updates to the comments around contexts to make this easier to understand. Woot!

http://drupalcode.org/project/config_translation.git/commitdiff/8a09f89f...

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