Avoid double counting outside collaborators, preventing deletions (#810)

* Clean up debug logging in diffable.js

Signed-off-by: Kyle Harding <[email protected]>

* Avoid double counting outside collaborators, preventing deletions

The 'direct' affiliation of collaborators includes the 'outside' collaborators,
and in testing when the resulting source contained duplicates the test
function was not deleting collaborators.

Signed-off-by: Kyle Harding <[email protected]>

---------

Signed-off-by: Kyle Harding <[email protected]>

Author: klutchell

lib/plugins/collaborators.js

@@ -15,9 +15,12 @@ module.exports = class Collaborators extends Diffable {
   }
 
   find () {
-    // this.log.debug(`Finding collaborators for { repo: ${this.repo.repo}, owner: ${this.repo.owner}, affiliation: 'direct', 'outside', and 'pending invites' }`)
+    // https://docs.github.com/en/rest/collaborators/collaborators?apiVersion=2022-11-28
+    // 'outside' means all outside collaborators of an organization-owned repository.
+    // 'direct' means all collaborators with permissions to an organization-owned repository, regardless of organization membership status. (includes outside collaborators)
+    // 'all' means all collaborators the authenticated user can see.
+    // We are using 'direct' to avoid double listing users outside collaborators and team members.
     return Promise.all([this.github.repos.listCollaborators({ repo: this.repo.repo, owner: this.repo.owner, affiliation: 'direct' }),
-      this.github.repos.listCollaborators({ repo: this.repo.repo, owner: this.repo.owner, affiliation: 'outside' }),
       this.github.repos.listInvitations({ repo: this.repo.repo, owner: this.repo.owner })])
       .then(res => {
         const mapCollaborator = user => {
@@ -31,9 +34,8 @@ module.exports = class Collaborators extends Diffable {
           }
         }
 
-        const results0 = (res[0].data || []).map(mapCollaborator)
-        const results1 = (res[1].data || []).map(mapCollaborator)
-        const results2 = (res[2].data || []).map(invite => {
+        const results1 = (res[0].data || []).map(mapCollaborator)
+        const results2 = (res[1].data || []).map(invite => {
           return {
           // Force all usernames to lowercase to avoid comparison issues.
             username: invite.invitee.login.toLowerCase(),
@@ -44,7 +46,7 @@ module.exports = class Collaborators extends Diffable {
             (invite.permissions === 'write' && 'push')
           }
         })
-        return results0.concat(results1).concat(results2)
+        return results1.concat(results2)
       })
       .catch(e => {
         this.logError(e)

lib/plugins/diffable.js

@@ -90,9 +90,9 @@ module.exports = class Diffable extends ErrorStash {
         const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields)
         const compare = mergeDeep.compareDeep(existingRecords, filteredEntries)
         const results = { msg: 'Changes found', additions: compare.additions, modifications: compare.modifications, deletions: compare.deletions }
-        this.log.debug(`Results of comparing ${this.constructor.name} diffable target ${JSON.stringify(existingRecords)} with source ${JSON.stringify(filteredEntries)} is ${results}`)
+        this.log.debug(`Results of comparing ${this.constructor.name} diffable target ${JSON.stringify(existingRecords)} with source ${JSON.stringify(filteredEntries)} is ${JSON.stringify(results)}`)
         if (!compare.hasChanges) {
-          this.log.debug(`There are no changes for ${this.constructor.name} for repo ${this.repo}. Skipping changes`)
+          this.log.debug(`There are no changes for ${this.constructor.name} for repo ${this.repo.repo}. Skipping changes`)
           return Promise.resolve()
         } else {
           if (this.nop) {