Merge branch '4511-handle-node-error-gracefully' into 'master'

Handle node details load failure gracefully on UI

Closes #4511 and #4552

See merge request gitlab-org/gitlab-ee!3992

(cherry picked from commit 2865c0ba)

68658bf4 Update classes for node status icon
0b34ccbd Emit `nodeDetailsLoadFailed` event on node details load failure
e8453efe Update test for node details load failure
6cf58a22 Handle node details load failure
589cfc25 Update tests for node details load failure
1c30818f Add changelog entry
78e83eb1 Default value to `0` when it is `null`
edaecca7 Update prop name

app/assets/stylesheets/pages/geo_nodes.scss

@@ -73,9 +73,13 @@
 }
 
 .geo-nodes {
-  .node-url-warning {
+  .node-status-icon-warning {
     fill: $gl-warning;
   }
+
+  .node-status-icon-failure {
+    fill: $gl-danger;
+  }
 }
 
 .node-details-list {

changelogs/unreleased-ee/4511-handle-node-error-gracefully.yml

+---
+title: Handle node details load failure gracefully on UI
+merge_request: 3992
+author:
+type: fixed

ee/app/assets/javascripts/geo_nodes/components/app.vue

@@ -80,8 +80,7 @@
             eventHub.$emit('nodeDetailsLoaded', this.store.getNodeDetails(nodeId));
           })
           .catch((err) => {
-            this.hasError = true;
-            this.errorMessage = err;
+            eventHub.$emit('nodeDetailsLoadFailed', nodeId, err);
           });
       },
       initNodeDetailsPolling(nodeId) {

ee/app/assets/javascripts/geo_nodes/components/geo_node_item.vue

 <script>
-  import icon from '~/vue_shared/components/icon.vue';
-  import loadingIcon from '~/vue_shared/components/loading_icon.vue';
-  import tooltip from '~/vue_shared/directives/tooltip';
+import { s__ } from '~/locale';
+import icon from '~/vue_shared/components/icon.vue';
+import loadingIcon from '~/vue_shared/components/loading_icon.vue';
+import tooltip from '~/vue_shared/directives/tooltip';
 
-  import eventHub from '../event_hub';
+import eventHub from '../event_hub';
 
-  import geoNodeActions from './geo_node_actions.vue';
-  import geoNodeDetails from './geo_node_details.vue';
+import geoNodeActions from './geo_node_actions.vue';
+import geoNodeDetails from './geo_node_details.vue';
 
-  export default {
+export default {
   components: {
     icon,
     loadingIcon,
@@ -39,37 +40,82 @@
   data() {
     return {
       isNodeDetailsLoading: true,
+      isNodeDetailsFailed: false,
       nodeHealthStatus: '',
+      errorMessage: '',
       nodeDetails: {},
     };
   },
   computed: {
-      showInsecureUrlWarning() {
+    isNodeNonHTTPS() {
       return this.node.url.startsWith('http://');
     },
+    showNodeStatusIcon() {
+      if (this.isNodeDetailsLoading) {
+        return false;
+      }
+
+      return this.isNodeNonHTTPS || this.isNodeDetailsFailed;
+    },
+    showNodeDetails() {
+      if (!this.isNodeDetailsLoading) {
+        return !this.isNodeDetailsFailed;
+      }
+      return false;
+    },
+    nodeStatusIconClass() {
+      const iconClasses = 'prepend-left-10 pull-left';
+      if (this.isNodeDetailsFailed) {
+        return `${iconClasses} node-status-icon-failure`;
+      }
+      return `${iconClasses} node-status-icon-warning`;
+    },
+    nodeStatusIconName() {
+      if (this.isNodeDetailsFailed) {
+        return 'status_failed_borderless';
+      }
+      return 'warning';
+    },
+    nodeStatusIconTooltip() {
+      if (this.isNodeDetailsFailed) {
+        return '';
+      }
+      return s__('GeoNodes|You have configured Geo nodes using an insecure HTTP connection. We recommend the use of HTTPS.');
+    },
   },
   created() {
     eventHub.$on('nodeDetailsLoaded', this.handleNodeDetails);
+    eventHub.$on('nodeDetailsLoadFailed', this.handleNodeDetailsFailure);
   },
   mounted() {
     this.handleMounted();
   },
   beforeDestroy() {
     eventHub.$off('nodeDetailsLoaded', this.handleNodeDetails);
+    eventHub.$off('nodeDetailsLoadFailed', this.handleNodeDetailsFailure);
   },
   methods: {
     handleNodeDetails(nodeDetails) {
       if (this.node.id === nodeDetails.id) {
         this.isNodeDetailsLoading = false;
+        this.isNodeDetailsFailed = false;
+        this.errorMessage = '';
         this.nodeDetails = nodeDetails;
         this.nodeHealthStatus = nodeDetails.health;
       }
     },
+    handleNodeDetailsFailure(nodeId, err) {
+      if (this.node.id === nodeId) {
+        this.isNodeDetailsLoading = false;
+        this.isNodeDetailsFailed = true;
+        this.errorMessage = err.message;
+      }
+    },
     handleMounted() {
       eventHub.$emit('pollNodeDetails', this.node.id);
     },
   },
-  };
+};
 </script>
 
 <template>
@@ -88,14 +134,13 @@
             />
             <icon
               v-tooltip
-              v-if="!isNodeDetailsLoading && showInsecureUrlWarning"
-              css-classes="prepend-left-10 pull-left node-url-warning"
-              name="warning"
+              v-if="showNodeStatusIcon"
               data-container="body"
               data-placement="bottom"
-              :title="s__(`GeoNodes|You have configured Geo nodes using
-an insecure HTTP connection. We recommend the use of HTTPS.`)"
+              :name="nodeStatusIconName"
               :size="18"
+              :css-classes="nodeStatusIconClass"
+              :title="nodeStatusIconTooltip"
             />
             <span class="inline pull-left prepend-left-10">
               <span
@@ -115,16 +160,24 @@ an insecure HTTP connection. We recommend the use of HTTPS.`)"
         </div>
       </div>
       <geo-node-actions
-        v-if="!isNodeDetailsLoading && nodeActionsAllowed"
+        v-if="showNodeDetails && nodeActionsAllowed"
         :node="node"
         :node-edit-allowed="nodeEditAllowed"
         :node-missing-oauth="nodeDetails.missingOAuthApplication"
       />
     </div>
     <geo-node-details
-      v-if="!isNodeDetailsLoading"
+      v-if="showNodeDetails"
       :node="node"
       :node-details="nodeDetails"
     />
+    <div
+      v-if="isNodeDetailsFailed"
+      class="prepend-top-10"
+    >
+      <p class="health-message">
+        {{ errorMessage }}
+      </p>
+    </div>
   </li>
 </template>

ee/app/assets/javascripts/geo_nodes/store/geo_nodes_store.js

@@ -44,34 +44,34 @@ export default class GeoNodesStore {
       missingOAuthApplication: rawNodeDetails.missing_oauth_application,
       storageShardsMatch: rawNodeDetails.storage_shards_match,
       replicationSlots: {
-        totalCount: rawNodeDetails.replication_slots_count,
-        successCount: rawNodeDetails.replication_slots_used_count,
+        totalCount: rawNodeDetails.replication_slots_count || 0,
+        successCount: rawNodeDetails.replication_slots_used_count || 0,
         failureCount: 0,
       },
       repositories: {
-        totalCount: rawNodeDetails.repositories_count,
-        successCount: rawNodeDetails.repositories_synced_count,
-        failureCount: rawNodeDetails.repositories_failed_count,
+        totalCount: rawNodeDetails.repositories_count || 0,
+        successCount: rawNodeDetails.repositories_synced_count || 0,
+        failureCount: rawNodeDetails.repositories_failed_count || 0,
       },
       wikis: {
-        totalCount: rawNodeDetails.wikis_count,
-        successCount: rawNodeDetails.wikis_synced_count,
-        failureCount: rawNodeDetails.wikis_failed_count,
+        totalCount: rawNodeDetails.wikis_count || 0,
+        successCount: rawNodeDetails.wikis_synced_count || 0,
+        failureCount: rawNodeDetails.wikis_failed_count || 0,
       },
       lfs: {
-        totalCount: rawNodeDetails.lfs_objects_count,
-        successCount: rawNodeDetails.lfs_objects_synced_count,
-        failureCount: rawNodeDetails.lfs_objects_failed_count,
+        totalCount: rawNodeDetails.lfs_objects_count || 0,
+        successCount: rawNodeDetails.lfs_objects_synced_count || 0,
+        failureCount: rawNodeDetails.lfs_objects_failed_count || 0,
       },
       jobArtifacts: {
-        totalCount: rawNodeDetails.job_artifacts_count,
-        successCount: rawNodeDetails.job_artifacts_synced_count,
-        failureCount: rawNodeDetails.job_artifacts_failed_count,
+        totalCount: rawNodeDetails.job_artifacts_count || 0,
+        successCount: rawNodeDetails.job_artifacts_synced_count || 0,
+        failureCount: rawNodeDetails.job_artifacts_failed_count || 0,
       },
       attachments: {
-        totalCount: rawNodeDetails.attachments_count,
-        successCount: rawNodeDetails.attachments_synced_count,
-        failureCount: rawNodeDetails.attachments_failed_count,
+        totalCount: rawNodeDetails.attachments_count || 0,
+        successCount: rawNodeDetails.attachments_synced_count || 0,
+        failureCount: rawNodeDetails.attachments_failed_count || 0,
       },
       lastEvent: {
         id: rawNodeDetails.last_event_id,

spec/javascripts/geo_nodes/components/app_spec.js

@@ -94,15 +94,15 @@ describe('AppComponent', () => {
         }, 0);
       });
 
-      it('sets error flag and message on failure', (done) => {
+      it('emits `nodeDetailsLoadFailed` event on failure', (done) => {
         const err = 'Something went wrong';
         const mock = new MockAdapter(axios);
+        spyOn(eventHub, '$emit');
         mock.onGet(`${vm.service.geoNodeDetailsBasePath}/2/status.json`).reply(500, err);
 
         vm.fetchNodeDetails(2);
         setTimeout(() => {
-          expect(vm.hasError).toBeTruthy();
-          expect(vm.errorMessage.response.data).toBe(err);
+          expect(eventHub.$emit).toHaveBeenCalledWith('nodeDetailsLoadFailed', 2, jasmine.any(Object));
           done();
         }, 0);
       });

spec/javascripts/geo_nodes/components/geo_node_item_spec.js

@@ -31,24 +31,118 @@ describe('GeoNodeItemComponent', () => {
   describe('data', () => {
     it('returns default data props', () => {
       expect(vm.isNodeDetailsLoading).toBeTruthy();
+      expect(vm.isNodeDetailsFailed).toBeFalsy();
       expect(vm.nodeHealthStatus).toBe('');
+      expect(vm.errorMessage).toBe('');
       expect(typeof vm.nodeDetails).toBe('object');
     });
   });
 
   describe('computed', () => {
-    describe('showInsecureUrlWarning', () => {
-      it('returns boolean value representing URL protocol security', () => {
-        // With altered mock data for secure URL
-        const mockNode = Object.assign({}, mockNodes[0], {
+    let vmHttps;
+    let mockNode;
+
+    beforeEach(() => {
+      // Altered mock data for secure URL
+      mockNode = Object.assign({}, mockNodes[0], {
         url: 'https://127.0.0.1:3001/',
       });
-        const vmX = createComponent(mockNode);
-        expect(vmX.showInsecureUrlWarning).toBeFalsy();
-        vmX.$destroy();
+      vmHttps = createComponent(mockNode);
+    });
+
+    afterEach(() => {
+      vmHttps.$destroy();
+    });
 
+    describe('isNodeNonHTTPS', () => {
+      it('returns `true` if Node URL protocol is non-HTTPS', () => {
         // With default mock data
-        expect(vm.showInsecureUrlWarning).toBeTruthy();
+        expect(vm.isNodeNonHTTPS).toBeTruthy();
+      });
+
+      it('returns `false` is Node URL protocol is HTTPS', () => {
+        // With altered mock data
+        expect(vmHttps.isNodeNonHTTPS).toBeFalsy();
+      });
+    });
+
+    describe('showNodeStatusIcon', () => {
+      it('returns `false` if Node details are still loading', () => {
+        vm.isNodeDetailsLoading = true;
+        expect(vm.showNodeStatusIcon).toBeFalsy();
+      });
+
+      it('returns `true` if Node details failed to load', () => {
+        vm.isNodeDetailsLoading = false;
+        vm.isNodeDetailsFailed = true;
+        expect(vm.showNodeStatusIcon).toBeTruthy();
+      });
+
+      it('returns `true` if Node details loaded and Node URL is non-HTTPS', () => {
+        vm.isNodeDetailsLoading = false;
+        vm.isNodeDetailsFailed = false;
+        expect(vm.showNodeStatusIcon).toBeTruthy();
+      });
+
+      it('returns `false` if Node details loaded and Node URL is HTTPS', () => {
+        vmHttps.isNodeDetailsLoading = false;
+        vmHttps.isNodeDetailsFailed = false;
+        expect(vmHttps.showNodeStatusIcon).toBeFalsy();
+      });
+    });
+
+    describe('showNodeDetails', () => {
+      it('returns `false` if Node details are still loading', () => {
+        vm.isNodeDetailsLoading = true;
+        expect(vm.showNodeDetails).toBeFalsy();
+      });
+
+      it('returns `false` if Node details failed to load', () => {
+        vm.isNodeDetailsLoading = false;
+        vm.isNodeDetailsFailed = true;
+        expect(vm.showNodeDetails).toBeFalsy();
+      });
+
+      it('returns `true` if Node details loaded', () => {
+        vm.isNodeDetailsLoading = false;
+        vm.isNodeDetailsFailed = false;
+        expect(vm.showNodeDetails).toBeTruthy();
+      });
+    });
+
+    describe('nodeStatusIconClass', () => {
+      it('returns `node-status-icon-failure` along with default classes if Node details failed to load', () => {
+        vm.isNodeDetailsFailed = true;
+        expect(vm.nodeStatusIconClass).toBe('prepend-left-10 pull-left node-status-icon-failure');
+      });
+
+      it('returns `node-status-icon-warning` along with default classes if Node details loaded and Node URL is non-HTTPS', () => {
+        vm.isNodeDetailsFailed = false;
+        expect(vm.nodeStatusIconClass).toBe('prepend-left-10 pull-left node-status-icon-warning');
+      });
+    });
+
+    describe('nodeStatusIconName', () => {
+      it('returns `warning` if Node details loaded and Node URL is non-HTTPS', () => {
+        vm.isNodeDetailsFailed = false;
+        expect(vm.nodeStatusIconName).toBe('warning');
+      });
+
+      it('returns `status_failed_borderless` if Node details failed to load', () => {
+        vm.isNodeDetailsFailed = true;
+        expect(vm.nodeStatusIconName).toBe('status_failed_borderless');
+      });
+    });
+
+    describe('nodeStatusIconTooltip', () => {
+      it('returns empty string if Node details failed to load', () => {
+        vm.isNodeDetailsFailed = true;
+        expect(vm.nodeStatusIconTooltip).toBe('');
+      });
+
+      it('returns tooltip string if Node details loaded and Node URL is non-HTTPS', () => {
+        vm.isNodeDetailsFailed = false;
+        expect(vm.nodeStatusIconTooltip).toBe('You have configured Geo nodes using an insecure HTTP connection. We recommend the use of HTTPS.');
       });
     });
   });
@@ -58,13 +152,15 @@ describe('GeoNodeItemComponent', () => {
       it('intializes props based on provided `nodeDetails`', () => {
         // With altered mock data with matching ID
         const mockNode = Object.assign({}, mockNodes[1]);
-        const vmX = createComponent(mockNode);
+        const vmNodePrimary = createComponent(mockNode);
 
-        vmX.handleNodeDetails(mockNodeDetails);
-        expect(vmX.isNodeDetailsLoading).toBeFalsy();
-        expect(vmX.nodeDetails).toBe(mockNodeDetails);
-        expect(vmX.nodeHealthStatus).toBe(mockNodeDetails.health);
-        vmX.$destroy();
+        vmNodePrimary.handleNodeDetails(mockNodeDetails);
+        expect(vmNodePrimary.isNodeDetailsLoading).toBeFalsy();
+        expect(vmNodePrimary.isNodeDetailsFailed).toBeFalsy();
+        expect(vmNodePrimary.errorMessage).toBe('');
+        expect(vmNodePrimary.nodeDetails).toBe(mockNodeDetails);
+        expect(vmNodePrimary.nodeHealthStatus).toBe(mockNodeDetails.health);
+        vmNodePrimary.$destroy();
 
         // With default mock data without matching ID
         vm.handleNodeDetails(mockNodeDetails);
@@ -74,6 +170,16 @@ describe('GeoNodeItemComponent', () => {
       });
     });
 
+    describe('handleNodeDetailsFailure', () => {
+      it('initializes props for Node details failure', () => {
+        const err = 'Something went wrong';
+        vm.handleNodeDetailsFailure(1, { message: err });
+        expect(vm.isNodeDetailsLoading).toBeFalsy();
+        expect(vm.isNodeDetailsFailed).toBeTruthy();
+        expect(vm.errorMessage).toBe(err);
+      });
+    });
+
     describe('handleMounted', () => {
       it('emits `pollNodeDetails` event and passes node ID', () => {
         spyOn(eventHub, '$emit');
@@ -90,6 +196,7 @@ describe('GeoNodeItemComponent', () => {
 
       const vmX = createComponent();
       expect(eventHub.$on).toHaveBeenCalledWith('nodeDetailsLoaded', jasmine.any(Function));
+      expect(eventHub.$on).toHaveBeenCalledWith('nodeDetailsLoadFailed', jasmine.any(Function));
       vmX.$destroy();
     });
   });
@@ -101,6 +208,7 @@ describe('GeoNodeItemComponent', () => {
       const vmX = createComponent();
       vmX.$destroy();
       expect(eventHub.$off).toHaveBeenCalledWith('nodeDetailsLoaded', jasmine.any(Function));
+      expect(eventHub.$off).toHaveBeenCalledWith('nodeDetailsLoadFailed', jasmine.any(Function));
     });
   });
 
@@ -121,5 +229,16 @@ describe('GeoNodeItemComponent', () => {
     it('renders node badge `Primary`', () => {
       expect(vm.$el.querySelectorAll('.node-badge.primary-node').length).not.toBe(0);
     });
+
+    it('renders node error message', (done) => {
+      const err = 'Something error message';
+      vm.isNodeDetailsFailed = true;
+      vm.errorMessage = err;
+      Vue.nextTick(() => {
+        expect(vm.$el.querySelectorAll('p.health-message').length).not.toBe(0);
+        expect(vm.$el.querySelector('p.health-message').innerText.trim()).toBe(err);
+        done();
+      });
+    });
   });
 });

spec/javascripts/geo_nodes/mock_data.js

@@ -133,7 +133,7 @@ export const mockNodeDetails = {
     successCount: 0,
     failureCount: 0,
   },
-  job_artifacts: {
+  jobArtifacts: {
     totalCount: 0,
     successCount: 0,
     failureCount: 0,