fix: handle error when branch already exists (#48)

* refactor

* Update createBranch method

* Tweak

* Add tests

* wip

* wip

* lint

Author: sinchang

src/tasks/processIssueComment/OptionsConfig/index.js

@@ -2,10 +2,11 @@ const ALL_CONTRIBUTORS_RC = '.all-contributorsrc'
 
 const { addContributorWithDetails } = require('all-contributors-cli')
 
+const { AllContributorBotError } = require('../utils/errors')
+
 class OptionsConfig {
-    constructor({ repository, commentReply }) {
+    constructor({ repository }) {
         this.repository = repository
-        this.commentReply = commentReply
         this.options
         this.originalOptionsSha
     }
@@ -22,12 +23,11 @@ class OptionsConfig {
             return optionsConfig
         } catch (error) {
             if (error instanceof SyntaxError) {
-                this.commentReply.reply(
+                throw new AllContributorBotError(
                     `This project's configuration file has malformed JSON: ${ALL_CONTRIBUTORS_RC}. Error:: ${
                         error.message
                     }`,
                 )
-                error.handled = true
             }
             throw error
         }

src/tasks/processIssueComment/Repository/index.js

@@ -1,40 +1,48 @@
-const { ResourceNotFoundError } = require('../utils/errors')
+const {
+    BranchNotFoundError,
+    ResourceNotFoundError,
+} = require('../utils/errors')
 
 class Repository {
-    constructor({ repo, owner, github }) {
+    constructor({ repo, owner, github, defaultBranch }) {
         this.github = github
         this.repo = repo
         this.owner = owner
+        this.defaultBranch = defaultBranch
+        this.baseBranch = defaultBranch
     }
 
     getFullname() {
         return `${this.owner}/${this.repo}`
     }
 
+    setBaseBranch(branchName) {
+        this.baseBranch = branchName
+    }
+
     async getFile(filePath) {
-        // https://octokit.github.io/rest.js/#api-Repos-getContents
-        let file
         try {
-            file = await this.github.repos.getContents({
+            // https://octokit.github.io/rest.js/#api-Repos-getContents
+            const file = await this.github.repos.getContents({
                 owner: this.owner,
                 repo: this.repo,
                 path: filePath,
+                ref: this.baseBranch,
             })
+            // Contents can be an array if its a directory, should be an edge case, and we can just crash
+            const contentBinary = file.data.content
+            const content = Buffer.from(contentBinary, 'base64').toString()
+            return {
+                content,
+                sha: file.data.sha,
+            }
         } catch (error) {
             if (error.status === 404) {
                 throw new ResourceNotFoundError(filePath, this.full_name)
             } else {
                 throw error
             }
         }
-
-        // Contents can be an array if its a directory, should be an edge case, and we can just crash
-        const contentBinary = file.data.content
-        const content = Buffer.from(contentBinary, 'base64').toString()
-        return {
-            content,
-            sha: file.data.sha,
-        }
     }
 
     async getMultipleFiles(filePathsArray) {
@@ -61,17 +69,23 @@ class Repository {
         return multipleFilesByPath
     }
 
-    async getHeadRef(defaultBranch) {
-        const result = await this.github.git.getRef({
-            owner: this.owner,
-            repo: this.repo,
-            ref: `heads/${defaultBranch}`,
-        })
-        return result.data.object.sha
+    async getRef(branchName) {
+        try {
+            const result = await this.github.git.getRef({
+                owner: this.owner,
+                repo: this.repo,
+                ref: `heads/${branchName}`,
+            })
+            return result.data.object.sha
+        } catch (error) {
+            if (error.status === 404) {
+                throw new BranchNotFoundError(branchName)
+            }
+        }
     }
 
-    async createBranch({ branchName, defaultBranch }) {
-        const fromSha = await this.getHeadRef(defaultBranch)
+    async createBranch(branchName) {
+        const fromSha = await this.getRef(this.defaultBranch)
 
         // https://octokit.github.io/rest.js/#api-Git-createRef
         await this.github.git.createRef({
@@ -140,27 +154,33 @@ class Repository {
         await Promise.all(createOrUpdateFilesMultiple)
     }
 
-    async createPullRequest({ title, body, branchName, defaultBranch }) {
-        const result = await this.github.pulls.create({
-            owner: this.owner,
-            repo: this.repo,
-            title,
-            body,
-            head: branchName,
-            base: defaultBranch,
-            maintainer_can_modify: true,
-        })
-        return result.data.html_url
+    async createPullRequest({ title, body, branchName }) {
+        try {
+            const result = await this.github.pulls.create({
+                owner: this.owner,
+                repo: this.repo,
+                title,
+                body,
+                head: branchName,
+                base: this.defaultBranch,
+                maintainer_can_modify: true,
+            })
+            return result.data.html_url
+        } catch (error) {
+            if (error.status === 422) {
+                console.log('Pull request already open') // eslint-disable-line no-console
+                return error.data.html_url
+            } else {
+                throw error
+            }
+        }
     }
 
-    async createPullRequestFromFiles({
-        title,
-        body,
-        filesByPath,
-        branchName,
-        defaultBranch,
-    }) {
-        await this.createBranch({ branchName, defaultBranch })
+    async createPullRequestFromFiles({ title, body, filesByPath, branchName }) {
+        const branchNameExists = branchName === this.baseBranch
+        if (!branchNameExists) {
+            await this.createBranch(branchName)
+        }
 
         await this.createOrUpdateFiles({
             filesByPath,
@@ -171,9 +191,7 @@ class Repository {
             title,
             body,
             branchName,
-            defaultBranch,
         })
-
         return pullRequestURL
     }
 }

src/tasks/processIssueComment/probot-processIssueComment.js

@@ -8,6 +8,7 @@ const parseComment = require('./utils/parse-comment')
 
 const {
     AllContributorBotError,
+    BranchNotFoundError,
     ResourceNotFoundError,
 } = require('./utils/errors')
 
@@ -20,7 +21,7 @@ async function processAddContributor({
     optionsConfig,
     who,
     contributions,
-    defaultBranch,
+    branchName,
 }) {
     const { name, avatar_url, profile } = await getUserDetails({
         github: context.github,
@@ -49,31 +50,52 @@ async function processAddContributor({
         originalSha: optionsConfig.getOriginalSha(),
     }
 
-    const safeWho = getSafeRef(who)
     const pullRequestURL = await repository.createPullRequestFromFiles({
         title: `docs: add ${who} as a contributor`,
         body: `Adds @${who} as a contributor for ${contributions.join(
             ', ',
         )}.\n\nThis was requested by ${commentReply.replyingToWho()} [in this comment](${commentReply.replyingToWhere()})`,
         filesByPath: filesByPathToUpdate,
-        branchName: `all-contributors/add-${safeWho}`,
-        defaultBranch,
+        branchName,
     })
 
     commentReply.reply(
         `I've put up [a pull request](${pullRequestURL}) to add @${who}! :tada:`,
     )
 }
 
-async function probotProcessIssueComment({ context, commentReply }) {
+async function setupRepository({ context, branchName }) {
+    const defaultBranch = context.payload.repository.default_branch
     const repository = new Repository({
         ...context.repo(),
         github: context.github,
+        defaultBranch,
     })
+
+    try {
+        await repository.getRef(branchName)
+        context.log.info(
+            `Branch: ${branchName} EXISTS, will work from this branch`,
+        )
+        repository.setBaseBranch(branchName)
+    } catch (error) {
+        if (error instanceof BranchNotFoundError) {
+            context.log.info(
+                `Branch: ${branchName} DOES NOT EXIST, will work from default branch`,
+            )
+        } else {
+            throw error
+        }
+    }
+
+    return repository
+}
+
+async function setupOptionsConfig({ repository }) {
     const optionsConfig = new OptionsConfig({
         repository,
-        commentReply,
     })
+
     try {
         await optionsConfig.fetch()
     } catch (error) {
@@ -84,18 +106,31 @@ async function probotProcessIssueComment({ context, commentReply }) {
         }
     }
 
+    return optionsConfig
+}
+
+async function probotProcessIssueComment({ context, commentReply }) {
     const commentBody = context.payload.comment.body
-    const parsedComment = parseComment(commentBody)
+    const { who, action, contributions } = parseComment(commentBody)
+
+    if (action === 'add') {
+        const safeWho = getSafeRef(who)
+        const branchName = `all-contributors/add-${safeWho}`
+
+        const repository = await setupRepository({
+            context,
+            branchName,
+        })
+        const optionsConfig = await setupOptionsConfig({ repository })
 
-    if (parsedComment.action === 'add') {
         await processAddContributor({
             context,
             commentReply,
             repository,
             optionsConfig,
-            who: parsedComment.who,
-            contributions: parsedComment.contributions,
-            defaultBranch: context.payload.repository.default_branch,
+            who,
+            contributions,
+            branchName,
         })
         return
     }
@@ -105,7 +140,7 @@ async function probotProcessIssueComment({ context, commentReply }) {
         `Basic usage: @all-contributors please add @jakebolam for code, doc and infra`,
     )
     commentReply.reply(
-        `For other usage see the [documentation](https://github.com/all-contributors/all-contributors-bot#usage)`,
+        `For other usages see the [documentation](https://github.com/all-contributors/all-contributors-bot#usage)`,
     )
     return
 }
@@ -115,9 +150,7 @@ async function probotProcessIssueCommentSafe({ context }) {
     try {
         await probotProcessIssueComment({ context, commentReply })
     } catch (error) {
-        if (error.handled) {
-            context.log.debug(error)
-        } else if (error instanceof AllContributorBotError) {
+        if (error instanceof AllContributorBotError) {
             context.log.info(error)
             commentReply.reply(error.message)
         } else {

src/tasks/processIssueComment/utils/errors.js

@@ -14,6 +14,13 @@ class ResourceNotFoundError extends AllContributorBotError {
     }
 }
 
+class BranchNotFoundError extends AllContributorBotError {
+    constructor(branchName) {
+        super(`${branchName} does not exist`)
+        this.name = this.constructor.name
+    }
+}
+
 class UserNotFoundError extends AllContributorBotError {
     constructor(username) {
         super(`Could not find the user ${username} on github.`)
@@ -23,6 +30,7 @@ class UserNotFoundError extends AllContributorBotError {
 
 module.exports = {
     AllContributorBotError,
+    BranchNotFoundError,
     ResourceNotFoundError,
     UserNotFoundError,
 }

test/tasks/processIssueComment/OptionsConfig/index.test.js

@@ -5,12 +5,10 @@ describe('ContentFiles', () => {
         repo: 'all-contributors-bot-test',
         owner: 'all-contributors',
     }
-    const mockCommentReply = {}
 
     test(`Add's new contributor`, async () => {
         const optionsConfig = new OptionsConfig({
             repository: mockRepository,
-            commentReply: mockCommentReply,
         })
 
         optionsConfig.options = {
@@ -40,7 +38,6 @@ describe('ContentFiles', () => {
     test(`Add's new contributions for contributor`, async () => {
         const optionsConfig = new OptionsConfig({
             repository: mockRepository,
-            commentReply: mockCommentReply,
         })
 
         optionsConfig.options = {
@@ -81,7 +78,6 @@ describe('ContentFiles', () => {
     test(`If profile URL is missing protocol, add it for them`, async () => {
         const optionsConfig = new OptionsConfig({
             repository: mockRepository,
-            commentReply: mockCommentReply,
         })
         optionsConfig.init()
 
@@ -108,7 +104,6 @@ describe('ContentFiles', () => {
     test(`Inits the contributor file`, () => {
         const optionsConfig = new OptionsConfig({
             repository: mockRepository,
-            commentReply: mockCommentReply,
         })
 
         expect(optionsConfig.options).toBeUndefined()

test/tasks/processIssueComment/Repository/index.test.js

@@ -13,6 +13,7 @@ describe('Repository', () => {
         repo: 'all-contributors-bot',
         owner: 'all-contributors',
         github: mockGithub,
+        defaultBranch: 'master',
     })
 
     const verifyBody = body => {
@@ -95,7 +96,6 @@ describe('Repository', () => {
                 },
             },
             branchName: 'all-contributors/add-jakebolam',
-            defaultBranch: 'master',
         })
         expect(pullRequestNumber).toEqual(
             'https://github.com/all-contributors/all-contributors-bot/pull/1347',