-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PP-12395: Create API key functionality #4394
Conversation
Creating an API key consists of two pages: the "API key name" and "New API key" pages. Validation of the name/description and cypress test will be done in the next PRs.
585e1bd
to
c2a8d0c
Compare
app/services/api-keys.service.js
Outdated
* @param {string} description | ||
* @param {string} email - the user email | ||
* @param {'API' | 'PRODUCTS'} tokenSource - The type of the token (must match one of TOKEN_TYPE values). | ||
* @returns {string} the new api key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a tad pedantic but this returns a Promise<String>
as it's async
response(req, res, 'simplified-account/settings/api-keys/new-api-key-details', { | ||
description, | ||
apiKey: newApiKey, | ||
backToApiKeysLink: formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, req.service.externalId, req.account.type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's better or worse to allow for customising the back link text, e.g. if a backLinkText
variable is provided in the context then it will override the text. I'll leave that up to you, just a thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth doing if I encounter more back links that don't have the text "Back". Going to leave this for now.
}) | ||
|
||
it('should pass context data to the response method', () => { | ||
expect(mockResponse.args[0][3]).to.have.property('backLink').to.equal( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nice stuff you can do with sinon-chai
expect(mockResponse.args[0][3]).to.have.property('backLink').to.equal( | |
expect(mockResponse).to.have.been.calledWith(sinon.match.any, sinon.match.any, sinon.match.any, { | |
backLink: '...' | |
}) |
but again just a suggestion, up to you which you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might as well delete this test and amend the one above it:
it('should pass req, res, template path and context to the response method', () => {
expect(mockResponse).to.have.been.calledWith(req, res, 'simplified-account/settings/api-keys/api-key-name',
{ backLink: formatSimplifiedAccountPathsFor(paths.simplifiedAccount.settings.apiKeys.index, SERVICE_ID, ACCOUNT_TYPE) })
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Creating an API key consists of two pages: the "API key name" and "New API key" pages.
Validation of the name/description and cypress test will be done in the next PRs.
API key name page:
New API key page: