Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 #1142

paulmelnikow · 2017-10-09T21:12:53Z

Add tests to request-handler to prepare for some tweaks to caching for [badge/dynamic/json] User defined JSON source badge #820
Clean up code in request-handler: renames, DRY, arrows, imports
Allow for clean shutdown of setInterval code. This requires the ability to cancel autosaving.
Upgrade to Mocha 4, and clean up so the process exits on its own (see mocha 4 doesn't exit unlike mocha 3 mochajs/mocha#3044)
Better encapsulate analytics

- Rename data -> queryParams for readability - Factor out duplicated code - Remove spurious return - Better encapsulate analytics

Upgrade to Mocha 4 which does not forcibly quit the process when tests complete mochajs/mocha#3044

# Conflicts: # package.json

paulmelnikow · 2017-10-17T22:14:03Z

@RedSparr0w I would really like to get this merged. Would you be up for reviewing it? Feel free to ask if any of it is unclear or if you have questions.

RedSparr0w · 2017-10-17T23:44:39Z

Had a look over it, only noticed 1 thing (will add a note), other than that all looks good to me.

RedSparr0w · 2017-10-17T23:46:57Z

lib/request-handler.js

@@ -109,7 +99,8 @@ function handleRequest (vendorRequestHandler) {
      }
      options.headers = options.headers || {};
      options.headers['User-Agent'] = options.headers['User-Agent'] || 'Shields.io';
-      return request(options, function(err, res, body) {
+
+      request(options, (err, res, body) => {


Looks like we need to either keep the return or update the parts in server.js where it is required.

Edit: Looks like it would be better updating server.js as it only appears on Lines 2422 & 2467

Good catch.

request(apiUrl, function(err, res, body) { if (err != null) { badgeData.text[1] = 'invalid'; sendBadge(format, badgeData); return; } try { ... sendBadge(format, badgeData); } catch(e) { badgeData.text[1] = 'malformed'; sendBadge(format, badgeData); } }).on('error', function(e) { badgeData.text[1] = 'inaccessible'; sendBadge(format, badgeData); });

It seems to me the .on('error', ... blocks for request's streaming API should simply be removed. request also provides response errors to the callback, and the callbacks are handling them.

Daniel15 · 2017-10-18T01:14:47Z

lib/analytics.js

@@ -14,16 +14,32 @@ if (process.env.REDISTOGO_URL) {
 }

 let analytics = {};
+let autosaveIntervalId;


Nit: Call this autoSaveIntervalId (capital S) for consistency?

I changed everything to lowercase. AFAIC autosave is one word. Changed autoLoad to load which seems to describe it better.

Daniel15 · 2017-10-18T01:15:46Z

lib/analytics.js

-setInterval(function analyticsAutoSave() {
+
+function performAutosave() {
+  const contents = JSON.stringify(analytics);
  if (useRedis) {


I feel like this should use the adapter pattern rather than hard-coding Redis vs plain text file. That could definitely be refactored separately though!

Yea, agreed. We're not even using Redis in production so I don't know if this code works. We could remove it, though it would make more sense to get some tests around it, since it makes our hosting portable to Heroku for people who want to self-host. Definitely agreed it would be nice to refactor it.

Daniel15 · 2017-10-18T01:16:12Z

lib/analytics.js

-}, analyticsAutoSavePeriod);
+}
+
+function scheduleAutosaving() {


We should be consistent about autosave (lowercase S) vs autoSave (capital S) in this file.

Daniel15 · 2017-10-18T01:17:06Z

lib/analytics.js

+}
+
+function setRoutes (server) {
+  server.ajax.on('analytics/v1', function(json, end) { end(analytics); });


Arrow function?

server.ajax.on('analytics/v1', (json, end) => { end(analytics); });

Daniel15 · 2017-10-18T01:17:43Z

lib/request-handler.js

@@ -109,7 +99,8 @@ function handleRequest (vendorRequestHandler) {
      }
      options.headers = options.headers || {};
      options.headers['User-Agent'] = options.headers['User-Agent'] || 'Shields.io';
-      return request(options, function(err, res, body) {
+
+      request(options, (err, res, body) => {


Daniel15 · 2017-10-18T01:18:03Z

lib/request-handler.spec.js

+      '/testing/123.svg?foo=2'
+    ).then(() => { assert.equal(handlerCallCount, 1); });
+  });
+});


Nice! I like this.

Daniel15 · 2017-10-18T01:19:05Z

Looks pretty nice. Just left some minor comments inline.

paulmelnikow · 2017-10-18T02:04:15Z

Thanks /Dani[ae]l/!

paulmelnikow added 2 commits October 9, 2017 17:08

Test caching request + minor cleanup

e271ffc

- Rename data -> queryParams for readability - Factor out duplicated code - Remove spurious return - Better encapsulate analytics

Minor fixes + cleanup

ed27525

paulmelnikow added the core Server, BaseService, GitHub auth, Shared helpers label Oct 9, 2017

paulmelnikow mentioned this pull request Oct 9, 2017

[badge/dynamic/json] User defined JSON source badge #820

Merged

paulmelnikow changed the title ~~Test caching request + minor cleanup~~ Add test for caching request + minor cleanup Oct 9, 2017

paulmelnikow added 4 commits October 10, 2017 13:39

Refactor analytics and github-auth to allow for a clean shutdown

8998867

Upgrade to Mocha 4 which does not forcibly quit the process when tests complete mochajs/mocha#3044

Merge branch 'master' into test-caching-request

3d6da59

Clean diff

66141d1

Clean diff

905baae

paulmelnikow changed the title ~~Add test for caching request + minor cleanup~~ Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 Oct 10, 2017

Merge branch 'master' into test-caching-request

ca45fbd

# Conflicts: # package.json

paulmelnikow requested a review from Daniel15 October 12, 2017 18:48

paulmelnikow mentioned this pull request Oct 12, 2017

Turn on prettier (except repo root) #1167

Merged

3 tasks

RedSparr0w reviewed Oct 17, 2017

View reviewed changes

Daniel15 approved these changes Oct 18, 2017

View reviewed changes

paulmelnikow added 4 commits October 17, 2017 21:40

Merge branch 'master' into test-caching-request

b56587e

rm useless .on(‘error’) blocks

bde32ef

Analytics caps consistency

8f591bc

Arrow fun fun functions

550be31

paulmelnikow merged commit dc44ba7 into badges:master Oct 18, 2017

paulmelnikow deleted the test-caching-request branch October 18, 2017 02:01

This was referenced Nov 2, 2017

"GitHub OAuth authentication failed to provide a code" when adding token #1243

Closed

Github badges are intermittently inaccessible #1245

Closed

dependabot-preview bot mentioned this pull request Apr 15, 2019

Bump eslint-plugin-import from 2.16.0 to 2.17.1 #3313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 #1142

Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 #1142

paulmelnikow commented Oct 9, 2017 •

edited

Loading

paulmelnikow commented Oct 17, 2017

RedSparr0w commented Oct 17, 2017

RedSparr0w Oct 17, 2017 •

edited

Loading

Daniel15 Oct 18, 2017

paulmelnikow Oct 18, 2017

Daniel15 Oct 18, 2017

paulmelnikow Oct 18, 2017

Daniel15 Oct 18, 2017

paulmelnikow Oct 18, 2017

Daniel15 Oct 18, 2017

Daniel15 Oct 18, 2017

Daniel15 Oct 18, 2017

Daniel15 Oct 18, 2017

Daniel15 commented Oct 18, 2017

paulmelnikow commented Oct 18, 2017

Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 #1142

Clean up request-handler, github-auth, and analytics; upgrade to Mocha 4 #1142

Conversation

paulmelnikow commented Oct 9, 2017 • edited Loading

paulmelnikow commented Oct 17, 2017

RedSparr0w commented Oct 17, 2017

RedSparr0w Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Daniel15 commented Oct 18, 2017

paulmelnikow commented Oct 18, 2017

paulmelnikow commented Oct 9, 2017 •

edited

Loading

RedSparr0w Oct 17, 2017 •

edited

Loading