fix(branches): prevent duplicate nop entries for multiple branches

lib/plugins/branches.js

@@ -30,12 +30,12 @@ module.exports = class Branches extends ErrorStash {
   }
 
   sync () {
-    const resArray = []
     return this.github.rest.repos.get(this.repo).then((currentRepo) => {
       return Promise.all(
         this.branches
           .filter(branch => branch.protection !== undefined)
           .map(branch => {
+            const branchResults = []
             // If branch protection is empty
             if (this.isEmpty(branch.protection)) {
               let p = Object.assign(this.repo, { branch: branch.name })
@@ -46,10 +46,10 @@ module.exports = class Branches extends ErrorStash {
               // Hack to handle closures and keep params from changing
               const params = Object.assign({}, p)
               if (this.nop) {
-                resArray.push(
+                branchResults.push(
                   new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.deleteBranchProtection.endpoint(params), 'Delete Branch Protection')
                 )
-                return Promise.resolve(resArray)
+                return Promise.resolve(branchResults)
               }
 
               return this.github.rest.repos.deleteBranchProtection(params).catch(e => { return [] })
@@ -71,38 +71,38 @@ module.exports = class Branches extends ErrorStash {
                 if (!changes.hasChanges) {
                   this.log.debug(`There are no changes for branch ${JSON.stringify(params)}. Skipping branch protection changes`)
                   if (this.nop) {
-                    return Promise.resolve(resArray)
+                    return Promise.resolve(branchResults)
                   }
                   return Promise.resolve()
                 }
 
                 this.log.debug(`There are changes for branch ${JSON.stringify(params)}\n ${JSON.stringify(changes)} \n Branch protection will be applied`)
                 if (this.nop) {
-                  resArray.push(new NopCommand(this.constructor.name, this.repo, null, results))
+                  branchResults.push(new NopCommand(this.constructor.name, this.repo, null, results))
                 }
 
                 Object.assign(params, requiredBranchProtectionDefaults, this.reformatAndReturnBranchProtection(structuredClone(result.data)), Overrides.removeOverrides(overrides, branch.protection, result.data), { headers: previewHeaders })
 
                 if (this.nop) {
-                  resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection'))
-                  return Promise.resolve(resArray)
+                  branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection'))
+                  return Promise.resolve(branchResults)
                 }
                 this.log.debug(`Adding branch protection ${JSON.stringify(params)}`)
                 return this.github.rest.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
               }).catch((e) => {
                 if (e.status === 404) {
                   Object.assign(params, requiredBranchProtectionDefaults, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders })
                   if (this.nop) {
-                    resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection'))
-                    return Promise.resolve(resArray)
+                    branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection'))
+                    return Promise.resolve(branchResults)
                   }
                   this.log.debug(`Adding branch protection ${JSON.stringify(params)}`)
                   return this.github.rest.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] })
                 } else {
                   this.logError(e)
                   if (this.nop) {
-                    resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR'))
-                    return Promise.resolve(resArray)
+                    branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR'))
+                    return Promise.resolve(branchResults)
                   }
                 }
               })

lib/settings.js

@@ -196,11 +196,20 @@ class Settings {
       return
     }
 
-    // remove duplicate rows in this.results
-    this.results = this.results.filter((thing, index, self) => {
-      return index === self.findIndex((t) => {
-        return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin
-      })
+    // Remove exact duplicate rows from this.results. Comparing only on
+    // type/repo/plugin would also drop legitimately distinct entries, e.g. the
+    // planned protection changes for two branches of the same repo.
+    const seenResults = new Set()
+    this.results = this.results.filter(res => {
+      if (!res) {
+        return true
+      }
+      const key = JSON.stringify([res.type, res.repo, res.plugin, res.endpoint, res.body, res.action])
+      if (seenResults.has(key)) {
+        return false
+      }
+      seenResults.add(key)
+      return true
     })
 
     let error = false

test/unit/lib/plugins/branches.test.js

@@ -363,6 +363,39 @@ describe('Branches', () => {
     })
   })
 
+  describe('in nop mode', () => {
+    function configureNop (config) {
+      return new Branches(true, github, { owner: 'bkeepers', repo: 'test' }, config, log, [])
+    }
+
+    beforeEach(() => {
+      github.rest.repos.updateBranchProtection.endpoint = jest.fn().mockImplementation(params => {
+        return { url: 'updateBranchProtection', body: params }
+      })
+      github.rest.repos.deleteBranchProtection.endpoint = jest.fn().mockImplementation(params => {
+        return { url: 'deleteBranchProtection', body: params }
+      })
+    })
+
+    describe('when multiple branches have changes', () => {
+      it('reports each planned change exactly once', () => {
+        const plugin = configureNop(
+          [
+            { name: 'master', protection: { enforce_admins: true } },
+            { name: 'develop', protection: { enforce_admins: true } }
+          ]
+        )
+
+        return plugin.sync().then(res => {
+          const commands = res.filter(Boolean)
+          // Each branch produces two NopCommands: the diff record and the
+          // endpoint command. Two branches must yield four entries, not eight.
+          expect(commands.length).toBe(4)
+        })
+      })
+    })
+  })
+
   describe.skip('return values', () => {
     it('returns updateBranchProtection Promise', () => {
       const plugin = configure(

test/unit/lib/settings.test.js

@@ -549,4 +549,38 @@ repository:
       expect(mockRepoSync).toHaveBeenCalledTimes(1)
     })
   }) // updateRepos - archived repo skipping
+
+  describe('handleResults', () => {
+    it('removes exact duplicates but keeps distinct entries for the same repo and plugin', async () => {
+      stubContext.octokit.rest.checks = { update: jest.fn().mockResolvedValue({}) }
+      stubContext.octokit.rest.issues = { createComment: jest.fn().mockResolvedValue({}) }
+      stubContext.payload.check_run = { id: 42, check_suite: { pull_requests: [{ number: 1 }] } }
+      stubContext.payload.repository = { owner: { login: 'test' }, name: 'test-repo' }
+      const settings = new Settings(true, stubContext, mockRepo, {}, mockRef)
+
+      const masterDiff = {
+        type: 'INFO',
+        plugin: 'Branches',
+        repo: 'test/test-repo',
+        endpoint: '',
+        body: '',
+        action: { msg: 'Changes for master', additions: {}, modifications: { a: 1 }, deletions: {} }
+      }
+      const developDiff = {
+        type: 'INFO',
+        plugin: 'Branches',
+        repo: 'test/test-repo',
+        endpoint: '',
+        body: '',
+        action: { msg: 'Changes for develop', additions: {}, modifications: { b: 2 }, deletions: {} }
+      }
+      settings.results = [masterDiff, { ...masterDiff }, developDiff]
+
+      await settings.handleResults()
+
+      // the literal duplicate goes, the distinct same-plugin same-repo entry stays
+      expect(settings.results).toHaveLength(2)
+      expect(settings.results.map(res => res.action.msg)).toEqual(['Changes for master', 'Changes for develop'])
+    })
+  }) // handleResults
 }) // Settings Tests