Repository: badges/shields
Pull request #3259 (2019-04-02 02:21:38)

refactor [GithubIssueDetail]
Some inline comments/questions added

shields-ci: 
<!--
  0 failure: 
  0 warning: 
  1 messages
  
  DangerID: danger-id-default;
-->



<table>
  <thead>
    <tr>
      <th width="50"></th>
      <th width="100%" data-danger-table="true">Messages</th>
    </tr>
  </thead>
  <tbody><tr>
      <td>:book:</td>
      <td>:sparkles: Thanks for your contribution to Shields, @calebcartwright!</td>
    </tr>
  </tbody>
</table>


<p align="right">
  Generated by :no_entry_sign: <a href="https://danger.systems/js">dangerJS</a> against 5a04ca184e8232d0e81f6371758a316d22c6de97
</p>


Patch:
diff --git a/services/github/github-issue-detail-redirect.service.js b/services/github/github-issue-detail-redirect.service.js
new file mode 100644
index 0000000000..f4ec6a1d8c
--- /dev/null
+++ b/services/github/github-issue-detail-redirect.service.js
@@ -0,0 +1,33 @@
+'use strict'
+
+const { redirector } = require('..')
+
+const whichMap = {
+  s: 'state',
+  u: 'author',
+}
+
+module.exports = [
+  redirector({
+    category: 'issue-tracking',
+    route: {
+      base: 'github',
+      pattern:
+        ':issueKind(issues|pulls)/detail/:which(s|u)/:user/:repo/:number([0-9]+)',
+    },
+    transformPath: ({ which, user, repo, number }) =>
+      `/github/issues/detail/${whichMap[which]}/${user}/${repo}/${number}`,
+    dateAdded: new Date('2019-04-04'),
+  }),
+  redirector({
+    category: 'issue-tracking',
+    route: {
+      base: 'github/pulls/detail',
+      pattern:
+        ':which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
+    },
+    transformPath: ({ which, user, repo, number }) =>
+      `/github/issues/detail/${which}/${user}/${repo}/${number}`,
+    dateAdded: new Date('2019-04-04'),
+  }),
+]
diff --git a/services/github/github-issue-detail.service.js b/services/github/github-issue-detail.service.js
index c4debed4f5..39f600f780 100644
--- a/services/github/github-issue-detail.service.js
+++ b/services/github/github-issue-detail.service.js
@@ -1,27 +1,137 @@
 'use strict'
 
-const LegacyService = require('../legacy-service')
-const {
-  makeLabel: getLabel,
-  makeBadgeData: getBadgeData,
-} = require('../../lib/badge-data')
-const { makeLogo: getLogo } = require('../../lib/logos')
-const { formatDate } = require('../text-formatters')
-const { age: ageColor } = require('../color-formatters')
+const Joi = require('joi')
+const { InvalidResponse } = require('..')
+const { nonNegativeInteger } = require('../validators')
+const { formatDate, metric } = require('../text-formatters')
+const { age } = require('../color-formatters')
+const { GithubAuthService } = require('./github-auth-service')
 const {
   documentation,
-  stateColor: githubStateColor,
-  commentsColor: githubCommentsColor,
-  checkErrorResponse: githubCheckErrorResponse,
+  errorMessagesFor,
+  stateColor,
+  commentsColor,
 } = require('./github-helpers')
 
