Repository: badges/shields
Pull request #3295 (2019-04-13 01:13:23)

Enhance status badge for PRs in [GithubIssueDetail]
I'm basically following other PRs -- I am not an expert on JS.
Please don't hesitate to spot any problems.

This PR addresses #1719 

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, @ice1000!</td>
    </tr>
  </tbody>
</table>


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

ice1000: There are some linter errors that I don't quite understand:
```
  53:27  error  Identifier 'merged_at' is not in camel case  camelcase
  53:38  error  Identifier 'closed_at' is not in camel case  camelcase
  55:9   error  Identifier 'merged_at' is not in camel case  camelcase
  57:9   error  Expected property shorthand                  object-shorthand
  61:16  error  Identifier 'closed_at' is not in camel case  camelcase
  63:9   error  Expected property shorthand                  object-shorthand
  69:9   error  Expected property shorthand                  object-shorthand
  77:13  error  Identifier 'merged_at' is not in camel case  camelcase
  77:24  error  Identifier 'closed_at' is not in camel case  camelcase
```

What if the API is using snake case?
ice1000: Trying to use this code:

```js
    return this.constructor.render({
      number,
      mergedAt: json['merged_at'],
      closedAt: json['closed_at'],
    })
```

instead of matching the properties out directly. Hope the CI will be happy with that.
ice1000: What I am thinking about is that the badge rendering logic is completely different and they're using different GitHub APIs.

Several questions:

+ Is it a good idea to have 2 API requests in the issue detail badge logic?
+ Isn't the badge rendering logic I've added are quite different from existing ones?

Yes, these questions are all present in the original issue, but my reason of adding a new badge is that I don't want to have 2 API requests in one badge and the logic is pretty different (because we're using different APIs!).
calebcartwright: > Is it a good idea to have 2 API requests in the issue detail badge logic?

No one is proposing making 2 sequential API calls. There's plenty of badge service implementations that use parameters to conditionally determine the URL call to make, the schema to use, how to transform the response, and how to render. 

Many such conditions are already handled within the existing service class. 
ice1000: > route/query parameters to conditionally determine the URL call to make

This is very very reasonable! I've got a plan in mind, please comment:

+ Add a new `which` to issue service, like `pull`
+ If the `which` is `pull`, use the logic that I've implemented to determine the `message`
calebcartwright: > This is very very reasonable! I've got a plan in mind, please comment:
> 
> * Add a new `which` to the issue detail service, like `pull`
> * If the `which` is `pull`, use the logic that I've implemented to determine the `message`

That's probably the approach I'd suggest as well. Below are some snippets from what I was mocking up locally, may be a useful reference:

Update the state map:
https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail.service.js#L21
```js
const stateMap = {
  schema: Joi.object({
    ...commonSchemaFields,
    state: Joi.string()
      .allow('open', 'closed')
      .required(),
    merged_at: Joi.string().allow(null),
  }).required(),
  transform: ({ json }) => ({
    state: json.state,
    merged: json.merged_at !== null,
  }),
  render: ({ value, isPR, number }) => {
    const state = value.state
    const label = `${isPR ? 'pull request' : 'issue'} ${number}`

    if (!isPR || state === 'open') {
      return {
        color: stateColor(state),
        label,
        message: state,
      }
    }

    if (value.merged) {
      return {
        label,
        message: 'merged',
        color: 'blueviolet',
      }
    }

    return {
      label,
      message: 'rejected',
      color: 'red',
    }
  },
}
```

the service class `route`:
```js
  static get route() {
    return {
      base: 'github',
      pattern:
        ':kind(issues|pulls)/detail/:which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
    }
  }
```

the service class example will need to have the `namedParams` and `staticPreview` tweaked:
```js
...
        namedParams: {
          kind: 'issues',
          which: 'state',
          user: 'badges',
          repo: 'shields',
          number: '979',
        },
        staticPreview: this.render({
          which: 'state',
          value: { state: 'closed' },
          isPR: false,
          number: '979',
        }),
...
```

and the service implementations for `fetch`, `handle` and `transform` at the bottom of the file:
```js
  async fetch({ kind, which, user, repo, number }) {
    return this._requestJson({
      url: `/repos/${user}/${repo}/${kind}/${number}`,
      schema: whichMap[which].schema,
      errorMessages: errorMessagesFor('issue, pull request or repo not found'),
    })
  }

  transform({ json, which, kind }) {
    const value = whichMap[which].transform({ json, which })
    const isPR = json.hasOwnProperty('pull_request') || kind === 'pulls'
    return { value, isPR }
  }

  async handle({ kind, which, user, repo, number }) {
    const json = await this.fetch({ kind, which, user, repo, number })
    const { value, isPR } = this.transform({ json, which, kind })
    return this.constructor.render({ which, value, isPR, number })
  }
```

And lastly, the `redirector` that's currently remapping `pulls` to `issues` here: https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.service.js#L22 will need to be removed

calebcartwright: There's a few tests that would need to be updated as well:

