[GitHub] Error message customisation for all status codes #1888
Conversation
Generated by 🚫 dangerJS |
PyvesB
changed the title
|
Hey, thanks for taking this on. I'll wait to leave comments 'till you're ready. There is a small conflict between this and #1890, so whichever goes in second will need a little bit of updating. |
PyvesB
changed the title
|
Okay, I think I'm mostly done here, I'm happy to hear some feedback and thoughts at this point! 👍 |
| assert.equal('inaccessible', badgeData.text[1]) | ||
| assert.equal('red', badgeData.colorscheme) | ||
| expect(checkErrorResponse(badgeData, 'something other than null', {})).to.be | ||
| .true |
There was a problem hiding this comment.
Is this on a new line because of prettier/eslint?
I feel like it would look better on 1 line, but i'm fine with it either way.
There was a problem hiding this comment.
Yes, I believe this is due to Prettier.
| checkErrorResponse({ text: [] }, null, { statusCode: 200 }) | ||
| ) | ||
| expect(checkErrorResponse({ text: [] }, null, { statusCode: 200 })).to.be | ||
| .false |
There was a problem hiding this comment.
Is this on a new line because of prettier/eslint?
I feel like it would look better on 1 line, but i'm fine with it either way.
There was a problem hiding this comment.
Yes, I believe this is due to Prettier.
lib/error-helper.js
Outdated
| @@ -1,19 +1,18 @@ | |||
| 'use strict' | |||
|
|
|||
| const { NotFound, InvalidResponse } = require('../services/errors') | |||
| const defaultErrorMessages = { | |||
| 404: 'not found', | |||
There was a problem hiding this comment.
Do you think it would be worth while adding a few more defaults?
401: Unauthorized
403: Forbidden
408: Request Timeout
429: Too Many Requests
500: Internal Server ErrorMaybe some others too, not really sure which would be the more common ones we would possibly encounter
There was a problem hiding this comment.
Is there an easy way to tell the CI server to run all service tests? If we start adding additional defaults, there is a risk of breaking existing tests.
There was a problem hiding this comment.
I don't believe there is, but it would likely fail due to timeouts.
Good point, i'm fine with just having 404 default for now, until we see a need for more.
lib/github-helpers.js
Outdated
| } | ||
| return standardCheckErrorResponse(badgeData, err, res, notFoundMessage) | ||
| errorMessages[404] = notFoundMessage | ||
| errorMessages[422] = notFoundMessage |
There was a problem hiding this comment.
Rather than mutate the inputs, it's probably better to create a new object:
{404: notFoundMessage, 422: notFoundMessage, ...errorMessages}Or maybe you want it in reverse:
{...errrorMessages, 404: notFoundMessage, 422: notFoundMessage}
|
I have rebased as #1890 was causing conflicts. Would @paulmelnikow or @RedSparr0w like to have one last look before I merge? |
|
It occurred to me that the new service base hadn't been updated, this should now be fixed. |
lib/github-helpers.spec.js
Outdated
| const badgeData = { text: [] } | ||
| expect( | ||
| checkErrorResponse(badgeData, null, { statusCode: 422 }, 'repo not found') | ||
| checkErrorResponse(badgeData, null, { statusCode: 422 }, 'user not found') |
There was a problem hiding this comment.
Should this statusCode be 404?
This pull request arises from this discussion and is inspired by @RedSparr0w's suggestion in those comments.
The idea is to allow callers of the
checkErrorResponseto provide a custom error message for all status codes, not just the 404 one.I also converted the remaining
assert.equalsusages toexpectin error-helper.spec.js.