Address reviewer comments

- fix empty spaces
- simplify helpers
- add more tests

app/assets/javascripts/import_entities/import_groups/components/import_actions_cell.vue

@@ -27,7 +27,6 @@ export default {
         ? `${this.group.last_import_target.target_namespace}/${this.group.last_import_target.new_name}`
         : null;
     },
-
     absoluteLastImportPath() {
       return joinPaths(gon.relative_url_root || '/', this.fullLastImportPath);
     },
@@ -46,16 +45,6 @@ export default {
 
 <template>
   <span class="gl-white-space-nowrap gl-inline-flex gl-align-items-center">
-    <gl-icon
-      v-if="isFinished"
-      v-gl-tooltip
-      :size="16"
-      name="information-o"
-      :title="
-        s__('BulkImports|Re-import creates a new group. It does not sync with the existing group.')
-      "
-      class="gl-mr-3"
-    />
     <gl-button
       v-if="isAvailableForImport"
       :disabled="isInvalid"
@@ -66,5 +55,15 @@ export default {
     >
       {{ isFinished ? __('Re-import') : __('Import') }}
     </gl-button>
+    <gl-icon
+      v-if="isFinished"
+      v-gl-tooltip
+      :size="16"
+      name="information-o"
+      :title="
+        s__('BulkImports|Re-import creates a new group. It does not sync with the existing group.')
+      "
+      class="gl-ml-3"
+    />
   </span>
 </template>

app/assets/javascripts/import_entities/import_groups/components/import_source_cell.vue

@@ -21,7 +21,6 @@ export default {
         ? `${this.group.last_import_target.target_namespace}/${this.group.last_import_target.new_name}`
         : null;
     },
-
     absoluteLastImportPath() {
       return joinPaths(gon.relative_url_root || '/', this.fullLastImportPath);
     },

app/assets/javascripts/import_entities/import_groups/components/import_table.vue

@@ -189,6 +189,24 @@ export default {
   },
 
   methods: {
+    isUnselectable(group) {
+      return !this.isAvailableForImport(group) || this.isInvalid(group);
+    },
+
+    rowClasses(group) {
+      const DEFAULT_CLASSES = [
+        'gl-border-gray-200',
+        'gl-border-0',
+        'gl-border-b-1',
+        'gl-border-solid',
+      ];
+      const result = [...DEFAULT_CLASSES];
+      if (this.isUnselectable(group)) {
+        result.push('gl-cursor-default!');
+      }
+      return result;
+    },
+
     qaRowAttributes(group, type) {
       if (type === 'row') {
         return {
@@ -250,10 +268,7 @@ export default {
 
       const table = this.getTableRef();
       this.groups.forEach((group, idx) => {
-        if (
-          table.isRowSelected(idx) &&
-          (!this.isAvailableForImport(group) || this.isInvalid(group))
-        ) {
+        if (table.isRowSelected(idx) && this.isUnselectable(group)) {
           table.unselectRow(idx);
         }
       });
@@ -338,7 +353,7 @@ export default {
           ref="table"
           class="gl-w-full"
           data-qa-selector="import_table"
-          tbody-tr-class="gl-border-gray-200 gl-border-0 gl-border-b-1 gl-border-solid"
+          :tbody-tr-class="rowClasses"
           :tbody-tr-attr="qaRowAttributes"
           :items="groups"
           :fields="$options.fields"

app/assets/javascripts/import_entities/import_groups/graphql/client_factory.js

@@ -239,8 +239,8 @@ export function createResolvers({ endpoints, sourceUrl, GroupsManager = SourceGr
         });
       },
 
-      async updateImportStatus(_, { id, status }, { client, getCacheKey }) {
-        groupsManager.updateImportProgress(id, status);
+      async updateImportStatus(_, { id, status: newStatus }, { client, getCacheKey }) {
+        groupsManager.updateImportProgress(id, newStatus);
 
         const progressItem = client.readFragment({
           fragment: bulkImportSourceGroupProgressFragment,
@@ -251,7 +251,9 @@ export function createResolvers({ endpoints, sourceUrl, GroupsManager = SourceGr
           }),
         });
 
-        if (status === STATUSES.FINISHED && progressItem && progressItem.status !== status) {
+        const isInProgress = Boolean(progressItem);
+        const { status: currentStatus } = progressItem ?? {};
+        if (newStatus === STATUSES.FINISHED && isInProgress && currentStatus !== newStatus) {
           const groups = groupsManager.getImportedGroupsByJobId(id);
 
           groups.forEach(async ({ id: groupId, importTarget }) => {
@@ -269,7 +271,7 @@ export function createResolvers({ endpoints, sourceUrl, GroupsManager = SourceGr
         return {
           __typename: clientTypenames.BulkImportProgress,
           id,
-          status,
+          status: newStatus,
         };
       },
 

app/assets/javascripts/import_entities/import_groups/graphql/services/source_groups_manager.js

@@ -38,10 +38,7 @@ export class SourceGroupsManager {
     this.importStates[importId] = {
       status: jobConfig.status,
       groups: jobConfig.groups.map((g) => ({
-        importTarget: {
-          target_namespace: g.import_target.target_namespace,
-          new_name: g.import_target.new_name,
-        },
+        importTarget: { ...g.import_target },
         id: g.id,
       })),
     };

spec/frontend/import_entities/import_groups/components/import_actions_cell_spec.js

-import { GlButton } from '@gitlab/ui';
+import { GlButton, GlIcon } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import { STATUSES } from '~/import_entities/constants';
 import ImportActionsCell from '~/import_entities/import_groups/components/import_actions_cell.vue';
@@ -20,23 +20,42 @@ describe('import actions cell', () => {
     wrapper.destroy();
   });
 
-  it('renders import button when group status is NONE', () => {
-    const group = generateFakeEntry({ id: 1, status: STATUSES.NONE });
-    createComponent({ group });
+  describe('when import status is NONE', () => {
+    beforeEach(() => {
+      const group = generateFakeEntry({ id: 1, status: STATUSES.NONE });
+      createComponent({ group });
+    });
 
-    const button = wrapper.findComponent(GlButton);
+    it('renders import button', () => {
+      const button = wrapper.findComponent(GlButton);
+      expect(button.exists()).toBe(true);
+      expect(button.text()).toBe('Import');
+    });
 
-    expect(button.exists()).toBe(true);
-    expect(button.text()).toBe('Import');
+    it('does not render icon with a hint', () => {
+      expect(wrapper.findComponent(GlIcon).exists()).toBe(false);
+    });
   });
 
-  it('renders re-import button when group status is FINISHED', () => {
-    const group = generateFakeEntry({ id: 1, status: STATUSES.FINISHED });
-    createComponent({ group });
+  describe('when import status is FINISHED', () => {
+    beforeEach(() => {
+      const group = generateFakeEntry({ id: 1, status: STATUSES.FINISHED });
+      createComponent({ group });
+    });
 
-    const button = wrapper.findComponent(GlButton);
-    expect(button.exists()).toBe(true);
-    expect(button.text()).toBe('Re-import');
+    it('renders re-import button', () => {
+      const button = wrapper.findComponent(GlButton);
+      expect(button.exists()).toBe(true);
+      expect(button.text()).toBe('Re-import');
+    });
+
+    it('renders icon with a hint', () => {
+      const icon = wrapper.findComponent(GlIcon);
+      expect(icon.exists()).toBe(true);
+      expect(icon.attributes().title).toBe(
+        'Re-import creates a new group. It does not sync with the existing group.',
+      );
+    });
   });
 
   it('does not render import button when group import is in progress', () => {
@@ -47,6 +66,18 @@ describe('import actions cell', () => {
     expect(button.exists()).toBe(false);
   });
 
+  it('renders import button as disabled when there are validation errors', () => {
+    const group = generateFakeEntry({
+      id: 1,
+      status: STATUSES.NONE,
+      validation_errors: [{ field: 'new_name', message: 'something ' }],
+    });
+    createComponent({ group });
+
+    const button = wrapper.findComponent(GlButton);
+    expect(button.props().disabled).toBe(true);
+  });
+
   it('emits import-group event when import button is clicked', () => {
     const group = generateFakeEntry({ id: 1, status: STATUSES.NONE });
     createComponent({ group });

spec/frontend/import_entities/import_groups/graphql/client_factory_spec.js

@@ -440,9 +440,10 @@ describe('Bulk import resolvers', () => {
         },
       });
 
-      expect(lastImportTarget).toMatchObject(IMPORT_TARGET);
+      expect(lastImportTarget).toStrictEqual(IMPORT_TARGET);
 
-      expect(progress).toMatchObject({
+      expect(progress).toStrictEqual({
+        __typename: clientTypenames.BulkImportProgress,
         id: FAKE_JOB_ID,
         status: NEW_STATUS,
       });
@@ -458,7 +459,8 @@ describe('Bulk import resolvers', () => {
         variables: { id: FAKE_JOB_ID, status: NEW_STATUS },
       });
 
-      expect(statusInResponse).toMatchObject({
+      expect(statusInResponse).toStrictEqual({
+        __typename: clientTypenames.BulkImportProgress,
         id: FAKE_JOB_ID,
         status: NEW_STATUS,
       });
@@ -476,7 +478,13 @@ describe('Bulk import resolvers', () => {
         variables: { sourceGroupId: GROUP_ID, field: FAKE_FIELD, message: FAKE_MESSAGE },
       });
 
-      expect(validationErrors).toMatchObject([{ field: FAKE_FIELD, message: FAKE_MESSAGE }]);
+      expect(validationErrors).toStrictEqual([
+        {
+          __typename: clientTypenames.BulkImportValidationError,
+          field: FAKE_FIELD,
+          message: FAKE_MESSAGE,
+        },
+      ]);
     });
 
     it('removeValidationError removes error from group', async () => {
@@ -497,7 +505,7 @@ describe('Bulk import resolvers', () => {
         variables: { sourceGroupId: GROUP_ID, field: FAKE_FIELD },
       });
 
-      expect(validationErrors).toMatchObject([]);
+      expect(validationErrors).toStrictEqual([]);
     });
   });
 });