From 2e30833c4debf385ac85761497a433f5bab014c6 Mon Sep 17 00:00:00 2001
From: Becky Siegel <beckysiegel@google.com>
Date: Tue, 22 Nov 2016 15:55:14 -0800
Subject: [PATCH] Add check for binary in total calculations

In the calculations for aggregating size_delta_inserted and
size_delta_deleted, which aggregate the total binary additions and
deletions, there was a missing check for whether a file was binary or
not before adding the value. This caused a strange total binary change
to appear. This change adds the binary check and also updates tests
to accurately represent that non-binary changes can have size_delta as
an attribute.

Bug: Issue 4992
Change-Id: I52da1c8f9fdcf6be94658f1b95721453a71a4213
---
 .../change/gr-file-list/gr-file-list.js       |  6 ++-
 .../gr-file-list/gr-file-list_test.html       | 38 +++++++++++++++----
 2 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
index f756bebd71..3f134942da 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list.js
@@ -179,8 +179,10 @@
         var inserted = obj.lines_inserted ? obj.lines_inserted : 0;
         var deleted = obj.lines_deleted ? obj.lines_deleted : 0;
         var total_size = (obj.size && obj.binary) ? obj.size : 0;
-        var size_delta_inserted = obj.size_delta > 0 ? obj.size_delta : 0;
-        var size_delta_deleted = obj.size_delta < 0 ? obj.size_delta : 0;
+        var size_delta_inserted =
+            obj.binary && obj.size_delta > 0 ? obj.size_delta : 0;
+        var size_delta_deleted =
+            obj.binary && obj.size_delta < 0 ? obj.size_delta : 0;
 
         return {
           inserted: acc.inserted + inserted,
diff --git a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
index 152227e0cf..b2c69a73d2 100644
--- a/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
+++ b/polygerrit-ui/app/elements/change/gr-file-list/gr-file-list_test.html
@@ -103,8 +103,20 @@ limitations under the License.
     test('calculate totals for patch number', function() {
       element._files = [
         {__path: '/COMMIT_MSG', lines_inserted: 9},
-        {__path: 'file_added_in_rev2.txt', lines_inserted: 1, lines_deleted: 1},
-        {__path: 'myfile.txt', lines_inserted: 1, lines_deleted: 1},
+        {
+          __path: 'file_added_in_rev2.txt',
+          lines_inserted: 1,
+          lines_deleted: 1,
+          size_delta: 10,
+          size: 100,
+        },
+        {
+          __path: 'myfile.txt',
+          lines_inserted: 1,
+          lines_deleted: 1,
+          size_delta: 10,
+          size: 100,
+        },
       ];
       assert.deepEqual(element._patchChange, {
         inserted: 2,
@@ -113,6 +125,8 @@ limitations under the License.
         size_delta_deleted: 0,
         total_size: 0,
       });
+      assert.isTrue(element._hideBinaryChangeTotals);
+      assert.isFalse(element._hideChangeTotals);
 
       // Test with a commit message that isn't the first file.
       element._files = [
@@ -127,6 +141,8 @@ limitations under the License.
         size_delta_deleted: 0,
         total_size: 0,
       });
+      assert.isTrue(element._hideBinaryChangeTotals);
+      assert.isFalse(element._hideChangeTotals);
 
       // Test with no commit message.
       element._files = [
@@ -140,6 +156,8 @@ limitations under the License.
         size_delta_deleted: 0,
         total_size: 0,
       });
+      assert.isTrue(element._hideBinaryChangeTotals);
+      assert.isFalse(element._hideChangeTotals);
 
       // Test with files missing either lines_inserted or lines_deleted.
       element._files = [
@@ -153,6 +171,8 @@ limitations under the License.
         size_delta_deleted: 0,
         total_size: 0,
       });
+      assert.isTrue(element._hideBinaryChangeTotals);
+      assert.isFalse(element._hideChangeTotals);
     });
 
     test('binary only files', function() {
@@ -168,6 +188,8 @@ limitations under the License.
         size_delta_deleted: -5,
         total_size: 220,
       });
+      assert.isFalse(element._hideBinaryChangeTotals);
+      assert.isTrue(element._hideChangeTotals);
     });
 
     test('binary and regular files', function() {
@@ -175,7 +197,7 @@ limitations under the License.
         {__path: '/COMMIT_MSG', lines_inserted: 9},
         {__path: 'file_binary', binary: true, size_delta: 10, size: 100},
         {__path: 'file_binary', binary: true, size_delta: -5, size: 120},
-        {__path: 'myfile.txt', lines_deleted: 5},
+        {__path: 'myfile.txt', lines_deleted: 5, size_delta: -10, size: 100},
         {__path: 'myfile2.txt', lines_inserted: 10},
       ];
       assert.deepEqual(element._patchChange, {
@@ -185,9 +207,11 @@ limitations under the License.
         size_delta_deleted: -5,
         total_size: 220,
       });
+      assert.isFalse(element._hideBinaryChangeTotals);
+      assert.isFalse(element._hideChangeTotals);
     });
 
-    test('_formatBytes function', function(){
+    test('_formatBytes function', function() {
       var table = {
         64: '+64 B',
         1023: '+1023 B',
@@ -200,7 +224,7 @@ limitations under the License.
         '-4096': '-4 KiB',
         '-1073741824': '-1 GiB',
         0: '+/-0 B',
-      }
+      };
 
       for (var bytes in table) {
         if (table.hasOwnProperty(bytes)) {
@@ -209,7 +233,7 @@ limitations under the License.
       }
     });
 
-    test('_formatPercentage function', function(){
+    test('_formatPercentage function', function() {
       var table = [
         { size: 100,
           delta: 100,
@@ -235,7 +259,7 @@ limitations under the License.
           delta: 10,
           display: '(+10%)',
         },
-      ]
+      ];
 
       table.forEach(function(item) {
         assert.equal(element._formatPercentage(