[GitHub] Fix undefined badges #1816

PyvesB · 2018-07-30T19:51:30Z

This pull request addresses issue #1768.

I noticed that many other GitHub badges were suffering from this same problem, but for now I've only fixed it for the "issues" badge and will wait for some initial feedback before tackling the other ones. 😉

shields-ci · 2018-07-30T19:52:35Z

	Messages
📖	✨ Thanks for your contribution to Shields, @PyvesB!

Generated by 🚫 dangerJS

RedSparr0w

Changes look good!
👍

RedSparr0w · 2018-07-30T23:05:18Z

this is a rather strange way for GitHub to report when a repo is not found:

{
  "message": "Validation Failed",
  "errors": [
    {
      "message": "The listed users and repositories cannot be searched either because the resources do not exist or you do not have permission to view them.",
      "resource": "Search",
      "field": "q",
      "code": "invalid"
    }
  ],
  "documentation_url": "https://developer.github.com/v3/search/"
}

Do you think it would be worth checking the message field matches and isn't trying to report a different error?

PyvesB · 2018-07-31T17:34:47Z

I get your point and I did think about that alternative as well, but I felt slightly less enthusiastic about it:

to avoid parsing the response body several times, we would probably have to parse it before we check the response codes and pass it as a parameter to githubCheckErrorResponse, which seems to me as the wrong ordering of operations. Also, would this lead us to more trouble when we migrate the GitHub services to the new refactored service structure?
It seems quite clumsy, we would have to iterate through all the elements inside the errors array and look for a message that is more likely to change than a status code.

Does this make sense?

RedSparr0w · 2018-07-31T20:58:56Z

No worries, I think it should be fine as is 👍

PyvesB · 2018-07-31T21:29:22Z

I'll have a go at fixing the other badges in the coming days, we can think about it some more in the meantime and see how it looks once everything is done. 😉

PyvesB · 2018-08-05T10:35:17Z

services/github/github.tester.js

@@ -74,7 +74,7 @@ t.create('License - API rate limit exceeded')
      message: "API rate limit exceeded for 123.123.123.123. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)",
      documentation_url: "https://developer.github.com/v3/#rate-limiting"
  }))
-  .expectJSON({ name: 'license', value: 'inaccessible', colorB: colorsB.lightgrey });
+  .expectJSON({ name: 'license', value: 'invalid', colorB: colorsB.lightgrey });


This change is due to the default error-helper.js handling. Wouldn't inaccessible be more appropriate for 403 responses for all badges?

It could be worth updating the error-helper to something similar to this:

defaultErrorMessages = { 404: 'not found', 418: "I'm a teapot", }; const checkErrorResponse = function(badgeData, err, res, errorMessage = 'not found') { if (typeof errorMessage === 'string') errorMessage = {...defaultErrorMessages, ...{404: errorMessage}} else errorMessage = {...defaultErrorMessages, ...errorMessage}; if (err != null) { badgeData.text[1] = 'inaccessible'; badgeData.colorscheme = 'red'; return true; } else if (errorMessage[res.statusCode] !== undefined) { badgeData.text[1] = errorMessage[res.statusCode]; badgeData.colorscheme = 'lightgrey'; return true; } else if (res.statusCode !== 200) { badgeData.text[1] = 'invalid'; badgeData.colorscheme = 'lightgrey'; return true; } else { return false; } }; //Usage: checkErrorResponse(badgeData, err, res, {403: 'repo not found'}); // 403: repo not found checkErrorResponse(badgeData, err, res, 'repo not found'); // 404: repo not found checkErrorResponse(badgeData, err, res, {403: 'repo not found', 404: 'repo not found'}); // 403, 404: repo not found

That way if services return a strange error code, we can still set a relevant error message.

inaccessible would be a nice way to convey a permission problem. For our badges, thoguh, it usually means the service can't be reached.

Maybe not authorized?

Yes, "not authorized" may be better suited for 403 response codes. "access denied" is a good alternative as well. I'm leaning towards a separate PR though, as this has not got much to do with "undefined" in GitHub badges.

paulmelnikow · 2018-08-07T20:47:04Z

Is this waiting for another review? Let me know if you need me to take another look.

PyvesB · 2018-08-07T21:32:42Z

Yes, I wouldn't mind someone taking a look at the final result! 👍

paulmelnikow · 2018-08-08T03:22:50Z

lib/github-helpers.js

@@ -10,10 +11,19 @@ function checkStateColor(s) {
  return { pending: 'dbab09', success: '2cbe4e', failure: 'cb2431', error: 'cb2431' }[s];
 }

