Controller class upgrade #33
Conversation
…ngle leaking global variable in config.js
|
No labels for in progress pull requests in this repository, maybe something like "do not merge" would be useful. @jupe |
app/controllers/campaigns.js
Outdated
| var defaultCtrl = new DefaultController(Campaign, 'Campaign'); | ||
| class Controller extends DefaultController { | ||
| constructor() { | ||
| super(mongoose.model('Campaign'), 'Campaign'); | ||
|
|
There was a problem hiding this comment.
perhaps it could give model name and super class get actual model object
app/controllers/campaigns.js
Outdated
|
|
||
| return this; | ||
| }; | ||
| this.paramFormat = DefaultController.format(); |
There was a problem hiding this comment.
perhaps these could be in super class as well
app/controllers/index.js
Outdated
| console.log(Model.name); | ||
|
|
||
| super(); | ||
| this.Model = Model; |
There was a problem hiding this comment.
do getter for this and move original object to private member
…js, removed rendundant format and also defined mongoose.Promise as the default implementation is deprecated.
app/controllers/index.js
Outdated
| } | ||
| } | ||
|
|
||
| find(req, res) { | ||
| this._model.query(req.query, (error, list) => { | ||
| this._model.find(req.query, (error, list) => { |
There was a problem hiding this comment.
why this change? all models are extended with mongoose-query -plugin (like this) so there should be also model.query()-api available. This is important because find doesn't support features we need from TMI API..
There was a problem hiding this comment.
Thought it was a deprecated call or something similar since I couldn't find references to it in the mongoose Model documentation. Extending seems to be broken at the moment. Looking into it.. Did not extend mock schema used in testing with proper plugins, resolved.
…to default implementation for debugging purposes.
…o cosmetic changes for index.js tests
… in loans.js, also slightly reformatted console logging made by all unit tests
…ctures away from actual tests to improve readability and better support for scaling
| Testcase.ensureIndexes(function (err) { | ||
| if (err) return handleError(err); | ||
| }); | ||
| // TODO figure out how to synchronously ensureIndexes so we do not close db connection before the process is completed |
There was a problem hiding this comment.
its still better to be asynchronous. Solution could be that we emit some common event bus message that indexes is ensured before allow clients to be connected to API's... -> "kind of boot sequence"...
… Used existing colors library to make logs clearer
… shoulds into expectations + minor formatting
… with defining specific mongo version, trying that.
…ecedes item saving to preserve generated _id
app/controllers/resources.js
Outdated
| @@ -72,26 +77,25 @@ class ResourcesController extends DefaultController { | |||
| static allocMultiple(req, res) { | |||
| req.Resource.allocateResources(req.body, (error, allocated) => { | |||
| if (error) { | |||
| res.status(404).json(error); | |||
| res.status(404).json({ error }); | |||
| request({ | ||
| followAllRedirects: true, | ||
| url: url, | ||
| url, |
There was a problem hiding this comment.
Eslint by default prefers new Object notation where variables can be initialized to objects with their own name as key. Still, I agree that mixing the two notation methods in a single object looks weird.
See: - new notations in ECMAScript 2015 -
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
app/controllers/authentication.js
Outdated
| winston.info('Github auth: checkOrganization error'); | ||
| callback({ status: 500, msg: err.toString() }); | ||
| } else { | ||
| const belongsOrg = _.find(response.body, { login: nconf.get('github').organization }); |
There was a problem hiding this comment.
move nconf.get..organization to top of the file as const.
add env.example.json:
"GITHUB_ORG": "ORGANIZATION",
"GITHUB_ADMINTEAM": "ADMIN-TEAM",
app/controllers/authentication.js
Outdated
| if (belongsOrg) { | ||
| callback(null, accessToken, headers, profile); | ||
| } else { | ||
| winston.info('Github auth: user not in organization'); |
There was a problem hiding this comment.
use warning level + include admin team name to log
app/controllers/authentication.js
Outdated
| callback({ status: 500, msg: err.toString() }); | ||
| } else { | ||
| const isAdmin = _.find(response.body, (team) => { | ||
| return team.name === nconf.get('github').adminTeam && team.organization.login === nconf.get('github').organization; |
There was a problem hiding this comment.
move nconf.get..:s to top of file as const
app/controllers/authentication.js
Outdated
| var headers = { 'User-Agent': 'Satellizer' }; | ||
| request.get({ url: accessTokenUrl, qs: params }, (err, response, accessToken) => { | ||
| if (err) { | ||
| winston.info('Github auth: authorization error'); |
app/controllers/authentication.js
Outdated
| const getProfile = (accessToken, headers, callback) => { | ||
| request.get({ url: userApiUrl, qs: accessToken, headers, json: true }, (err, response, profile) => { | ||
| if (err) { | ||
| winston.info('Github auth: getProfile error'); |
app/controllers/authentication.js
Outdated
| winston.info('Github auth: getProfile error'); | ||
| callback({ status: 500, msg: err.toString() }); | ||
| } else if (!err && response.statusCode !== 200) { | ||
| winston.info('Github auth: bad profile response'); |
app/controllers/authentication.js
Outdated
| winston.info('Github auth: bad profile response'); | ||
| callback({ status: 409, msg: 'Did not get github profile.' }); | ||
| } else if (!profile.email) { | ||
| winston.info('Github auth: no email error'); |
There was a problem hiding this comment.
warn level. This is by the way problematic: GitHub default configuration use email as private so its not visible here. We have to change email as optional field probably..
There was a problem hiding this comment.
Good point but we still need some user-friendly way to search for users(before it was email). Github username could work, but in any case, one or more fields of user needs to be searchable.
app/controllers/authentication.js
Outdated
| const checkOrganization = (accessToken, headers, profile, callback) => { | ||
| request.get({ url: `${userApiUrl}/orgs`, qs: accessToken, headers, json: true }, (err, response) => { | ||
| if (err) { | ||
| winston.info('Github auth: checkOrganization error'); |
…top of the script
Status
FINISHED
Description
This PR refactors controllers to use Class architecture.
Todos
controllers
cleanup
Functional testingDocumentationImpacted areas