Unchanged strings on page become customized when editing others

Patch looks good, but don't we need a test?

Yes, definitely need tests.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,44 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    } else if (!$existing_translation && !empty($new_translation_string)) {

As per coding standard this would need to be "elseif" and on a separate line.

First initial version of patch for Simple Test

Woot, thanks!

Adding Issue summary template

The proposed patch looks good, I only found one condition I think should be tested and some minor code fixes:

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+   * Test that strings are only saved and marked as customized when they are
+   * actually changed.

Should be shortened to one line if possible :) Eg.

"Tests that only changed strings are saved customized when edited."

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Queries strings with showing only non-customized strings.

"Query strings..." - no need to talk 3rd person here.

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Submitting the translations withouth changing the translation, so the

without

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleTranslationTest.phpundefined
@@ -467,4 +468,65 @@ function testStringSearch() {
+    // Submitting the translations with a new the translation, so the
+    // translation will be saved as customized and no result will be returned.
+    $textarea = current($this->xpath('//textarea'));
+    $lid = (string) $textarea[0]['name'];
+    $edit = array(
+      $lid => $this->randomName(100),
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $edit, t('Save translations'));
+
+    $this->assertText(t('No strings available.'), "No result anymore, so the string has been saved as customized.");

I would add a test that it actually shows up on a separate search for customized strings specifically.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,45 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    // compare the new strings with the existing strings the same strings
+    // are created.

"a string in the same format is created" instead of "the same strings are created", since we are only creating one string, right? :)

+++ b/core/modules/locale/locale.pages.incundefined
@@ -402,31 +402,45 @@ function locale_translate_edit_form_submit($form, &$form_state) {
+    $is_changed = FALSE;
+
+    if ($existing_translation && $existing_translation_objects[$lid]->translation != $new_translation_string_delimited) {
+      // If there is an existing translation in the DB and the new translation
+      // is not the same as the existing one.
+      $is_changed = TRUE;
     }
-    if ($has_translation) {
+    elseif (!$existing_translation && !empty($new_translation_string)) {
+      $is_changed = TRUE;
+    }

I can see that this is broken down to an if() and elseif() clause to delineate the two conditions. So maybe adding more docs to the second one, eg. "Newly entered translation." or somesuch would help underline the need for the two blocks of code (instead just one big condition).

Steps to reproduce

1. Install the latest Drupal 8.x using the standard profile and built-in English language.
2. Enable Language, Locale, and Entity Translation module.
3. Go to '/admin/config/regional/language', click 'Add language', and choose 'German'.
4. Go to '/admin/config/regional/translate/import', Choose German and add translation file 'drupal-7.17.de'. Don't touch that three checkboxes in this page.
5. Go to '/admin/config/regional/translate/translate', choose language 'German', search in 'Both translated and untranslated', and click filter.
6. Edit the first translated string at cloumn 'TRANSLATION FOR GERMAN', and click 'Save translations'.
7. Still this page, Choose 'Translation language' at German, 'Search in' at 'Only translated strings', 'Translation type' at 'Customized translation', click Filter. We get all strings as customized. In fact , we edited only one string.

hmmm, did I missing something. After I applied this patch, this problem remains. Maybe I need a refresh install to test this?

@smiletrl: well, the tests were not complete above either, so the fix might not be complete either. I don't think you would need a fresh install, but trying again never hurts :) Your steps to reproduce seem spot on.

BTW I believe @Schnitzel does not have capacity to work on this, so unassigning to make it evident that it is free to take. Need people to work on this :)

update Issue with report template

Updated issue summary remaining tasks to make it easier for people to find a part of the issue to do

Some of the remaining tasks are novice, some medium (like reviewing the tests to see if they are complete). The tasks do not have to be done in order. This is a good one for a couple people to jump into.

fix html

#13: 1832614-13.patch queued for re-testing.

#13: 1832614-13.patch queued for re-testing.

#13: 1832614-test-only-will-fail.patch queued for re-testing.

#7: 1832614-test.patch queued for re-testing.

#2: 1832614-1.patch queued for re-testing.

Updated issue summary to help novice people identify what they can do.

Since the past patch failed automated test, I guess a better idea is to seperate the test code from fix code.

This attachment is devided from #13 only to fix the problem, not including test code, and it's been updated according to suggestions at #14.

To test the patch, we will need another fresh installment of drupal8, because it won't have effect on existing records in db. That's why I didn't see it work last time.

Let's see will this patch make drupal robot happy:) Will work on test code later.

Well, the earlier tests looked pretty complete, so I think it is better to start looking if the test had issues, or it properly identified a bug with the fix itself. As opposed to starting to write a new test from scratch... Thanks for picking this up!

There're some bugs with the test code.

Firstly, before the last two test assertText, there's no search/filter action, like

    $this->drupalPost('admin/config/regional/translate/translate', $edit, t('Save translations'));

    $source = $this->assertText($translation->getString(), 'Translation still marked as non-customized.');

There should be search/filter action, e.g,

   $this->drupalPost('admin/config/regional/translate/translate', $search, t('Filter'));

after saving strings and right before assertion.

Secondly, in this issue's case, we need to create more than one translated and uncustomized strings(manually, we import .po file to do this), update one of them as customized, and then search the customized strings. I think we need assertText to find that customized string and assertNoText to make sure uncustomized strings are not saved as customized. This is also what suggested in #14.

Let me try to fix this:)

The first patch is the test code, and of course it will fail. It's been updated according to #29/#28, although there're some tweaks.

In this issue, when save unchanged translation strings, these strings are saved as customized. So

+    // Ensure unchanged translation string does appear if searching non-customized translation.
+    $search = array(
+      'string' => $string->getString(),
+      'langcode' => 'de',
+      'translation' => 'translated',
+      'customized' => '0',
+    );
+    $this->drupalPost('admin/config/regional/translate/translate', $search, t('Filter'));
+    $source = $this->assertText($string->getString(), 'Translation is not marked as customized.');

will fail. And the next search will fail too, because this search fails.

interdiff-13-29.txt indicates changes of test code between #13 and #29/#28.

The final patch is the whole patch, including fix code and test code. All these patches work fine in local tests. So, let's see will they satisfy drupal testbot~

Sending for tests. @smiletrl: thanks!

The changes look good to me and testing the fixed functionality properly. Thanks!

Long variable names, but committed to 8.x. Thanks.

Woot, thanks!

@smiletrl: thanks again! If you are looking for tasks on a similar scale #1853720: Hide language selection option is backwards or #1869328: Restore simplicity of language list are very good candidates :) Thanks for helping out :)