Github Integration #42
Conversation
…students/batch-11-relayer into sourav-integration-github
…students/batch-11-relayer into sourav-integration-github
…students/batch-11-relayer into sourav-integration-github
Still inputs collection refactoring needs to be done
…students/batch-11-relayer into sourav-integration-github
|
|
||
| app.use(pino()); | ||
| app.use(express.json()); | ||
| app.use(express.urlencoded({ extended: false })); | ||
| app.use(express.static(path.join(__dirname, 'public'))); | ||
| app.use(express.static(publicFolder)); | ||
| // app.use('/api/v1', AuthenticationMiddleware); | ||
| app.set('view engine', 'ejs'); |
There was a problem hiding this comment.
Yes, as we are serving our OAuth pages from the backend server. Also, the authorization callbacks are directly handled by our backend. :)
|
|
||
| app.use(pino()); | ||
| app.use(express.json()); | ||
| app.use(express.urlencoded({ extended: false })); | ||
| app.use(express.static(path.join(__dirname, 'public'))); | ||
| app.use(express.static(publicFolder)); | ||
| // app.use('/api/v1', AuthenticationMiddleware); | ||
| app.set('view engine', 'ejs'); | ||
| app.set('views', path.join(__dirname, '/thirdparty/views')); | ||
| requireAll(modelDirPath); |
There was a problem hiding this comment.
all require should happen before any logic. Ideally, they should be the first thing
|
|
||
| const AppsCollection = mongoose.model('Apps'); | ||
| const getApps = (req, res) => { |
There was a problem hiding this comment.
add some line breaks, it is not readable
| console.log(req.body); | ||
| const createdApp = await AppsCollection.create(req.body); | ||
| res.send(createdApp); | ||
| const generatedResponse = responseLib.generateResponse(false, actionStatus.SUCCESS, |
There was a problem hiding this comment.
pass in an object with values instead of multiple params. It's easier to update/ changes params later.
Also the code is more readable
|
|
||
| const getEventDetails = (req, res) => { | ||
| let eventDetails; | ||
| for (const event of IntegrationConfig[req.params.appName].Events) { |
There was a problem hiding this comment.
what if IntegrationConfig[req.params.appName].Events is undefined/ null?
What is appName is not present in params. The server will crash. Please handle the errors
| @@ -8,7 +8,7 @@ const slackWebHookListener = async (event, authedUsers) => { | |||
| }; | |||
|
|
|||
| const gitWebHookListener = (event) => { | |||
| console.log(event); | |||
| // console.log(event); | |||
| @@ -1,7 +1,7 @@ | |||
| import jwt from 'jsonwebtoken'; | |||
| import time from './timeLib'; | |||
|
|
|||
| const { JWT_SECRET } = process.env; | |||
| const JWT_SECRET = 'zF9?nCmaZH&?fF9'; | |||
There was a problem hiding this comment.
This is absolutely wrong.
Putting in secrets in code is not safe
|
|
||
| eventEmitter.on('Github:Authorize', async (payload) => { | ||
| // eslint-disable-next-line camelcase | ||
| const { state, installation_id, code } = payload; |
There was a problem hiding this comment.
you can have camelcase using installation_id: installationId
| method: 'POST', | ||
| headers: { Accept: 'application/json' }, | ||
| json: { | ||
| code, |
| module.exports = { | ||
| path: '/app', | ||
| path: '/api/v1', |
There was a problem hiding this comment.
assuming you are gonna use path to append to route so you can call something like this
/api/v1/your-path
But this way you are taking away all the benefits. versioning is there for backward compatibility. Once you rename the variable to /api/v2 all previous routes are gone.
Correct me if I made a wrong assumption.
Pull Request Template
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes #43
Type of change
How Has This Been Tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist