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

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 {
-    components: {
-      icon,
-      loadingIcon,
-      geoNodeActions,
-      geoNodeDetails,
+export default {
+  components: {
+    icon,
+    loadingIcon,
+    geoNodeActions,
+    geoNodeDetails,
+  },
+  directives: {
+    tooltip,
+  },
+  props: {
+    node: {
+      type: Object,
+      required: true,
     },
-    directives: {
-      tooltip,
+    primaryNode: {
+      type: Boolean,
+      required: true,
     },
-    props: {
-      node: {
-        type: Object,
-        required: true,
-      },
-      primaryNode: {
-        type: Boolean,
-        required: true,
-      },
-      nodeActionsAllowed: {
-        type: Boolean,
-        required: true,
-      },
-      nodeEditAllowed: {
-        type: Boolean,
-        required: true,
-      },
+    nodeActionsAllowed: {
+      type: Boolean,
+      required: true,
     },
-    data() {
-      return {
-        isNodeDetailsLoading: true,
-        nodeHealthStatus: '',
-        nodeDetails: {},
-      };
+    nodeEditAllowed: {
+      type: Boolean,
+      required: true,
     },
-    computed: {
-      showInsecureUrlWarning() {
-        return this.node.url.startsWith('http://');
-      },
+  },
+  data() {
+    return {
+      isNodeDetailsLoading: true,
+      isNodeDetailsFailed: false,
+      nodeHealthStatus: '',
+      errorMessage: '',
+      nodeDetails: {},
+    };
+  },
+  computed: {
+    isNodeNonHTTPS() {
+      return this.node.url.startsWith('http://');
     },
-    created() {
-      eventHub.$on('nodeDetailsLoaded', this.handleNodeDetails);
+    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`;
     },
-    mounted() {
-      this.handleMounted();
+    nodeStatusIconName() {
+      if (this.isNodeDetailsFailed) {
+        return 'status_failed_borderless';
+      }
+      return 'warning';
     },
-    beforeDestroy() {
-      eventHub.$off('nodeDetailsLoaded', this.handleNodeDetails);
+    nodeStatusIconTooltip() {
+      if (this.isNodeDetailsFailed) {
+        return '';
+      }
+      return s__('GeoNodes|You have configured Geo nodes using an insecure HTTP connection. We recommend the use of HTTPS.');
     },
-    methods: {
-      handleNodeDetails(nodeDetails) {
-        if (this.node.id === nodeDetails.id) {
-          this.isNodeDetailsLoading = false;
-          this.nodeDetails = nodeDetails;
-          this.nodeHealthStatus = nodeDetails.health;
-        }
-      },
-      handleMounted() {
-        eventHub.$emit('pollNodeDetails', this.node.id);
-      },
+  },
+  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], {
-          url: 'https://127.0.0.1:3001/',
-        });
-        const vmX = createComponent(mockNode);
-        expect(vmX.showInsecureUrlWarning).toBeFalsy();
-        vmX.$destroy();
+    let vmHttps;
+    let mockNode;
 
+    beforeEach(() => {
+      // Altered mock data for secure URL
+      mockNode = Object.assign({}, mockNodes[0], {
+        url: 'https://127.0.0.1:3001/',
+      });
+      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,