* the [unit tests](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail.spec.js#L13) should have a couple more added to validate the new render logic and the [transform](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail.spec.js#L98) logic 
* in the [redirector tests](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.tester.js) 
  * [this one](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.tester.js#L11) can be deleted
  * [this one](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.tester.js#L48) should be updated (change `issues` to `pulls`)
  * [this one](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail-redirect.tester.js#L58) should be updated (change `issues` to `pulls`)
* in the [live/service tests](https://github.com/badges/shields/blob/gh-pr-status/services/github/github-issue-detail.tester.js) we should add a test or two that use a real PR

ice1000: Done, please review.
calebcartwright: Excellent! I'll review here shortly
ice1000: Addressed all comments, rebased on master, please take a look.
calebcartwright: Review app deployed https://shields-staging-pr-3295.herokuapp.com/

https://shields-staging-pr-3295.herokuapp.com/github/pulls/detail/state/badges/shields/3269.svg
![](https://shields-staging-pr-3295.herokuapp.com/github/pulls/detail/state/badges/shields/3269.svg)

Looking good!

Patch:
diff --git a/services/github/github-issue-detail-redirect.service.js b/services/github/github-issue-detail-redirect.service.js
index f4ec6a1d8c..90ffa91b60 100644
--- a/services/github/github-issue-detail-redirect.service.js
+++ b/services/github/github-issue-detail-redirect.service.js
@@ -15,19 +15,10 @@ module.exports = [
       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}`,
+    transformPath: ({ issueKind, which, user, repo, number }) =>
+      `/github/${issueKind}/detail/${
+        whichMap[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 39f600f780..f44d6a6021 100644
--- a/services/github/github-issue-detail.service.js
+++ b/services/github/github-issue-detail.service.js
@@ -24,13 +24,36 @@ const stateMap = {
     state: Joi.string()
       .allow('open', 'closed')
       .required(),
+    merged_at: Joi.string().allow(null),
   }).required(),
-  transform: ({ json }) => json.state,
-  render: ({ value, isPR, number }) => ({
-    color: stateColor(value),
-    label: `${isPR ? 'pull request' : 'issue'} ${number}`,
-    message: value,
+  transform: ({ json }) => ({
+    state: json.state,
+    // Because eslint will not be happy with this snake_case name :(
+    merged: json['merged_at'] !== null,
   }),
+  render: ({ value, isPR, number }) => {
+    const state = value.state
+    const label = `${isPR ? 'pull request' : 'issue'} ${number}`
+
+    if (!isPR || state === 'open') {
+      return {
+        color: stateColor(state),
+        label,
+        message: state,
+      }
+    } else if (value.merged) {
+      return {
+        label,
+        message: 'merged',
+        color: 'blueviolet',
+      }
+    } else
+      return {
+        label,
+        message: 'rejected',
+        color: 'red',
+      }
+  },
 }
 
 const titleMap = {
@@ -138,9 +161,9 @@ module.exports = class GithubIssueDetail extends GithubAuthService {
 
   static get route() {
     return {
-      base: 'github/issues/detail',
+      base: 'github',
       pattern:
-        ':which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
+        ':kind(issues|pulls)/detail/:which(state|title|author|label|comments|age|last-update)/:user/:repo/:number([0-9]+)',
     }
   }
 
@@ -149,6 +172,7 @@ module.exports = class GithubIssueDetail extends GithubAuthService {
       {
         title: 'GitHub issue/pull request detail',
         namedParams: {
+          kind: 'issues',
           which: 'state',
           user: 'badges',
           repo: 'shields',
@@ -156,7 +180,7 @@ module.exports = class GithubIssueDetail extends GithubAuthService {
         },
         staticPreview: this.render({
           which: 'state',
-          value: 'closed',
+          value: { state: 'closed' },
           isPR: false,
           number: '979',
         }),
@@ -185,23 +209,23 @@ module.exports = class GithubIssueDetail extends GithubAuthService {
     return whichMap[which].render({ which, value, isPR, number })
   }
 
-  async fetch({ which, user, repo, number }) {
+  async fetch({ kind, which, user, repo, number }) {
     return this._requestJson({
-      url: `/repos/${user}/${repo}/issues/${number}`,
+      url: `/repos/${user}/${repo}/${kind}/${number}`,
       schema: whichMap[which].schema,
       errorMessages: errorMessagesFor('issue, pull request or repo not found'),
     })
   }
 
-  transform({ json, which }) {
+  transform({ json, which, kind }) {
     const value = whichMap[which].transform({ json, which })
-    const isPR = json.hasOwnProperty('pull_request')
+    const isPR = json.hasOwnProperty('pull_request') || kind === 'pulls'
     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 })
+  async handle({ kind, which, user, repo, number }) {
+    const json = await this.fetch({ kind, which, user, repo, number })
+    const { value, isPR } = this.transform({ json, which, kind })
     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
index 22eb89d4b3..c876d7c207 100644
--- a/services/github/github-issue-detail.spec.js
+++ b/services/github/github-issue-detail.spec.js
@@ -12,7 +12,7 @@ describe('GithubIssueDetail', function() {
   test(GithubIssueDetail.render, () => {
     given({
       which: 'state',
-      value: 'open',
+      value: { state: 'open' },
       number: '12',
       isPR: true,
     }).expect({
@@ -22,7 +22,7 @@ describe('GithubIssueDetail', function() {
     })
     given({
       which: 'state',
-      value: 'closed',
+      value: { state: 'closed' },
       number: '15',
       isPR: false,
     }).expect({
@@ -99,9 +99,26 @@ describe('GithubIssueDetail', function() {
       which: 'state',
       json: { state: 'closed' },
     }).expect({
-      value: 'closed',
+      // Since it's a PR, the "merged" value is not crucial here.
+      value: { state: 'closed', merged: true },
       isPR: false,
     })
+    given({
+      which: 'state',
+      kind: 'pulls',
+      json: { state: 'closed', merged_at: null },
+    }).expect({
+      value: { state: 'closed', merged: false },
+      isPR: true,
+    })
+    given({
+      which: 'state',
+      kind: 'pulls',
+      json: { state: 'closed', merged_at: 'I am not null' },
+    }).expect({
+      value: { state: 'closed', merged: true },
+      isPR: true,
+    })
     given({
       which: 'title',
       json: { pull_request: {}, title: 'refactor [Codecov]' },


Test patch:
diff --git a/services/github/github-issue-detail-redirect.tester.js b/services/github/github-issue-detail-redirect.tester.js
index bf857018b7..b8946e7978 100644
--- a/services/github/github-issue-detail-redirect.tester.js
+++ b/services/github/github-issue-detail-redirect.tester.js
@@ -8,16 +8,6 @@ const t = (module.exports = new ServiceTester({
   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,
@@ -43,10 +33,7 @@ t.create('github pulls detail (s shorthand)')
     followRedirect: false,
   })
   .expectStatus(301)
-  .expectHeader(
-    'Location',
-    '/github/issues/detail/state/badges/shields/979.svg'
-  )
+  .expectHeader('Location', '/github/pulls/detail/state/badges/shields/979.svg')
 
 t.create('github pulls detail (u shorthand)')
   .get('/pulls/detail/u/badges/shields/979.svg', {
@@ -55,5 +42,5 @@ t.create('github pulls detail (u shorthand)')
   .expectStatus(301)
   .expectHeader(
     'Location',
-    '/github/issues/detail/author/badges/shields/979.svg'
+    '/github/pulls/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 6e98ba824b..0a9de66414 100644
--- a/services/github/github-issue-detail.tester.js
+++ b/services/github/github-issue-detail.tester.js
@@ -5,32 +5,32 @@ const { isFormattedDate } = require('../test-validators')
 const t = (module.exports = require('../tester').createServiceTester())
 
 t.create('github issue state')
-  .get('/state/badges/shields/979.json')
+  .get('/issues/detail/state/badges/shields/979.json')
   .expectBadge({
     label: 'issue 979',
     message: Joi.equal('open', 'closed'),
   })
 
 t.create('github issue state (repo not found)')
-  .get('/state/badges/helmets/979.json')
+  .get('/issues/detail/state/badges/helmets/979.json')
   .expectBadge({
     label: 'issue/pull request',
     message: 'issue, pull request or repo not found',
   })
 
 t.create('github issue title')
-  .get('/title/badges/shields/979.json')
+  .get('/issues/detail/title/badges/shields/979.json')
   .expectBadge({
     label: 'issue 979',
     message: 'Github rate limits cause transient service test failures in CI',
   })
 
 t.create('github issue author')
-  .get('/author/badges/shields/979.json')
+  .get('/issues/detail/author/badges/shields/979.json')
   .expectBadge({ label: 'author', message: 'paulmelnikow' })
 
 t.create('github issue label')
-  .get('/label/badges/shields/979.json')
+  .get('/issues/detail/label/badges/shields/979.json')
   .expectBadge({
     label: 'label',
     message: Joi.equal(
@@ -40,16 +40,28 @@ t.create('github issue label')
   })
 
 t.create('github issue comments')
-  .get('/comments/badges/shields/979.json')
+  .get('/issues/detail/comments/badges/shields/979.json')
   .expectBadge({
     label: 'comments',
     message: Joi.number().greater(15),
   })
 
 t.create('github issue age')
-  .get('/age/badges/shields/979.json')
+  .get('/issues/detail/age/badges/shields/979.json')
   .expectBadge({ label: 'created', message: isFormattedDate })
 
 t.create('github issue update')
-  .get('/last-update/badges/shields/979.json')
+  .get('/issues/detail/last-update/badges/shields/979.json')
   .expectBadge({ label: 'updated', message: isFormattedDate })
+
+t.create('github pull request merge state')
+  .get('/pulls/detail/state/pingcap/raft-rs/201.json')
+  .expectBadge({ label: 'pull request 201', message: 'merged' })
+
+t.create('github pull request merge state (pull request not found)')
+  // it's an issue
+  .get('/pulls/detail/state/pingcap/raft-rs/177.json')
+  .expectBadge({
+    label: 'issue/pull request',
+    message: 'issue, pull request or repo not found',
+  })