refactor: fix a layer violation causing a circular dependency (#192)

Author: kormide

e2e/e2e.spec.ts

@@ -729,7 +729,7 @@ describe("e2e tests", () => {
     });
 
     fakeGitHub.mockUser({
-      login: GitHubClient.GITHUB_ACTIONS_BOT.username,
+      login: GitHubClient.GITHUB_ACTIONS_BOT.login,
       id: GitHubClient.GITHUB_ACTIONS_BOT.id,
     });
     fakeGitHub.mockUser({ login: "publish-to-bcr-bot[bot]", id: 123 });

src/application/notifications.ts

@@ -1,10 +1,9 @@
-import { Inject, Injectable } from "@nestjs/common";
+import { Injectable } from "@nestjs/common";
 import { UserFacingError } from "../domain/error.js";
 import { Maintainer } from "../domain/metadata-file.js";
 import { Repository } from "../domain/repository.js";
-import { User } from "../domain/user.js";
+import { User, UserService } from "../domain/user.js";
 import { Authentication, EmailClient } from "../infrastructure/email.js";
-import { GitHubClient } from "../infrastructure/github.js";
 import { SecretsClient } from "../infrastructure/secrets.js";
 
 @Injectable()
@@ -16,7 +15,7 @@ export class NotificationsService {
   constructor(
     private readonly emailClient: EmailClient,
     private readonly secretsClient: SecretsClient,
-    @Inject("rulesetRepoGitHubClient") private githubClient: GitHubClient
+    private readonly userService: UserService
   ) {
     if (process.env.NOTIFICATIONS_EMAIL === undefined) {
       throw new Error("Missing NOTIFICATIONS_EMAIL environment variable.");
@@ -70,7 +69,7 @@ export class NotificationsService {
     const fetchedEmails = (
       await Promise.all(
         maintainersWithOnlyGithubHandle.map((m) =>
-          this.githubClient.getRepoUser(m.github, rulesetRepo)
+          this.userService.getUser(m.github)
         )
       )
     )

src/application/release-event-handler.ts

@@ -11,7 +11,7 @@ import {
   RulesetRepoError,
   RulesetRepository,
 } from "../domain/ruleset-repository.js";
-import { User } from "../domain/user.js";
+import { User, UserService } from "../domain/user.js";
 import { GitHubClient } from "../infrastructure/github.js";
 import { NotificationsService } from "./notifications.js";
 
@@ -24,9 +24,8 @@ interface PublishAttempt {
 @Injectable()
 export class ReleaseEventHandler {
   constructor(
-    @Inject("rulesetRepoGitHubClient")
-    private rulesetRepoGitHubClient: GitHubClient,
     @Inject("appOctokit") private appOctokit: Octokit,
+    private readonly userService: UserService,
     private readonly findRegistryForkService: FindRegistryForkService,
     private readonly createEntryService: CreateEntryService,
     private readonly publishEntryService: PublishEntryService,
@@ -40,10 +39,7 @@ export class ReleaseEventHandler {
         process.env.BAZEL_CENTRAL_REGISTRY
       );
 
-      let releaser = await this.rulesetRepoGitHubClient.getRepoUser(
-        event.payload.sender.login,
-        repository
-      );
+      let releaser = await this.userService.getUser(event.payload.sender.login);
       const releaseUrl = event.payload.release.html_url;
 
       const tag = event.payload.release.tag_name;
@@ -117,7 +113,8 @@ export class ReleaseEventHandler {
       for (let bcrFork of candidateBcrForks) {
         const bcrForkGitHubClient = await GitHubClient.forRepoInstallation(
           this.appOctokit,
-          bcrFork
+          bcrFork.owner,
+          bcrFork.name
         );
 
         const attempt = await this.attemptPublish(
@@ -275,9 +272,8 @@ export class ReleaseEventHandler {
       );
 
       // Fetch the releaser to get their name
-      const fixedReleaser = await this.rulesetRepoGitHubClient.getRepoUser(
-        rulesetRepo.config.fixedReleaser.login,
-        rulesetRepo
+      const fixedReleaser = await this.userService.getUser(
+        rulesetRepo.config.fixedReleaser.login
       );
 
       return {

src/application/webhook/app.module.ts

@@ -2,6 +2,8 @@ import { Module } from "@nestjs/common";
 import { CreateEntryService } from "../../domain/create-entry.js";
 import { FindRegistryForkService } from "../../domain/find-registry-fork.js";
 import { PublishEntryService } from "../../domain/publish-entry.js";
+import { RepositoryService } from "../../domain/repository.js";
+import { UserService } from "../../domain/user.js";
 import { EmailClient } from "../../infrastructure/email.js";
 import { GitClient } from "../../infrastructure/git.js";
 import { SecretsClient } from "../../infrastructure/secrets.js";
@@ -24,6 +26,8 @@ import {
     CreateEntryService,
     FindRegistryForkService,
     PublishEntryService,
+    RepositoryService,
+    UserService,
     APP_OCTOKIT_PROVIDER,
     BCR_APP_OCTOKIT_PROVIDER,
     RULESET_REPO_GITHUB_CLIENT_PROVIDER,

src/application/webhook/providers.ts

@@ -47,7 +47,8 @@ export const RULESET_REPO_GITHUB_CLIENT_PROVIDER: Provider = {
 
     const githubClient = await GitHubClient.forRepoInstallation(
       appOctokit,
-      rulesetRepo,
+      rulesetRepo.owner,
+      rulesetRepo.name,
       installationId
     );
     return githubClient;
@@ -67,7 +68,8 @@ export const BCR_GITHUB_CLIENT_PROVIDER: Provider = {
 
     const githubClient = await GitHubClient.forRepoInstallation(
       bcrAppOctokit,
-      bcr
+      bcr.owner,
+      bcr.name
     );
     return githubClient;
   },

src/domain/create-entry.spec.ts

@@ -6,7 +6,11 @@ import fs, { PathLike } from "node:fs";
 import os from "node:os";
 import path from "node:path";
 import { GitClient } from "../infrastructure/git";
-import { GitHubApp, GitHubClient } from "../infrastructure/github";
+import {
+  GitHubApp,
+  GitHubClient,
+  User as GitHubUser,
+} from "../infrastructure/github";
 import {
   fakeMetadataFile,
   fakeModuleFile,
@@ -26,7 +30,7 @@ import { ModuleFile } from "./module-file";
 import { ReleaseArchive } from "./release-archive";
 import { Repository } from "./repository";
 import { RulesetRepository } from "./ruleset-repository";
-import { User } from "./user";
+import { User, UserService } from "./user";
 
 let createEntryService: CreateEntryService;
 let mockGitClient: Mocked<GitClient>;
@@ -942,16 +946,18 @@ describe("commitEntryToNewBranch", () => {
     const tag = "v1.2.3";
     const rulesetRepo = await RulesetRepository.create("repo", "owner", tag);
     const bcrRepo = CANONICAL_BCR;
-    const releaser = GitHubClient.GITHUB_ACTIONS_BOT;
-    const botUser: User = {
+    const releaser = UserService.fromGitHubUser(
+      GitHubClient.GITHUB_ACTIONS_BOT
+    );
+    const botUser: Partial<GitHubUser> = {
       name: "publish-to-bcr",
-      username: "publish-to-bcr[bot]",
+      login: "publish-to-bcr[bot]",
       email: `12345+"publish-to-bcr[bot]@users.noreply.github.com`,
     };
     const botApp = { slug: "publish-to-bcr" } as GitHubApp;
 
     mockBcrGitHubClient.getApp.mockResolvedValue(botApp);
-    mockBcrGitHubClient.getBotAppUser.mockResolvedValue(botUser);
+    mockBcrGitHubClient.getBotAppUser.mockResolvedValue(botUser as GitHubUser);
 
     await createEntryService.commitEntryToNewBranch(
       rulesetRepo,
@@ -1105,7 +1111,7 @@ describe("pushEntryToFork", () => {
     );
     expect(
       mockBcrForkGitHubClient.getAuthenticatedRemoteUrl
-    ).toHaveBeenCalledWith(bcrForkRepo);
+    ).toHaveBeenCalledWith(bcrForkRepo.owner, bcrForkRepo.name);
   });
 
   test("adds a remote with the authenticated url for the fork to the local bcr repo", async () => {

src/domain/create-entry.ts

@@ -17,7 +17,7 @@ import { ReleaseArchive } from "./release-archive.js";
 import { Repository } from "./repository.js";
 import { RulesetRepository } from "./ruleset-repository.js";
 import { SourceTemplate } from "./source-template.js";
-import { User } from "./user.js";
+import { User, UserService } from "./user.js";
 
 export class VersionAlreadyPublishedError extends UserFacingError {
   public constructor(version: string) {
@@ -44,7 +44,10 @@ export class CreateEntryService {
     tag: string,
     moduleRoot: string
   ): Promise<{ moduleName: string }> {
-    await Promise.all([rulesetRepo.shallowCloneAndCheckout(tag), bcrRepo.shallowCloneAndCheckout("main")]);
+    await Promise.all([
+      rulesetRepo.shallowCloneAndCheckout(tag),
+      bcrRepo.shallowCloneAndCheckout("main"),
+    ]);
 
     const version = RulesetRepository.getVersionFromTag(tag);
 
@@ -123,7 +126,7 @@ export class CreateEntryService {
     const branchName = `${repoAndVersion}-${randomBytes(4).toString("hex")}`;
 
     let commitAuthor: Partial<User> = releaser;
-    if (releaser.username === GitHubClient.GITHUB_ACTIONS_BOT.username) {
+    if (UserService.isGitHubActionsBot(releaser)) {
       const botApp = await this.bcrGitHubClient.getApp();
       const botAppUser = await this.bcrGitHubClient.getBotAppUser(botApp);
 
@@ -157,7 +160,8 @@ export class CreateEntryService {
     githubClient: GitHubClient
   ): Promise<void> {
     const authenticatedRemoteUrl = await githubClient.getAuthenticatedRemoteUrl(
-      bcrForkRepo
+      bcrForkRepo.owner,
+      bcrForkRepo.name
     );
 
     if (!(await this.gitClient.hasRemote(bcr.diskPath, "authed-fork"))) {