fix: fund with multiple funding sources

`npm fund` human output was appending any items that had multiple
funding sources to the current package title as comma-separated names.

This commit fixes the problem by properly selecting the first item of a
each funding element and only using that as its index for printing the
human output tree representation.

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

3 files changed, 87 insertions, 20 deletions

diff --git a/lib/fund.js b/lib/fund.js
index fde01f469..d1c58d9b9 100644
--- a/lib/fund.js
+++ b/lib/fund.js
@@ -42,26 +42,33 @@ function printHuman (fundingInfo, { color, unicode }) {
 
   const result = depth({
     tree: fundingInfo,
+
+    // composes human readable package name
+    // and creates a new archy item for readable output
     visit: ({ name, version, funding }) => {
-      // composes human readable package name
-      // and creates a new archy item for readable output
-      const { url } = funding || {}
+      const [fundingSource] = []
+        .concat(normalizeFunding(funding))
+        .filter(isValidFunding)
+      const { url } = fundingSource || {}
       const pkgRef = getPrintableName({ name, version })
-      const label = url ? tree({
-        label: color ? chalk.bgBlack.white(url) : url,
-        nodes: [pkgRef]
-      }).trim() : pkgRef
       let item = {
-        label
+        label: pkgRef
       }
 
-      // stacks all packages together under the same item
-      if (seenUrls.has(url)) {
-        item = seenUrls.get(url)
-        item.label += `, ${pkgRef}`
-        return null
-      } else {
-        seenUrls.set(url, item)
+      if (url) {
+        item.label = tree({
+          label: color ? chalk.bgBlack.white(url) : url,
+          nodes: [pkgRef]
+        }).trim()
+
+        // stacks all packages together under the same item
+        if (seenUrls.has(url)) {
+          item = seenUrls.get(url)
+          item.label += `, ${pkgRef}`
+          return null
+        } else {
+          seenUrls.set(url, item)
+        }
       }
 
       return item
diff --git a/tap-snapshots/test-lib-fund.js-TAP.test.js b/tap-snapshots/test-lib-fund.js-TAP.test.js
index 4ff676ca1..7ad86ebee 100644
--- a/tap-snapshots/test-lib-fund.js-TAP.test.js
+++ b/tap-snapshots/test-lib-fund.js-TAP.test.js
@@ -82,3 +82,13 @@ no-funding-package@0.0.0
 
 
 `
+
+exports[`test/lib/fund.js TAP sub dep with fund info and a parent with no funding info > should nest sub dep as child of root 1`] = `
+test-multiple-funding-sources@1.0.0
++-- http://example.com/b
+|   \`-- b@1.0.0
+\`-- http://example.com/c
+    \`-- c@1.0.0
+
+
+`
diff --git a/test/lib/fund.js b/test/lib/fund.js
index 9025d7ee0..fc6a63aa1 100644
--- a/test/lib/fund.js
+++ b/test/lib/fund.js
@@ -190,7 +190,7 @@ const _flatOptions = {
   unicode: false,
   which: undefined
 }
-let openUrl = (url, msg, cb) => {
+const openUrl = (url, msg, cb) => {
   if (url === 'http://npmjs.org') {
     cb(new Error('ERROR'))
     return
@@ -212,7 +212,7 @@ const fund = requireInject('../../lib/fund.js', {
   },
   '../../lib/utils/open-url.js': openUrl,
   '../../lib/utils/output.js': msg => { result += msg + '\n' },
-  'pacote': {
+  pacote: {
     manifest: (arg) => arg.name === 'ntl'
       ? Promise.resolve({
         funding: 'http://example.com/pacote'
@@ -221,7 +221,6 @@ const fund = requireInject('../../lib/fund.js', {
   }
 })
 
-
 test('fund with no package containing funding', t => {
   _flatOptions.prefix = t.testdir({
     'package.json': JSON.stringify({
@@ -472,7 +471,7 @@ test('fund using symlink ref', t => {
       name: 'using-symlink-ref',
       version: '1.0.0'
     }),
-    'a': {
+    a: {
       'package.json': JSON.stringify({
         name: 'a',
         version: '1.0.0',
@@ -497,6 +496,8 @@ test('fund using symlink ref', t => {
 
     // using target ref
     fund(['./a'], (err) => {
+      t.ifError(err, 'should not error out')
+
       t.match(
         printUrl,
         'http://example.com/a',
@@ -779,7 +780,7 @@ test('fund colors', t => {
           version: '1.0.0',
           funding: 'http://example.com/e'
         })
-      },
+      }
     }
   })
   _flatOptions.color = true
@@ -793,3 +794,52 @@ test('fund colors', t => {
     t.end()
   })
 })
+
+test('sub dep with fund info and a parent with no funding info', t => {
+  _flatOptions.prefix = t.testdir({
+    'package.json': JSON.stringify({
+      name: 'test-multiple-funding-sources',
+      version: '1.0.0',
+      dependencies: {
+        a: '^1.0.0',
+        b: '^1.0.0'
+      }
+    }),
+    node_modules: {
+      a: {
+        'package.json': JSON.stringify({
+          name: 'a',
+          version: '1.0.0',
+          dependencies: {
+            c: '^1.0.0'
+          }
+        })
+      },
+      b: {
+        'package.json': JSON.stringify({
+          name: 'b',
+          version: '1.0.0',
+          funding: 'http://example.com/b'
+        })
+      },
+      c: {
+        'package.json': JSON.stringify({
+          name: 'c',
+          version: '1.0.0',
+          funding: [
+            'http://example.com/c',
+            'http://example.com/c-other'
+          ]
+        })
+      }
+    }
+  })
+
+  fund([], (err) => {
+    t.ifError(err, 'should not error out')
+    t.matchSnapshot(result, 'should nest sub dep as child of root')
+
+    result = ''
+    t.end()
+  })
+})