From 0ee62135f2f91a500fc15953b14b99e74d95abb4 Mon Sep 17 00:00:00 2001
From: Paladox none <thomasmulhall410@yahoo.com>
Date: Fri, 8 Feb 2019 17:14:15 +0000
Subject: Fix avatars not showing correctly

Sometimes avatars do not load on the change metadata or the dashboard.

I found this is because the this._account check sometimes returns null
which then cause it to hit the this.hidden = true. Because there was
no setting hidden back to false, gr-avatar stayed permanently hidden
even if "account" became defined.

Bug: Issue 9851
Change-Id: I2627d7c830fc8825790f15359024496ff9e0e6be
---
 .../app/elements/shared/gr-avatar/gr-avatar.js     | 21 ++++---
 .../elements/shared/gr-avatar/gr-avatar_test.html  | 70 ++++++++++++++++++++--
 2 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar.js b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar.js
index f32e940b3f..bf563823ac 100644
--- a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar.js
+++ b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar.js
@@ -41,25 +41,30 @@
 
     attached() {
       Promise.all([
-        this.$.restAPI.getConfig(),
+        this._getConfig(),
         Gerrit.awaitPluginsLoaded(),
       ]).then(([cfg]) => {
         this._hasAvatars = !!(cfg && cfg.plugin && cfg.plugin.has_avatars);
-        if (this._hasAvatars && this.account) {
-          // src needs to be set if avatar becomes visible
-          this._updateAvatarURL();
-        } else {
-          this.hidden = true;
-        }
+
+        this._updateAvatarURL();
       });
     },
 
+    _getConfig() {
+      return this.$.restAPI.getConfig();
+    },
+
     _accountChanged(account) {
       this._updateAvatarURL();
     },
 
     _updateAvatarURL() {
-      if (this.hidden || !this._hasAvatars) { return; }
+      if (!this._hasAvatars || !this.account) {
+        this.hidden = true;
+        return;
+      }
+      this.hidden = false;
+
       const url = this._buildAvatarURL(this.account);
       if (url) {
         this.style.backgroundImage = 'url("' + url + '")';
diff --git a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.html b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.html
index f137c7fa01..5ce17c05b9 100644
--- a/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.html
+++ b/polygerrit-ui/app/elements/shared/gr-avatar/gr-avatar_test.html
@@ -35,14 +35,17 @@ limitations under the License.
 <script>
   suite('gr-avatar tests', () => {
     let element;
+    let sandbox;
 
     setup(() => {
-      stub('gr-rest-api-interface', {
-        getConfig() { return Promise.resolve({plugin: {has_avatars: true}}); },
-      });
+      sandbox = sinon.sandbox.create();
       element = fixture('basic');
     });
 
+    teardown(() => {
+      sandbox.restore();
+    });
+
     test('methods', () => {
       assert.equal(element._buildAvatarURL(
           {
@@ -94,22 +97,32 @@ limitations under the License.
             ],
           }),
           '/accounts/123/avatar?s=16');
+      assert.equal(element._buildAvatarURL(undefined), '');
     });
 
     test('dom for existing account', () => {
       assert.isFalse(element.hasAttribute('hidden'));
+
+      sandbox.stub(element, '_getConfig', () => {
+        return Promise.resolve({plugin: {has_avatars: true}});
+      });
+
       element.imageSize = 64;
       element.account = {
         _account_id: 123,
       };
+
       assert.strictEqual(element.style.backgroundImage, '');
+
       // Emulate plugins loaded.
       Gerrit._setPluginsPending([]);
-      return Promise.all([
+
+      Promise.all([
         element.$.restAPI.getConfig(),
         Gerrit.awaitPluginsLoaded(),
       ]).then(() => {
         assert.isFalse(element.hasAttribute('hidden'));
+
         assert.isTrue(
             element.style.backgroundImage.includes('/accounts/123/avatar?s=64'));
       });
@@ -117,10 +130,57 @@ limitations under the License.
 
     test('dom for non available account', () => {
       assert.isFalse(element.hasAttribute('hidden'));
-      element.account = null;
+
+      sandbox.stub(element, '_getConfig', () => {
+        return Promise.resolve({plugin: {has_avatars: true}});
+      });
+
+      // Emulate plugins loaded.
+      Gerrit._setPluginsPending([]);
+
+      return Promise.all([
+        element.$.restAPI.getConfig(),
+        Gerrit.awaitPluginsLoaded(),
+      ]).then(() => {
+        assert.isTrue(element.hasAttribute('hidden'));
+
+        assert.strictEqual(element.style.backgroundImage, '');
+      });
+    });
+
+    test('avatar config not set and account not set', () => {
+      assert.isFalse(element.hasAttribute('hidden'));
+
+      sandbox.stub(element, '_getConfig', () => {
+        return Promise.resolve({});
+      });
+
+      // Emulate plugins loaded.
+      Gerrit._setPluginsPending([]);
+
+      return Promise.all([
+        element.$.restAPI.getConfig(),
+        Gerrit.awaitPluginsLoaded(),
+      ]).then(() => {
+        assert.isTrue(element.hasAttribute('hidden'));
+      });
+    });
+
+    test('avatar config not set and account set', () => {
       assert.isFalse(element.hasAttribute('hidden'));
+
+      sandbox.stub(element, '_getConfig', () => {
+        return Promise.resolve({});
+      });
+
+      element.imageSize = 64;
+      element.account = {
+        _account_id: 123,
+      };
+
       // Emulate plugins loaded.
       Gerrit._setPluginsPending([]);
+
       return Promise.all([
         element.$.restAPI.getConfig(),
         Gerrit.awaitPluginsLoaded(),
-- 
cgit v1.2.3