From 5cb40d1ba70a9eb220c99938d950c6b5ee761502 Mon Sep 17 00:00:00 2001
From: Wyatt Allen <wyatta@google.com>
Date: Mon, 18 Sep 2017 15:47:22 -0700
Subject: [PATCH] Include settings checkbox to drive legacycid_in_change_table
 preference

The legacycid_in_change_table preference predates the change_table
column preference and is thus persisted in separate field. With this
change, a checkbox to drive this preference is added to the change table
editor component as though it were a change_table preference.

Feature: Issue 5333
Change-Id: Ia055ef23d7eec341455680b67bb6f3b0bdcbf531
---
 .../gr-change-table-editor.html               | 11 +++++++
 .../gr-change-table-editor.js                 | 10 ++++++
 .../gr-change-table-editor_test.html          | 31 ++++++++++++++-----
 .../gr-settings-view/gr-settings-view.html    |  1 +
 .../gr-settings-view/gr-settings-view.js      |  6 +++-
 .../gr-settings-view_test.html                | 19 ++++++++++++
 6 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
index c93ed0cc68..43343de0aa 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.html
@@ -49,6 +49,17 @@ limitations under the License.
           </tr>
         </thead>
         <tbody>
+          <tr>
+            <td>Number</td>
+            <td
+                class="checkboxContainer"
+                on-tap="_handleTargetTap">
+              <input
+                  type="checkbox"
+                  name="number"
+                  checked$="[[showNumber]]">
+            </td>
+          </tr>
           <template is="dom-repeat" items="[[columnNames]]">
             <tr>
               <td>[[item]]</td>
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
index 50a1146185..4d87f9edfa 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor.js
@@ -22,6 +22,10 @@
         type: Array,
         notify: true,
       },
+      showNumber: {
+        type: Boolean,
+        notify: true,
+      },
     },
 
     behaviors: [
@@ -53,6 +57,12 @@
         // The target is the checkbox itself.
         checkbox = Polymer.dom(e).rootTarget;
       }
+
+      if (checkbox.name === 'number') {
+        this.showNumber = checkbox.checked;
+        return;
+      }
+
       this.set('displayedColumns',
           this._updateDisplayedColumns(
               this.displayedColumns, checkbox.name, checkbox.checked));
diff --git a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
index 61093b9ef2..00ef16fb02 100644
--- a/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-change-table-editor/gr-change-table-editor_test.html
@@ -62,15 +62,17 @@ limitations under the License.
       const rows = element.$$('tbody').querySelectorAll('tr');
       let tds;
 
-      assert.equal(rows.length, element.columnNames.length);
+      // The `+ 1` is for the number column, which isn't included in the change
+      // table behavior's list.
+      assert.equal(rows.length, element.columnNames.length + 1);
       for (let i = 0; i < columns.length; i++) {
-        tds = rows[i].querySelectorAll('td');
+        tds = rows[i + 1].querySelectorAll('td');
         assert.equal(tds[0].textContent, columns[i]);
       }
     });
 
     test('hide item', () => {
-      const checkbox = element.$$('table input');
+      const checkbox = element.$$('table tr:nth-child(2) input');
       const isChecked = checkbox.checked;
       const displayedLength = element.displayedColumns.length;
       assert.isTrue(isChecked);
@@ -78,8 +80,7 @@ limitations under the License.
       MockInteractions.tap(checkbox);
       flushAsynchronousOperations();
 
-      assert.equal(element.displayedColumns.length,
-          displayedLength - 1);
+      assert.equal(element.displayedColumns.length, displayedLength - 1);
     });
 
     test('show item', () => {
@@ -91,7 +92,7 @@ limitations under the License.
         'Updated',
       ]);
       flushAsynchronousOperations();
-      const checkbox = element.$$('table input');
+      const checkbox = element.$$('table tr:nth-child(2) input');
       const isChecked = checkbox.checked;
       const displayedLength = element.displayedColumns.length;
       assert.isFalse(isChecked);