+function checkErrorResponse(badgeData, err, res, notFoundMessage = 'repo not found') {
+  const isError = standardCheckErrorResponse(badgeData, err, res, notFoundMessage);
+  if (res && res.statusCode === 422) {


It's not completely clear whether we need the side effect of standardCheckErrorResponse in this case. If we don't, would it be clearer to move this if to the top of the function, and then return standardCheckErrorResponse(...)?

paulmelnikow · 2018-08-08T03:23:22Z

lib/github-helpers.spec.js

+'use strict';
+
+const chai = require('chai');
+const { assert } = chai;


Could you use the expect forms like most of our tests?

Done, we should probably revisit error-helper.spec.js as well at some point, and possibly some other specs. 😉

paulmelnikow · 2018-08-08T03:23:40Z

lib/github-helpers.spec.js

+const { assert } = chai;
+const { checkErrorResponse } = require('./github-helpers');
+
+chai.use(require('chai-as-promised'));


This could be removed.

shields-deployment · 2018-08-08T19:58:01Z

This pull request was merged to master branch. Now this change is waiting for deployment.
Deploys usually happen every few weeks. After deployment changes are copied to gh-pages branch.

This badge displays deployment status:

Drafted implementation for issues badge

dfa2cf5

PyvesB added bug Bugs in badges and the frontend service-badge New or updated service badge labels Jul 30, 2018

RedSparr0w approved these changes Jul 30, 2018

View reviewed changes

PyvesB changed the title ~~[GitHub] Fix undefined badges~~ [GitHub] Fix undefined badges - WIP Jul 31, 2018

paulmelnikow approved these changes Aug 1, 2018

View reviewed changes

PyvesB added 2 commits August 5, 2018 10:47

Extended fix to all GitHub badges

04e69ae

Fixed tests

5fb061a

PyvesB force-pushed the undefined_github branch from e8725df to 5fb061a Compare August 5, 2018 10:16

PyvesB commented Aug 5, 2018

View reviewed changes

PyvesB added 4 commits August 5, 2018 11:44

Drafted implementation for issues badge

b5d3d1c

Extended fix to all GitHub badges

ec34e10

Fixed tests

6a9683a

Rebased from master

4247937

PyvesB force-pushed the undefined_github branch from 210b715 to 4247937 Compare August 5, 2018 10:49

PyvesB changed the title ~~[GitHub] Fix undefined badges - WIP~~ [GitHub] Fix undefined badges Aug 5, 2018

paulmelnikow approved these changes Aug 8, 2018

View reviewed changes

PyvesB added 3 commits August 8, 2018 18:54

Updated tests

07600a2

Rearranged checkErrorResponse code

e72989d

Merge branch 'master' into undefined_github

8072f11

paulmelnikow approved these changes Aug 8, 2018

View reviewed changes

PyvesB merged commit e1affea into badges:master Aug 8, 2018

PyvesB mentioned this pull request Aug 10, 2018

[GitHub] Error message customisation for all status codes #1888

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GitHub] Fix undefined badges #1816

[GitHub] Fix undefined badges #1816

PyvesB commented Jul 30, 2018

shields-ci commented Jul 30, 2018 •

edited

Loading

RedSparr0w left a comment

RedSparr0w commented Jul 30, 2018

PyvesB commented Jul 31, 2018

RedSparr0w commented Jul 31, 2018

PyvesB commented Jul 31, 2018

PyvesB Aug 5, 2018 •

edited

Loading

RedSparr0w Aug 6, 2018

paulmelnikow Aug 7, 2018

PyvesB Aug 7, 2018 •

edited

Loading

paulmelnikow commented Aug 7, 2018

PyvesB commented Aug 7, 2018

paulmelnikow Aug 8, 2018

paulmelnikow Aug 8, 2018

PyvesB Aug 8, 2018 •

edited

Loading

paulmelnikow Aug 8, 2018

shields-deployment bot commented Aug 8, 2018

[GitHub] Fix undefined badges #1816

[GitHub] Fix undefined badges #1816

Conversation

PyvesB commented Jul 30, 2018

shields-ci commented Jul 30, 2018 • edited Loading

RedSparr0w left a comment

Choose a reason for hiding this comment

RedSparr0w commented Jul 30, 2018

PyvesB commented Jul 31, 2018

RedSparr0w commented Jul 31, 2018

PyvesB commented Jul 31, 2018

PyvesB Aug 5, 2018 • edited Loading

Choose a reason for hiding this comment

RedSparr0w Aug 6, 2018

Choose a reason for hiding this comment

paulmelnikow Aug 7, 2018

Choose a reason for hiding this comment

PyvesB Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

paulmelnikow commented Aug 7, 2018

PyvesB commented Aug 7, 2018

paulmelnikow Aug 8, 2018

Choose a reason for hiding this comment

paulmelnikow Aug 8, 2018

Choose a reason for hiding this comment

PyvesB Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

paulmelnikow Aug 8, 2018

Choose a reason for hiding this comment

shields-deployment bot commented Aug 8, 2018

shields-ci commented Jul 30, 2018 •

edited

Loading

PyvesB Aug 5, 2018 •

edited

Loading

PyvesB Aug 7, 2018 •

edited

Loading

PyvesB Aug 8, 2018 •

edited

Loading