Merge branch 'master' into refactor-badge-color

doc/TUTORIAL.md

@@ -214,6 +214,7 @@ Description of the code:
 1. As with the first example, we declare strict mode at the start of each file.
 2. Our badge will query a JSON API so we will extend `BaseJsonService` instead of `BaseService`. This contains some helpers to reduce the need for boilerplate when calling a JSON API.
 3. In this case we are making a version badge, which is a common pattern. Instead of directly returning an object in this badge we will use a helper function to format our data consistently. There are a variety of helper functions to help with common tasks in `/lib`. Some useful generic helpers can be found in:
+    * [build-status.js](https://github.com/badges/shields/blob/master/lib/build-status.js)
     * [color-formatters.js](https://github.com/badges/shields/blob/master/lib/color-formatters.js)
     * [licenses.js](https://github.com/badges/shields/blob/master/lib/licenses.js)
     * [text-formatters.js](https://github.com/badges/shields/blob/master/lib/text-formatters.js)

frontend/components/markup-modal/markup-modal-content.js

@@ -29,7 +29,6 @@ export default class MarkupModalContent extends React.Component {
   state = {
     path: '',
     link: '',
-    markupFormat: 'link',
     message: undefined,
   }
 
@@ -146,7 +145,7 @@ export default class MarkupModalContent extends React.Component {
   }
 
   handleQueryStringChange = ({ queryString, isComplete }) => {
-    this.setState({ queryString, queryStringIsComplete: isComplete })
+    this.setState({ queryString })
   }
 
   render() {

lib/build-status.js

@@ -2,12 +2,22 @@
 
 const Joi = require('joi')
 
-const happyStatuses = ['passed', 'passing', 'success']
+const greenStatuses = [
+  'fixed',
+  'passed',
+  'passing',
+  'succeeded',
+  'success',
+  'successful',
+]
+
+const orangeStatuses = ['partially succeeded', 'unstable', 'timeout']
 
-const unhappyStatuses = ['error', 'failed', 'failing', 'unstable']
+const redStatuses = ['error', 'failed', 'failing']
 
 const otherStatuses = [
   'building',
+  'canceled',
   'cancelled',
   'expired',
   'no tests',
@@ -20,21 +30,25 @@ const otherStatuses = [
   'scheduled',
   'skipped',
   'stopped',
-  'timeout',
   'waiting',
 ]
 
 const isBuildStatus = Joi.equal(
-  happyStatuses.concat(unhappyStatuses).concat(otherStatuses)
+  greenStatuses
+    .concat(orangeStatuses)
+    .concat(redStatuses)
+    .concat(otherStatuses)
 )
 
 function renderBuildStatusBadge({ label, status }) {
   let message
   let color
-  if (happyStatuses.includes(status)) {
+  if (greenStatuses.includes(status)) {
     message = 'passing'
     color = 'brightgreen'
-  } else if (unhappyStatuses.includes(status)) {
+  } else if (orangeStatuses.includes(status)) {
+    color = 'orange'
+  } else if (redStatuses.includes(status)) {
     message = status === 'failed' ? 'failing' : status
     color = 'red'
   } else {

lib/build-status.spec.js

@@ -29,26 +29,37 @@ test(renderBuildStatusBadge, () => {
 
 test(renderBuildStatusBadge, () => {
   forCases([
+    given({ status: 'fixed' }),
     given({ status: 'passed' }),
     given({ status: 'passing' }),
+    given({ status: 'succeeded' }),
     given({ status: 'success' }),
+    given({ status: 'successful' }),
   ]).assert('should be brightgreen', b =>
     expect(b).to.include({ color: 'brightgreen' })
   )
 })
 
+test(renderBuildStatusBadge, () => {
+  forCases([
+    given({ status: 'partially succeeded' }),
+    given({ status: 'timeout' }),
+    given({ status: 'unstable' }),
+  ]).assert('should be orange', b => expect(b).to.include({ color: 'orange' }))
+})
+
 test(renderBuildStatusBadge, () => {
   forCases([
     given({ status: 'error' }),
     given({ status: 'failed' }),
     given({ status: 'failing' }),
-    given({ status: 'unstable' }),
   ]).assert('should be red', b => expect(b).to.include({ color: 'red' }))
 })
 
 test(renderBuildStatusBadge, () => {
   forCases([
     given({ status: 'building' }),
+    given({ status: 'canceled' }),
     given({ status: 'cancelled' }),
     given({ status: 'expired' }),
     given({ status: 'no tests' }),
@@ -61,7 +72,6 @@ test(renderBuildStatusBadge, () => {
     given({ status: 'scheduled' }),
     given({ status: 'skipped' }),
     given({ status: 'stopped' }),
-    given({ status: 'timeout' }),
     given({ status: 'waiting' }),
   ]).assert('should have undefined color', b =>
     expect(b).to.include({ color: undefined })

lib/request-handler.js

@@ -89,19 +89,35 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
 
   const allowedKeys = flattenQueryParams(handlerOptions.queryParams)
   const {
-    cacheLength: serviceCacheLengthSeconds,
+    cacheLength: serviceDefaultCacheLengthSeconds,
     fetchLimitBytes,
   } = handlerOptions
 
   return (queryParams, match, end, ask) => {
     const reqTime = new Date()
 
-    setCacheHeaders({
-      cacheHeaderConfig,
-      serviceCacheLengthSeconds,
-      queryParams,
-      res: ask.res,
-    })
+    // `defaultCacheLengthSeconds` can be overridden by
+    // `serviceDefaultCacheLengthSeconds` (either by category or on a badge-
+    // by-badge basis). Then in turn that can be overridden by
+    // `serviceOverrideCacheLengthSeconds` (which we expect to be used only in
+    // the dynamic badge) but only if `serviceOverrideCacheLengthSeconds` is
+    // longer than `serviceDefaultCacheLengthSeconds` and then the `maxAge`
+    // query param can also override both of those but again only if `maxAge`
+    // is longer.
+    //
+    // When the legacy services have been rewritten, all the code in here
+    // will go away, which should achieve this goal in a simpler way.
+    //
+    // Ref: https://github.com/badges/shields/pull/2755
+    function setCacheHeadersOnResponse(res, serviceOverrideCacheLengthSeconds) {
+      setCacheHeaders({
+        cacheHeaderConfig,
+        serviceDefaultCacheLengthSeconds,
+        serviceOverrideCacheLengthSeconds,
+        queryParams,
+        res,
+      })
+    }
 
     analytics.noteRequest(queryParams, match)
 
@@ -123,6 +139,10 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
       const tooSoon = +reqTime - cached.time < cached.interval
       if (tooSoon || cached.dataChange / cached.reqs <= freqRatioMax) {
         const svg = makeBadge(cached.data.badgeData)
+        setCacheHeadersOnResponse(
+          ask.res,
+          cached.data.badgeData.cacheLengthSeconds
+        )
         makeSend(cached.data.format, ask.res, end)(svg)
         cachedVersionSent = true
         // We do not wish to call the vendor servers.
@@ -140,9 +160,13 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
         return
       }
       if (requestCache.has(cacheIndex)) {
-        const cached = requestCache.get(cacheIndex).data
-        const svg = makeBadge(cached.badgeData)
-        makeSend(cached.format, ask.res, end)(svg)
+        const cached = requestCache.get(cacheIndex)
+        const svg = makeBadge(cached.data.badgeData)
+        setCacheHeadersOnResponse(
+          ask.res,
+          cached.data.badgeData.cacheLengthSeconds
+        )
+        makeSend(cached.data.format, ask.res, end)(svg)
         return
       }
       ask.res.setHeader('Cache-Control', 'no-cache, no-store, must-revalidate')
@@ -155,6 +179,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
         extension = 'svg'
       }
       const svg = makeBadge(badgeData)
+      setCacheHeadersOnResponse(ask.res)
       makeSend(extension, ask.res, end)(svg)
     }, 25000)
 
@@ -256,6 +281,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) {
           requestCache.set(cacheIndex, updatedCache)
           if (!cachedVersionSent) {
             const svg = makeBadge(badgeData)
+            setCacheHeadersOnResponse(ask.res, badgeData.cacheLengthSeconds)
             makeSend(format, ask.res, end)(svg)
           }
         },

lib/request-handler.spec.js

@@ -25,6 +25,16 @@ function fakeHandler(queryParams, match, sendBadge, request) {
   sendBadge(format, badgeData)
 }
 
+function createFakeHandlerWithCacheLength(cacheLengthSeconds) {
+  return function fakeHandler(queryParams, match, sendBadge, request) {
+    const [, someValue, format] = match
+    const badgeData = getBadgeData('testing', queryParams)
+    badgeData.text[1] = someValue
+    badgeData.cacheLengthSeconds = cacheLengthSeconds
+    sendBadge(format, badgeData)
+  }
+}
+
 function fakeHandlerWithNetworkIo(queryParams, match, sendBadge, request) {
   const [, someValue, format] = match
   request('https://www.google.com/foo/bar', (err, res, buffer) => {
@@ -198,6 +208,62 @@ describe('The request handler', function() {
         expect(res.headers.get('cache-control')).to.equal('max-age=900')
       })
 
+      it('should set the expected cache headers on cached responses', async function() {
+        register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 900 } })
+
+        // Make first request.
+        await fetch(`${baseUrl}/testing/123.json`)
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        const expectedExpiry = new Date(
+          +new Date(res.headers.get('date')) + 900000
+        ).toGMTString()
+        expect(res.headers.get('expires')).to.equal(expectedExpiry)
+        expect(res.headers.get('cache-control')).to.equal('max-age=900')
+      })
+
+      it('should let live service data override the default cache headers with longer value', async function() {
+        camp.route(
+          /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/,
+          handleRequest(
+            { defaultCacheLengthSeconds: 300 },
+            (queryParams, match, sendBadge, request) => {
+              ++handlerCallCount
+              createFakeHandlerWithCacheLength(400)(
+                queryParams,
+                match,
+                sendBadge,
+                request
+              )
+            }
+          )
+        )
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        expect(res.headers.get('cache-control')).to.equal('max-age=400')
+      })
+
+      it('should not let live service data override the default cache headers with shorter value', async function() {
+        camp.route(
+          /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/,
+          handleRequest(
+            { defaultCacheLengthSeconds: 300 },
+            (queryParams, match, sendBadge, request) => {
+              ++handlerCallCount
+              createFakeHandlerWithCacheLength(200)(
+                queryParams,
+                match,
+                sendBadge,
+                request
+              )
+            }
+          )
+        )
+
+        const res = await fetch(`${baseUrl}/testing/123.json`)
+        expect(res.headers.get('cache-control')).to.equal('max-age=300')
+      })
+
       it('should set the expires header to current time + maxAge', async function() {
         register({ cacheHeaderConfig: { defaultCacheLengthSeconds: 0 } })
         const res = await fetch(`${baseUrl}/testing/123.json?maxAge=3600`)

server.js

@@ -13,8 +13,8 @@ const config = require('config').util.toObject()
 if (+process.argv[2]) {
   config.public.bind.port = +process.argv[2]
 }
-if (+process.argv[3]) {
-  config.public.bind.address = +process.argv[3]
+if (process.argv[3]) {
+  config.public.bind.address = process.argv[3]
 }
 
 console.log('Configuration:')

services/amo/amo.service.js

@@ -157,6 +157,10 @@ class AmoUsers extends LegacyService {
 //
 // Do not base new services on this code.
 class Amo extends LegacyService {
+  static get category() {
+    return 'other'
+  }
+
   static registerLegacyRouteHandler({ camp, cache }) {
     camp.route(
       /^\/amo\/(v|d|rating|stars|users)\/(.*)\.(svg|png|gif|jpg|json)$/,

services/appveyor/appveyor-base.js

@@ -3,10 +3,11 @@
 const Joi = require('joi')
 const BaseJsonService = require('../base-json')
 const { nonNegativeInteger } = require('../validators')
+const { isBuildStatus } = require('../../lib/build-status')
 
 const schema = Joi.object({
   build: Joi.object({
-    status: Joi.string().required(),
+    status: isBuildStatus,
     jobs: Joi.array()
       .items({
         testsCount: nonNegativeInteger,

services/appveyor/appveyor-ci.service.js

@@ -2,6 +2,8 @@
 
 const AppVeyorBase = require('./appveyor-base')
 
+const { renderBuildStatusBadge } = require('../../lib/build-status')
+
 module.exports = class AppVeyorCi extends AppVeyorBase {
   static get route() {
     return this.buildRoute('appveyor/ci')
@@ -25,17 +27,7 @@ module.exports = class AppVeyorCi extends AppVeyorBase {
   }
 
   static render({ status }) {
-    if (status === 'success') {
-      return { message: 'passing', color: 'brightgreen' }
-    } else if (
-      status !== 'running' &&
-      status !== 'queued' &&
-      status !== 'no builds found'
-    ) {
-      return { message: 'failing', color: 'red' }
-    } else {
-      return { message: status }
-    }
+    return renderBuildStatusBadge({ status })
   }
 
   async handle({ user, repo, branch }) {

services/appveyor/appveyor-tests.tester.js

@@ -3,19 +3,19 @@
 const Joi = require('joi')
 
 const isAppveyorTestTotals = Joi.string().regex(
-  /^(?:[0-9]+ (?:passed|skipped|failed)(?:, )?)+$/
+  /^[0-9]+ passed(, [0-9]+ failed)?(, [0-9]+ skipped)?$/
 )
 
 const isCompactAppveyorTestTotals = Joi.string().regex(
-  /^(?:[0-9]* ?(?:✔|✘|➟) ?[0-9]*(?:, | \| )?)+$/
+  /^✔ [0-9]+( \| ✘ [0-9]+)?( \| ➟ [0-9]+)?$/
 )
 
 const isCustomAppveyorTestTotals = Joi.string().regex(
-  /^(?:[0-9]+ (?:good|bad|n\/a)(?:, )?)+$/
+  /^[0-9]+ good(, [0-9]+ bad)?(, [0-9]+ n\/a)?$/
 )
 
 const isCompactCustomAppveyorTestTotals = Joi.string().regex(
-  /^(?:[0-9]* ?(?:💃|🤦‍♀️|🤷) ?[0-9]*(?:, | \| )?)+$/
+  /^💃 [0-9]+( \| 🤦‍♀️ [0-9]+)?( \| 🤷 [0-9]+)?$/
 )
 
 const t = (module.exports = require('../create-service-tester')())