@@ -105,9 +106,9 @@ limitations under the License.
     });
 
     test('_handleTargetTap', () => {
-      const checkbox = element.$$('table input');
+      const checkbox = element.$$('table tr:nth-child(2) input');
       let originalDisplayedColumns = element.displayedColumns;
-      const td = element.$$('table .checkboxContainer');
+      const td = element.$$('table tr:nth-child(2) .checkboxContainer');
       const displayedColumnStub =
           sandbox.stub(element, '_updateDisplayedColumns');
 
@@ -125,6 +126,20 @@ limitations under the License.
           checkbox.checked));
     });
 
+    test('_handleTargetTap on number', () => {
+      element.showNumber = false;
+      const checkbox = element.$$('table tr:nth-child(1) input');
+      const displayedColumnStub =
+          sandbox.stub(element, '_updateDisplayedColumns');
+
+      MockInteractions.tap(checkbox);
+      assert.isFalse(displayedColumnStub.called);
+      assert.isTrue(element.showNumber);
+
+      MockInteractions.tap(checkbox);
+      assert.isFalse(element.showNumber);
+    });
+
     test('_updateDisplayedColumns', () => {
       let name = 'Subject';
       let checked = false;
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
index d039a3b864..cacccdac01 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.html
@@ -321,6 +321,7 @@ limitations under the License.
         </h2>
         <fieldset id="changeTableColumns">
           <gr-change-table-editor
+              show-number="{{_showNumber}}"
               displayed-columns="{{_localChangeTableColumns}}">
           </gr-change-table-editor>
           <gr-button
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
index c3332b3061..25dd259c45 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view.js
@@ -121,6 +121,8 @@
        * For testing purposes.
        */
       _loadingPromise: Object,
+
+      _showNumber: Boolean,
     },
 
     behaviors: [
@@ -132,7 +134,7 @@
       '_handlePrefsChanged(_localPrefs.*)',
       '_handleDiffPrefsChanged(_diffPrefs.*)',
       '_handleMenuChanged(_localMenu.splices)',
-      '_handleChangeTableChanged(_localChangeTableColumns)',
+      '_handleChangeTableChanged(_localChangeTableColumns, _showNumber)',
     ],
 
     attached() {
@@ -147,6 +149,7 @@
 
       promises.push(this.$.restAPI.getPreferences().then(prefs => {
         this.prefs = prefs;
+        this._showNumber = !!prefs.legacycid_in_change_table;
         this._copyPrefs('_localPrefs', 'prefs');
         this._cloneMenu();
         this._cloneChangeTableColumns();
@@ -303,6 +306,7 @@
 
     _handleSaveChangeTable() {
       this.set('prefs.change_table', this._localChangeTableColumns);
+      this.set('prefs.legacycid_in_change_table', this._showNumber);
       this._cloneChangeTableColumns();
       return this.$.restAPI.savePreferences(this.prefs).then(() => {
         this._changeTableChanged = false;
diff --git a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
index e1182c1cae..868a9e326b 100644
--- a/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
+++ b/polygerrit-ui/app/elements/settings/gr-settings-view/gr-settings-view_test.html
@@ -386,6 +386,25 @@ limitations under the License.
       assert.isTrue(element.$.emailEditor.loadData.calledOnce);
     });
 
+    test('_handleSaveChangeTable', () => {
+      let newColumns = ['Owner', 'Project', 'Branch'];
+      element._localChangeTableColumns = newColumns.slice(0);
+      element._showNumber = false;
+      const cloneStub = sandbox.stub(element, '_cloneChangeTableColumns');
+      element._handleSaveChangeTable();
+      assert.isTrue(cloneStub.calledOnce);
+      assert.deepEqual(element.prefs.change_table, newColumns);
+      assert.isNotOk(element.prefs.legacycid_in_change_table);
+
+      newColumns = ['Size'];
+      element._localChangeTableColumns = newColumns;
+      element._showNumber = true;
+      element._handleSaveChangeTable();
+      assert.isTrue(cloneStub.calledTwice);
+      assert.deepEqual(element.prefs.change_table, newColumns);
+      assert.isTrue(element.prefs.legacycid_in_change_table);
+    });
+
     suite('_getFilterDocsLink', () => {
       test('with http: docs base URL', () => {
         const base = 'http://example.com/';