-// This legacy service should be rewritten to use e.g. BaseJsonService.
-//
-// Tips for rewriting:
-// https://github.com/badges/shields/blob/master/doc/rewriting-services.md
-//
-// Do not base new services on this code.
-module.exports = class GithubIssueDetail extends LegacyService {
+const commonSchemaFields = {
+  number: nonNegativeInteger,
+  pull_request: Joi.any(),
+}
+
+const stateMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    state: Joi.string()
+      .allow('open', 'closed')
+      .required(),
+  }).required(),
+  transform: ({ json }) => json.state,
+  render: ({ value, isPR, number }) => ({
+    color: stateColor(value),
+    label: `${isPR ? 'pull request' : 'issue'} ${number}`,
+    message: value,
+  }),
+}
+
+const titleMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    title: Joi.string().required(),
+  }).required(),
+  transform: ({ json }) => json.title,
+  render: ({ value, isPR, number }) => ({
+    label: `${isPR ? 'pull request' : 'issue'} ${number}`,
+    message: value,
+  }),
+}
+
+const authorMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    user: Joi.object({
+      login: Joi.string().required(),
+    }).required(),
+  }).required(),
+  transform: ({ json }) => json.user.login,
+  render: ({ value }) => ({
+    label: 'author',
+    message: value,
+  }),
+}
+
+const labelMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    labels: Joi.array()
+      .items(
+        Joi.object({
+          name: Joi.string().required(),
+          color: Joi.string().required(),
+        })
+      )
+      .required(),
+  }).required(),
+  transform: ({ json }) => {
+    if (json.labels.length === 0) {
+      throw new InvalidResponse({ prettyMessage: 'no labels found' })
+    }
+    return {
+      names: json.labels.map(l => l.name),
+      colors: json.labels.map(l => l.color),
+    }
+  },
+  render: ({ value }) => {
+    let color
+    if (value.colors.length === 1) {
+      color = value.colors[0]
+    }
+    return {
+      color,
+      label: 'label',
+      message: value.names.join(' | '),
+    }
+  },
+}
+
+const commentsMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    comments: nonNegativeInteger,
+  }).required(),
+  transform: ({ json }) => json.comments,
+  render: ({ value }) => ({
+    color: commentsColor(value),
+    label: 'comments',
+    message: metric(value),
+  }),
+}
+
+const ageUpdateMap = {
+  schema: Joi.object({
+    ...commonSchemaFields,
+    created_at: Joi.date().required(),
+    updated_at: Joi.date().required(),
+  }).required(),
+  transform: ({ json, which }) =>
+    which === 'age' ? json.created_at : json.updated_at,
+  render: ({ which, value }) => ({
+    color: age(value),
+    label: which === 'age' ? 'created' : 'updated',
+    message: formatDate(value),
+  }),
+}
+
+const whichMap = {
+  state: stateMap,
+  title: titleMap,
+  author: authorMap,
+  label: labelMap,
+  comments: commentsMap,
+  age: ageUpdateMap,
+  'last-update': ageUpdateMap,
+}
+
+module.exports = class GithubIssueDetail extends GithubAuthService {
   static get category() {
     return 'issue-tracking'
   }
@@ -30,7 +140,7 @@ module.exports = class GithubIssueDetail extends LegacyService {
     return {
       base: 'github/issues/detail',
       pattern:
-        ':which(s|state|title|u|author|label|comments|age|last-update)/:user/:repo/:number(0-9+)',
+        ':which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
     }
   }
 
@@ -38,19 +148,18 @@ module.exports = class GithubIssueDetail extends LegacyService {
     return [
       {
         title: 'GitHub issue/pull request detail',
-        pattern:
-          ':which(state|title|author|label|comments|age|last-update)/:user/:repo/:number',
         namedParams: {
           which: 'state',
           user: 'badges',
           repo: 'shields',
           number: '979',
         },
-        staticPreview: {
-          label: 'issue 979',
-          message: 'closed',
-          color: 'red',
-        },
+        staticPreview: this.render({
+          which: 'state',
+          value: 'closed',
+          isPR: false,
+          number: '979',
+        }),
         keywords: [
           'state',
           'title',
@@ -65,96 +174,34 @@ module.exports = class GithubIssueDetail extends LegacyService {
     ]
   }
 
-  static registerLegacyRouteHandler({ camp, cache, githubApiProvider }) {
-    camp.route(
-      /^\/github\/(?:issues|pulls)\/detail\/(s|title|u|label|comments|age|last-update)\/([^/]+)\/([^/]+)\/(\d+)\.(svg|png|gif|jpg|json)$/,
-      cache((queryParams, match, sendBadge, request) => {
-        const [, which, owner, repo, number, format] = match
-        const uri = `/repos/${owner}/${repo}/issues/${number}`
-        const badgeData = getBadgeData(
-          `issue/pull request ${number}`,
-          queryParams
-        )
-        if (badgeData.template === 'social') {
-          badgeData.logo = getLogo('github', queryParams)
-        }
-        githubApiProvider.request(request, uri, {}, (err, res, buffer) => {
-          if (
-            githubCheckErrorResponse(
-              badgeData,
-              err,
-              res,
-              'issue, pull request or repo not found'
-            )
-          ) {
-            sendBadge(format, badgeData)
-            return
-          }
-          try {
-            const parsedData = JSON.parse(buffer)
-            const isPR = 'pull_request' in parsedData
-            const noun = isPR ? 'pull request' : 'issue'
-            badgeData.text[0] = getLabel(
-              `${noun} ${parsedData.number}`,
-              queryParams
-            )
-            switch (which) {
-              case 's':
-              case 'state': {
-                const state = (badgeData.text[1] = parsedData.state)
-                badgeData.colorscheme = undefined
-                badgeData.colorB = queryParams.colorB || githubStateColor(state)
-                break
-              }
-              case 'title':
-                badgeData.text[1] = parsedData.title
-                break
-              case 'u':
-              case 'author':
-                badgeData.text[0] = getLabel('author', queryParams)
-                badgeData.text[1] = parsedData.user.login
-                break
-              case 'label':
-                badgeData.text[0] = getLabel('label', queryParams)
-                badgeData.text[1] = parsedData.labels
-                  .map(i => i.name)
-                  .join(' | ')
-                if (parsedData.labels.length === 1) {
-                  badgeData.colorscheme = undefined
-                  badgeData.colorB =
-                    queryParams.colorB || parsedData.labels[0].color
-                }
-                break
-              case 'comments': {
-                badgeData.text[0] = getLabel('comments', queryParams)
-                const comments = (badgeData.text[1] = parsedData.comments)
-                badgeData.colorscheme = undefined
-                badgeData.colorB =
-                  queryParams.coloB || githubCommentsColor(comments)
-                break
-              }
-              case 'age':
-              case 'last-update': {
-                const label = which === 'age' ? 'created' : 'updated'
-                const date =
-                  which === 'age'
-                    ? parsedData.created_at
-                    : parsedData.updated_at
-                badgeData.text[0] = getLabel(label, queryParams)
-                badgeData.text[1] = formatDate(date)
-                badgeData.colorscheme = ageColor(Date.parse(date))
-                break
-              }
-              default:
-                throw Error('Unreachable due to regex')
-            }
-            sendBadge(format, badgeData)
-          } catch (e) {
-            badgeData.text[1] = 'invalid'
-            sendBadge(format, badgeData)
-          }
-        })
-      })
-    )
+  static get defaultBadgeData() {
+    return {
+      label: 'issue/pull request',
+      color: 'informational',
+    }
+  }
+
+  static render({ which, value, isPR, number }) {
+    return whichMap[which].render({ which, value, isPR, number })
+  }
+
+  async fetch({ which, user, repo, number }) {
+    return this._requestJson({
+      url: `/repos/${user}/${repo}/issues/${number}`,
+      schema: whichMap[which].schema,
+      errorMessages: errorMessagesFor('issue, pull request or repo not found'),
+    })
+  }
+
+  transform({ json, which }) {
+    const value = whichMap[which].transform({ json, which })
+    const isPR = json.hasOwnProperty('pull_request')
+    return { value, isPR }
+  }
+
+  async handle({ which, user, repo, number }) {
+    const json = await this.fetch({ which, user, repo, number })
+    const { value, isPR } = this.transform({ json, which })
+    return this.constructor.render({ which, value, isPR, number })
   }
 }
diff --git a/services/github/github-issue-detail.spec.js b/services/github/github-issue-detail.spec.js
new file mode 100644
index 0000000000..22eb89d4b3
--- /dev/null
+++ b/services/github/github-issue-detail.spec.js
@@ -0,0 +1,182 @@
+'use strict'
+
+const { expect } = require('chai')
+const { test, given } = require('sazerac')
+const { InvalidResponse } = require('..')
+const { age } = require('../color-formatters')
+const { formatDate, metric } = require('../text-formatters')
+const GithubIssueDetail = require('./github-issue-detail.service')
+const { stateColor, commentsColor } = require('./github-helpers')
+
+describe('GithubIssueDetail', function() {
+  test(GithubIssueDetail.render, () => {
+    given({
+      which: 'state',
+      value: 'open',
+      number: '12',
+      isPR: true,
+    }).expect({
+      label: 'pull request 12',
+      message: 'open',
+      color: stateColor('open'),
+    })
+    given({
+      which: 'state',
+      value: 'closed',
+      number: '15',
+      isPR: false,
+    }).expect({
+      label: 'issue 15',
+      message: 'closed',
+      color: stateColor('closed'),
+    })
+    given({
+      which: 'title',
+      value: 'refactor [FooService]',
+      number: '232',
+      isPR: true,
+    }).expect({
+      label: 'pull request 232',
+      message: 'refactor [FooService]',
+    })
+    given({
+      which: 'title',
+      value: 'Packagist: invalid response data',
+      number: '345',
+      isPR: false,
+    }).expect({
+      label: 'issue 345',
+      message: 'Packagist: invalid response data',
+    })
+    given({
+      which: 'author',
+      value: 'calebcartwright',
+    }).expect({
+      label: 'author',
+      message: 'calebcartwright',
+    })
+    given({
+      which: 'label',
+      value: { names: ['feature'], colors: ['a2eeef'] },
+    }).expect({
+      color: 'a2eeef',
+      message: 'feature',
+      label: 'label',
+    })
+    given({
+      which: 'label',
+      value: { names: ['service-badge', 'bug'], colors: ['a2eeef', 'ee0701'] },
+    }).expect({
+      color: undefined,
+      message: 'service-badge | bug',
+      label: 'label',
+    })
+    given({ which: 'comments', value: 27 }).expect({
+      label: 'comments',
+      message: metric(27),
+      color: commentsColor('closed'),
+    })
+    given({
+      which: 'age',
+      value: '2019-04-01T20:09:31Z',
+    }).expect({
+      label: 'created',
+      message: formatDate('2019-04-01T20:09:31Z'),
+      color: age('2019-04-01T20:09:31Z'),
+    })
+    given({
+      which: 'last-update',
+      value: '2019-04-02T20:09:31Z',
+    }).expect({
+      label: 'updated',
+      message: formatDate('2019-04-02T20:09:31Z'),
+      color: age('2019-04-02T20:09:31Z'),
+    })
+  })
+
+  test(GithubIssueDetail.prototype.transform, () => {
+    given({
+      which: 'state',
+      json: { state: 'closed' },
+    }).expect({
+      value: 'closed',
+      isPR: false,
+    })
+    given({
+      which: 'title',
+      json: { pull_request: {}, title: 'refactor [Codecov]' },
+    }).expect({
+      value: 'refactor [Codecov]',
+      isPR: true,
+    })
+    given({
+      which: 'author',
+      json: { user: { login: 'dependabot' } },
+    }).expect({
+      value: 'dependabot',
+      isPR: false,
+    })
+    given({
+      which: 'label',
+      json: {
+        pull_request: {},
+        labels: [
+          { name: 'service-badge', color: 'a2eeef' },
+          { name: 'bug', color: 'ee0701' },
+        ],
+      },
+    }).expect({
+      value: {
+        names: ['service-badge', 'bug'],
+        colors: ['a2eeef', 'ee0701'],
+      },
+      isPR: true,
+    })
+    given({
+      which: 'label',
+      json: { labels: [{ name: 'bug', color: 'ee0701' }] },
+    }).expect({
+      value: {
+        names: ['bug'],
+        colors: ['ee0701'],
+      },
+      isPR: false,
+    })
+    given({
+      which: 'comments',
+      json: { comments: 100 },
+    }).expect({
+      value: 100,
+      isPR: false,
+    })
+    given({
+      which: 'age',
+      json: { created_at: '2019-04-01T20:09:31Z' },
+    }).expect({
+      value: '2019-04-01T20:09:31Z',
+      isPR: false,
+    })
+    given({
+      which: 'last-update',
+      json: { updated_at: '2019-04-02T20:09:31Z' },
+    }).expect({
+      value: '2019-04-02T20:09:31Z',
+      isPR: false,
+    })
+  })
+
+  context('transform()', function() {
+    it('throws InvalidResponse error when issue has no labels', function() {
+      try {
+        GithubIssueDetail.prototype.transform({
+          which: 'label',
+          json: { labels: [] },
+        })
+        expect.fail('Expected to throw')
+      } catch (e) {
+        expect(e).to.be.an.instanceof(InvalidResponse)
+        expect(e.prettyMessage).to.equal('no labels found')
+      }
+    })
+  })
+})


Test patch:
diff --git a/services/github/github-issue-detail-redirect.tester.js b/services/github/github-issue-detail-redirect.tester.js
new file mode 100644
index 0000000000..bf857018b7
--- /dev/null
+++ b/services/github/github-issue-detail-redirect.tester.js
@@ -0,0 +1,59 @@
+'use strict'
+
+const { ServiceTester } = require('../tester')
+
+const t = (module.exports = new ServiceTester({
+  id: 'GithubIssueDetailRedirect',
+  title: 'GithubIssueDetailRedirect',
+  pathPrefix: '/github',
+}))
+
+t.create('github issue detail (pulls alias)')
+  .get('/pulls/detail/author/badges/shields/3259.svg', {
+    followRedirect: false,
+  })
+  .expectStatus(301)
+  .expectHeader(
+    'Location',
+    '/github/issues/detail/author/badges/shields/3259.svg'
+  )
+
+t.create('github issue detail (s shorthand)')
+  .get('/issues/detail/s/badges/shields/979.svg', {
+    followRedirect: false,
+  })
+  .expectStatus(301)
+  .expectHeader(
+    'Location',
+    '/github/issues/detail/state/badges/shields/979.svg'
+  )
+
+t.create('github issue detail (u shorthand)')
+  .get('/issues/detail/u/badges/shields/979.svg', {
+    followRedirect: false,
+  })
+  .expectStatus(301)
+  .expectHeader(
+    'Location',
+    '/github/issues/detail/author/badges/shields/979.svg'
+  )
+
+t.create('github pulls detail (s shorthand)')
+  .get('/pulls/detail/s/badges/shields/979.svg', {
+    followRedirect: false,
+  })
+  .expectStatus(301)
+  .expectHeader(
+    'Location',
+    '/github/issues/detail/state/badges/shields/979.svg'
+  )
+
+t.create('github pulls detail (u shorthand)')
+  .get('/pulls/detail/u/badges/shields/979.svg', {
+    followRedirect: false,
+  })
+  .expectStatus(301)
+  .expectHeader(
+    'Location',
+    '/github/issues/detail/author/badges/shields/979.svg'
+  )
diff --git a/services/github/github-issue-detail.tester.js b/services/github/github-issue-detail.tester.js
index 82d2c21303..6e98ba824b 100644
--- a/services/github/github-issue-detail.tester.js
+++ b/services/github/github-issue-detail.tester.js
@@ -5,16 +5,16 @@ const { isFormattedDate } = require('../test-validators')
 const t = (module.exports = require('../tester').createServiceTester())
 
 t.create('github issue state')
-  .get('/s/badges/shields/979.json')
+  .get('/state/badges/shields/979.json')
   .expectBadge({
     label: 'issue 979',
     message: Joi.equal('open', 'closed'),
   })
 
 t.create('github issue state (repo not found)')
-  .get('/s/badges/helmets/979.json')
+  .get('/state/badges/helmets/979.json')
   .expectBadge({
-    label: 'issue/pull request 979',
+    label: 'issue/pull request',
     message: 'issue, pull request or repo not found',
   })
 
@@ -26,7 +26,7 @@ t.create('github issue title')
   })
 
 t.create('github issue author')
-  .get('/u/badges/shields/979.json')
+  .get('/author/badges/shields/979.json')
   .expectBadge({ label: 'author', message: 'paulmelnikow' })
 
 t.create('github issue label')