Fix missing error message on repository selection

The repository selection was structured such that you would get in the
`else` case if there was nothing selected, but this case would also be
used if for some other reason the selected item was not valid.

This restructures the conditions to first check whether the user
cancelled out of the operation and will silently return in that case. In
other cases where it cannot determine the repositories, it will now show
a user-visible error.

Author: koesie10

extensions/ql-vscode/src/remote-queries/repository-selection.ts

@@ -49,15 +49,21 @@ export async function getRepositorySelection(): Promise<RepositorySelection> {
     options,
   );
 
-  if (quickpick?.repositories?.length) {
+  if (!quickpick) {
+    // We don't need to display a warning pop-up in this case, since the user just escaped out of the operation.
+    // We set 'true' to make this a silent exception.
+    throw new UserCancellationException("No repositories selected", true);
+  }
+
+  if (quickpick.repositories?.length) {
     void extLogger.log(
       `Selected repositories: ${quickpick.repositories.join(", ")}`,
     );
     return { repositories: quickpick.repositories };
-  } else if (quickpick?.repositoryList) {
+  } else if (quickpick.repositoryList) {
     void extLogger.log(`Selected repository list: ${quickpick.repositoryList}`);
     return { repositoryLists: [quickpick.repositoryList] };
-  } else if (quickpick?.useCustomRepo) {
+  } else if (quickpick.useCustomRepo) {
     const customRepo = await getCustomRepo();
     if (customRepo === undefined) {
       // The user cancelled, do nothing.
@@ -70,7 +76,7 @@ export async function getRepositorySelection(): Promise<RepositorySelection> {
     }
     void extLogger.log(`Entered repository: ${customRepo}`);
     return { repositories: [customRepo] };
-  } else if (quickpick?.useAllReposOfOwner) {
+  } else if (quickpick.useAllReposOfOwner) {
     const owner = await getOwner();
     if (owner === undefined) {
       // The user cancelled, do nothing.
@@ -82,9 +88,9 @@ export async function getRepositorySelection(): Promise<RepositorySelection> {
     void extLogger.log(`Entered owner: ${owner}`);
     return { owners: [owner] };
   } else {
-    // We don't need to display a warning pop-up in this case, since the user just escaped out of the operation.
-    // We set 'true' to make this a silent exception.
-    throw new UserCancellationException("No repositories selected", true);
+    // This means the user has selected something, but there is nothing actually linked to this item. We want to show
+    // this to the user.
+    throw new UserCancellationException("No repositories selected", false);
   }
 }
 

extensions/ql-vscode/src/vscode-tests/no-workspace/remote-queries/repository-selection.test.ts

@@ -65,6 +65,28 @@ describe("repository selection", () => {
       expect(repoSelection.owners).toBeUndefined();
       expect(repoSelection.repositories).toEqual(["foo/bar", "foo/baz"]);
     });
+
+    it("should return an error for an empty repository list", async () => {
+      // Fake return values
+      quickPickSpy.mockResolvedValue({
+        repositories: [],
+      } as unknown as QuickPickItem);
+      getRemoteRepositoryListsSpy.mockReturnValue({
+        list1: ["foo/bar", "foo/baz"],
+        list2: [],
+      });
+
+      await expect(getRepositorySelection()).rejects.toThrow(
+        "No repositories selected",
+      );
+      await expect(getRepositorySelection()).rejects.toThrow(
+        UserCancellationException,
+      );
+      await expect(getRepositorySelection()).rejects.toHaveProperty(
+        "silent",
+        false,
+      );
+    });
   });
 
   describe("system level repo lists", () => {
@@ -149,6 +171,10 @@ describe("repository selection", () => {
       await expect(getRepositorySelection()).rejects.toThrow(
         UserCancellationException,
       );
+      await expect(getRepositorySelection()).rejects.toHaveProperty(
+        "silent",
+        true,
+      );
     });
   });
 
@@ -219,6 +245,10 @@ describe("repository selection", () => {
       await expect(getRepositorySelection()).rejects.toThrow(
         UserCancellationException,
       );
+      await expect(getRepositorySelection()).rejects.toHaveProperty(
+        "silent",
+        true,
+      );
     });
   });
 
@@ -313,4 +343,20 @@ describe("repository selection", () => {
       expect(repoSelection.repositories).toEqual(["owner3/repo3"]);
     });
   });
+
+  it("should allow the user to cancel", async () => {
+    // Fake return values
+    quickPickSpy.mockResolvedValue(undefined);
+
+    await expect(getRepositorySelection()).rejects.toThrow(
+      "No repositories selected",
+    );
+    await expect(getRepositorySelection()).rejects.toThrow(
+      UserCancellationException,
+    );
+    await expect(getRepositorySelection()).rejects.toHaveProperty(
+      "silent",
+      true,
+    );
+  });
 });