fix: lib/team.js tweaks and tests

- Removes unnecessary colon when listing 0 users/teams
- Removes unimplemented `npm team edit` placeholder
- Adds `test/lib/team.js` tests

- Fixes: https://github.com/npm/statusboard/issues/176

PR-URL: https://github.com/npm/cli/pull/2314
Credit: @ruyadorno
Close: #2314
Reviewed-by: @isaacs

3 files changed, 668 insertions, 8 deletions

diff --git a/lib/team.js b/lib/team.js
index 582dadb3f..54c9c68a3 100644
--- a/lib/team.js
+++ b/lib/team.js
@@ -1,11 +1,14 @@
+'use strict'
+
 const columns = require('cli-columns')
 const libteam = require('libnpmteam')
+
 const npm = require('./npm.js')
 const output = require('./utils/output.js')
 const otplease = require('./utils/otplease.js')
 const usageUtil = require('./utils/usage')
 
-const subcommands = ['create', 'destroy', 'add', 'rm', 'ls', 'edit']
+const subcommands = ['create', 'destroy', 'add', 'rm', 'ls']
 
 const usage = usageUtil(
   'team',
@@ -13,8 +16,7 @@ const usage = usageUtil(
   'npm team destroy <scope:team> [--otp <otpcode>]\n' +
   'npm team add <scope:team> <user> [--otp <otpcode>]\n' +
   'npm team rm <scope:team> <user> [--otp <otpcode>]\n' +
-  'npm team ls <scope>|<scope:team>\n' +
-  'npm team edit <scope:team>'
+  'npm team ls <scope>|<scope:team>\n'
 )
 
 const completion = (opts, cb) => {
@@ -28,7 +30,6 @@ const completion = (opts, cb) => {
     case 'destroy':
     case 'add':
     case 'rm':
-    case 'edit':
       return cb(null, [])
     default:
       return cb(new Error(argv[2] + ' not recognized'))
@@ -56,8 +57,6 @@ const team = async ([cmd, entity = '', user = '']) => {
         else
           return teamListTeams(entity, opts)
       }
-      case 'edit':
-        throw new Error('`npm team edit` is not implemented yet.')
       default:
         throw usage
     }
@@ -125,7 +124,9 @@ const teamListUsers = async (entity, opts) => {
   else if (opts.parseable)
     output(users.join('\n'))
   else if (!opts.silent && opts.loglevel !== 'silent') {
-    output(`\n@${entity} has ${users.length} user${users.length === 1 ? '' : 's'}:\n`)
+    const plural = users.length === 1 ? '' : 's'
+    const more = users.length === 0 ? '' : ':\n'
+    output(`\n@${entity} has ${users.length} user${plural}${more}`)
     output(columns(users, { padding: 1 }))
   }
 }
@@ -137,7 +138,9 @@ const teamListTeams = async (entity, opts) => {
   else if (opts.parseable)
     output(teams.join('\n'))
   else if (!opts.silent && opts.loglevel !== 'silent') {
-    output(`\n@${entity} has ${teams.length} team${teams.length === 1 ? '' : 's'}:\n`)
+    const plural = teams.length === 1 ? '' : 's'
+    const more = teams.length === 0 ? '' : ':\n'
+    output(`\n@${entity} has ${teams.length} team${plural}${more}`)
     output(columns(teams.map(t => `@${t}`), { padding: 1 }))
   }
 }
diff --git a/tap-snapshots/test-lib-team.js-TAP.test.js b/tap-snapshots/test-lib-team.js-TAP.test.js
new file mode 100644
index 000000000..73123ee1a
--- /dev/null
+++ b/tap-snapshots/test-lib-team.js-TAP.test.js
@@ -0,0 +1,85 @@
+/* IMPORTANT
+ * This snapshot file is auto-generated, but designed for humans.
+ * It should be checked into source control and tracked carefully.
+ * Re-generate by setting TAP_SNAPSHOT=1 and running tests.
+ * Make sure to inspect the output below.  Do not ignore changes!
+ */
+'use strict'
+exports[`test/lib/team.js TAP team add <scope:team> <user> --parseable > should output success result for parseable add user 1`] = `
+foo	npmcli:developers	added
+`
+
+exports[`test/lib/team.js TAP team add <scope:team> <user> default output > should output success result for add user 1`] = `
+foo added to @npmcli:developers
+`
+
+exports[`test/lib/team.js TAP team create <scope:team> --parseable > should output parseable success result for create team 1`] = `
+npmcli:newteam	created
+`
+
+exports[`test/lib/team.js TAP team create <scope:team> default output > should output success result for create team 1`] = `
++@npmcli:newteam
+`
+
+exports[`test/lib/team.js TAP team destroy <scope:team> --parseable > should output parseable result for destroy team 1`] = `
+npmcli:newteam	deleted
+`
+
+exports[`test/lib/team.js TAP team destroy <scope:team> default output > should output success result for destroy team 1`] = `
+-@npmcli:newteam
+`
+
+exports[`test/lib/team.js TAP team ls <scope:team> --parseable > should list users for a parseable scope:team 1`] = `
+darcyclarke
+isaacs
+nlf
+ruyadorno
+`
+
+exports[`test/lib/team.js TAP team ls <scope:team> default output > should list users for a given scope:team 1`] = `
+
+@npmcli:developers has 4 users:
+darcyclarke isaacs nlf ruyadorno
+`
+
+exports[`test/lib/team.js TAP team ls <scope:team> no users > should list no users for a given scope 1`] = `
+
+@npmcli:developers has 0 users
+`
+
+exports[`test/lib/team.js TAP team ls <scope:team> single user > should list single user for a given scope 1`] = `
+
+@npmcli:developers has 1 user:
+foo
+`
+
+exports[`test/lib/team.js TAP team ls <scope> --parseable > should list teams for a parseable scope 1`] = `
+npmcli:designers
+npmcli:developers
+npmcli:product
+`
+
+exports[`test/lib/team.js TAP team ls <scope> default output > should list teams for a given scope 1`] = `
+
+@npmcli has 3 teams:
+@npmcli:designers @npmcli:developers @npmcli:product
+`
+
+exports[`test/lib/team.js TAP team ls <scope> no teams > should list no teams for a given scope 1`] = `
+
+@npmcli has 0 teams
+`
+
+exports[`test/lib/team.js TAP team ls <scope> single team > should list single team for a given scope 1`] = `
+
+@npmcli has 1 team:
+@npmcli:developers
+`
+
+exports[`test/lib/team.js TAP team rm <scope:team> <user> --parseable > should output parseable result for remove user 1`] = `
+foo	npmcli:newteam	removed
+`
+
+exports[`test/lib/team.js TAP team rm <scope:team> <user> default output > should output success result for remove user 1`] = `
+foo removed from @npmcli:newteam
+`
diff --git a/test/lib/team.js b/test/lib/team.js
new file mode 100644
index 000000000..c534cc832
--- /dev/null
+++ b/test/lib/team.js
@@ -0,0 +1,572 @@
+const t = require('tap')
+const requireInject = require('require-inject')
+
+let result = ''
+const libnpmteam = {
+  async add () {},
+  async create () {},
+  async destroy () {},
+  async lsTeams () {},
+  async lsUsers () {},
+  async rm () {},
+}
+const npm = { flatOptions: {} }
+const mocks = {
+  libnpmteam,
+  'cli-columns': a => a.join(' '),
+  '../../lib/npm.js': npm,
+  '../../lib/utils/output.js': (...msg) => {
+    result += msg.join('\n')
+  },
+  '../../lib/utils/otplease.js': async (opts, fn) => fn(opts),
+  '../../lib/utils/usage.js': () => 'usage instructions',
+}
+
+t.afterEach(cb => {
+  result = ''
+  npm.flatOptions = {}
+  cb()
+})
+
+const team = requireInject('../../lib/team.js', mocks)
+
+t.test('no args', t => {
+  team([], err => {
+    t.match(
+      err,
+      'usage instructions',
+      'should throw usage instructions'
+    )
+    t.end()
+  })
+})
+
+t.test('team add <scope:team> <user>', t => {
+  t.test('default output', t => {
+    team(['add', '@npmcli:developers', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output success result for add user')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['add', '@npmcli:developers', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(
+        result,
+        'should output success result for parseable add user'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['add', '@npmcli:developers', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        {
+          added: true,
+          team: 'npmcli:developers',
+          user: 'foo',
+        },
+        'should output success result for add user json'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['add', '@npmcli:developers', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not output success if silent')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('team create <scope:team>', t => {
+  t.test('default output', t => {
+    team(['create', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output success result for create team')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['create', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(
+        result,
+        'should output parseable success result for create team'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['create', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        {
+          created: true,
+          team: 'npmcli:newteam',
+        },
+        'should output success result for create team'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['create', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not output create success if silent')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('team destroy <scope:team>', t => {
+  t.test('default output', t => {
+    team(['destroy', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output success result for destroy team')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['destroy', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output parseable result for destroy team')
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['destroy', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        {
+          deleted: true,
+          team: 'npmcli:newteam',
+        },
+        'should output parseable result for destroy team'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['destroy', '@npmcli:newteam'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not output destroy if silent')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('team ls <scope>', t => {
+  const libnpmteam = {
+    async lsTeams () {
+      return [
+        'npmcli:developers',
+        'npmcli:designers',
+        'npmcli:product',
+      ]
+    },
+  }
+
+  const team = requireInject('../../lib/team.js', {
+    ...mocks,
+    libnpmteam,
+  })
+
+  t.test('default output', t => {
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list teams for a given scope')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list teams for a parseable scope')
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        [
+          'npmcli:designers',
+          'npmcli:developers',
+          'npmcli:product',
+        ],
+        'should json list teams for a scope json'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not list teams if silent')
+      t.end()
+    })
+  })
+
+  t.test('no teams', t => {
+    const libnpmteam = {
+      async lsTeams () {
+        return []
+      },
+    }
+
+    const team = requireInject('../../lib/team.js', {
+      ...mocks,
+      libnpmteam,
+    })
+
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list no teams for a given scope')
+      t.end()
+    })
+  })
+
+  t.test('single team', t => {
+    const libnpmteam = {
+      async lsTeams () {
+        return ['npmcli:developers']
+      },
+    }
+
+    const team = requireInject('../../lib/team.js', {
+      ...mocks,
+      libnpmteam,
+    })
+
+    team(['ls', '@npmcli'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list single team for a given scope')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('team ls <scope:team>', t => {
+  const libnpmteam = {
+    async lsUsers () {
+      return ['nlf', 'ruyadorno', 'darcyclarke', 'isaacs']
+    },
+  }
+  const team = requireInject('../../lib/team.js', {
+    ...mocks,
+    libnpmteam,
+  })
+
+  t.test('default output', t => {
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list users for a given scope:team')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list users for a parseable scope:team')
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        [
+          'darcyclarke',
+          'isaacs',
+          'nlf',
+          'ruyadorno',
+        ],
+        'should list users for a scope:team json'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not output users if silent')
+      t.end()
+    })
+  })
+
+  t.test('no users', t => {
+    const libnpmteam = {
+      async lsUsers () {
+        return []
+      },
+    }
+
+    const team = requireInject('../../lib/team.js', {
+      ...mocks,
+      libnpmteam,
+    })
+
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list no users for a given scope')
+      t.end()
+    })
+  })
+
+  t.test('single user', t => {
+    const libnpmteam = {
+      async lsUsers () {
+        return ['foo']
+      },
+    }
+
+    const team = requireInject('../../lib/team.js', {
+      ...mocks,
+      libnpmteam,
+    })
+
+    team(['ls', '@npmcli:developers'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should list single user for a given scope')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('team rm <scope:team> <user>', t => {
+  t.test('default output', t => {
+    team(['rm', '@npmcli:newteam', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output success result for remove user')
+      t.end()
+    })
+  })
+
+  t.test('--parseable', t => {
+    npm.flatOptions.parseable = true
+
+    team(['rm', '@npmcli:newteam', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.matchSnapshot(result, 'should output parseable result for remove user')
+      t.end()
+    })
+  })
+
+  t.test('--json', t => {
+    npm.flatOptions.json = true
+
+    team(['rm', '@npmcli:newteam', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(
+        JSON.parse(result),
+        {
+          removed: true,
+          team: 'npmcli:newteam',
+          user: 'foo',
+        },
+        'should output json result for remove user'
+      )
+      t.end()
+    })
+  })
+
+  t.test('--silent', t => {
+    npm.flatOptions.silent = true
+
+    team(['rm', '@npmcli:newteam', 'foo'], err => {
+      if (err)
+        throw err
+
+      t.deepEqual(result, '', 'should not output rm result if silent')
+      t.end()
+    })
+  })
+
+  t.end()
+})
+
+t.test('completion', t => {
+  const { completion } = team
+
+  t.test('npm team autocomplete', t => {
+    completion({
+      conf: {
+        argv: {
+          remain: ['npm', 'team'],
+        },
+      },
+    }, (err, res) => {
+      if (err)
+        throw err
+
+      t.strictSame(
+        res,
+        ['create', 'destroy', 'add', 'rm', 'ls'],
+        'should auto complete with subcommands'
+      )
+
+      t.end()
+    })
+  })
+
+  t.test('npm team <subcommand> autocomplete', async t => {
+    const check = (subcmd) => new Promise((res, rej) =>
+      completion({
+        conf: {
+          argv: {
+            remain: ['npm', 'team', subcmd],
+          },
+        },
+      }, (err, response) => {
+        if (err)
+          rej(err)
+
+        t.strictSame(
+          response,
+          [],
+          `should not autocomplete ${subcmd} subcommand`
+        )
+        res()
+      }))
+
+    await ['create', 'destroy', 'add', 'rm', 'ls'].map(check)
+  })
+
+  t.test('npm team unknown subcommand autocomplete', t => {
+    completion({
+      conf: {
+        argv: {
+          remain: ['npm', 'team', 'missing-subcommand'],
+        },
+      },
+    }, (err, res) => {
+      t.match(
+        err,
+        /missing-subcommand not recognized/,
+        'should throw a a not recognized error'
+      )
+
+      t.end()
+    })
+  })
+
+  t.end()
+})