Skip to content
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

Validation module #109

Merged
merged 5 commits into from
Dec 27, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions lib/controller/prefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,20 @@ const prefsInput = require('../cli/input/prefs');
const prefsOutput = require('../cli/output/prefs');
const prefsModel = require('../model/prefs');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is prefs the only command to suffer from the validation problem? What about other commands. Please check again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Top of my head, I can forsee the following invalid inputs.

  • Invalid hostname and or port
  • Invalid login credentials - deduced without even contacting the gitlab. For example, usernames such as 123 are obviously not valid. We can look at the validity of usernames for GitLab and repeat the same logic here.

We need validations for the above scenarios too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sir since we have a dual nature of input, these validations can only be done after the input it taken from the stdin when necessary. This happens in the input/prefs module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is prefs the only command to suffer from the validation problem? What about other commands. Please check again.

For now only prefs has the nested command model which is not taken care by caporal. No other commands have this issue right now. I made the validation module to be extensible, so can be extended if need arises in any of the other modules.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed about the validation functions feature provided by caporal. Is it better to leave the validation functions inside the controller modules or to have controllers/validators/<controller-name.js>?
Putting the non-trivial validators in controllers/validators/<controller-name.js> may improve the code design.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, the nested commands are all lined up in the TODO feature list. So, the validators.js is not just one single file; we might need a lot of them.


const validator = require('./validation');

const onPrefs = async (args, options, logger) => {
logger.moduleLog('info', 'Prefs', 'Prefs command invoked for');
logger.moduleLog('info', 'Prefs', args.preference);
let changePrefEvent = await prefsInput.getInput(args, options);
logger.moduleLog('debug', 'Prefs', 'Event for the prefs command called');
logger.moduleLog('debug', 'Prefs', changePrefEvent);
changePrefEvent = await prefsModel.storePrefs(changePrefEvent);
prefsOutput.sendOutput(changePrefEvent);
if (validator.prefs(args, options)) {
let changePrefEvent = await prefsInput.getInput(args, options);
logger.moduleLog('debug', 'Prefs', 'Event for the prefs command called');
logger.moduleLog('debug', 'Prefs', changePrefEvent);
changePrefEvent = await prefsModel.storePrefs(changePrefEvent);
prefsOutput.sendOutput(changePrefEvent);
} else {
logger.moduleLog('error', 'Prefs', 'Invalid command');
}
};

const addTo = (program) => {
Expand Down
27 changes: 27 additions & 0 deletions lib/controller/validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
const prefsOutput = require('../cli/output/prefs');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed on doing the validation at the controller. So, the location of this module is appropriate. However, we are going to need validation for all the commands, not just preferences. Hence, we may need to think about a better structure (perhaps validation/prefs.js to include validators for all the controllers).

Another possibility is to put small validators in the respective controllers themselves and only push reasonably big / complex validators to a separate module.


const supportedServers = ['ms', 'gitlab'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These constants are better derived from config.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure sir

const supportedCommand = ['show', 'changelang', 'changeserver', 'logger'];

const prefs = (args, options) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can ponder a bit on appropriate guidelines for the use of arrow functions in this project. If you can read a bit, and create a wiki page while also quoting good quality references, we can stick to the created guidelines everywhere in the project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sir. I'll read up about it.

if (supportedCommand.indexOf(args.preference) === -1) {
prefsOutput.sendOutput({ name: 'invalid_command' });
return false;
}
if (args.preference === 'changeserver') {
if (options.type && supportedServers.indexOf(options.type) === -1) {
prefsOutput.sendOutput({
name: 'invalid_server',
details: {
supportedServers,
},
});
return false;
}
}
return true;
};

module.exports = {
prefs,
};
27 changes: 27 additions & 0 deletions test/unit/lib/controller/prefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const prefsInput = require('../../../../lib/cli/input/prefs');
const prefsOutput = require('../../../../lib/cli/output/prefs');
const prefsModel = require('../../../../lib/model/prefs');
const prefsController = require('../../../../lib/controller/prefs');
const validator = require('../../../../lib/controller/validation');

chai.use(sinonChai);
chai.should();
Expand All @@ -25,13 +26,15 @@ describe('For prefs controller', function () {
});

it('should call the prefs action of program with right arguments when command is valid', testPrefsValid);
it('should exits the program when command is invalid', testPrefsInvalid);
});

/* eslint-disable max-lines-per-function */
function testPrefsValid(done) {
const mockprefsInput = sandbox.mock(prefsInput);
const mockprefsOutput = sandbox.mock(prefsOutput);
const mockprefsModel = sandbox.mock(prefsModel);
const mockValidator = sandbox.mock(validator);

const testMsPort = 8999;

Expand All @@ -48,6 +51,9 @@ function testPrefsValid(done) {
}).resolves(changedPrefs);
mockprefsModel.expects('storePrefs').withExactArgs(changedPrefs).resolves(changedPrefs);
mockprefsOutput.expects('sendOutput').withExactArgs(changedPrefs);
mockValidator.expects('prefs').withExactArgs({ preference: 'changeserver' }, {
type: 'ms', host: 'abc', port: '8999', lang: undefined, maxsize: undefined, blacklist: undefined,
}).returns(true);

prefsController.addTo(program);

Expand All @@ -61,6 +67,27 @@ function testPrefsValid(done) {
mockprefsInput.verify();
mockprefsOutput.verify();
mockprefsModel.verify();
mockValidator.verify();
done();
}, 0);
}

function testPrefsInvalid(done) {
const mockValidator = sandbox.mock(validator);
mockValidator.expects('prefs').withExactArgs({ preference: 'changeserver' }, {
type: 'github', host: 'abc', port: '8999', lang: undefined, maxsize: undefined, blacklist: undefined,
}).returns(false);

prefsController.addTo(program);

program.exec(['prefs', 'changeserver'], {
type: 'github',
host: 'abc',
port: '8999',
});

setTimeout(() => {
mockValidator.verify();
done();
}, 0);
}
68 changes: 68 additions & 0 deletions test/unit/lib/controller/validation.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
const chai = require('chai');
const sinon = require('sinon');
const sinonChai = require('sinon-chai');

const validator = require('../../../../lib/controller/validation');
const prefsOutput = require('../../../../lib/cli/output/prefs');

chai.should();
chai.use(sinonChai);

const sandbox = sinon.createSandbox();

describe('for validation', function () {
afterEach(function () {
sandbox.restore();
});

it('should return true on correct command', testValidCommand);
it('should call prefs output with correct arguments on invalid command', testInvalidPrefsCommand);
it('should call prefs output with correct arguments on invalid server', testInvalidServer);
});

function testValidCommand(done) {
const testArgs = {
preference: 'changeserver',
};
const testOptions = {
type: 'ms',
};

validator.prefs(testArgs, testOptions).should.be.equal(true);
done();
}

function testInvalidPrefsCommand(done) {
const mockprefsOutput = sandbox.mock(prefsOutput);
const testArgs = {
preference: 'represent',
};

mockprefsOutput.expects('sendOutput').withExactArgs({ name: 'invalid_command' });

validator.prefs(testArgs).should.be.equal(false);
mockprefsOutput.verify();
done();
}

function testInvalidServer(done) {
const mockprefsOutput = sandbox.mock(prefsOutput);
const testArgs = {
preference: 'changeserver',
};
const testOptions = {
type: 'github',
};
const supportedServers = ['ms', 'gitlab'];

mockprefsOutput.expects('sendOutput').withExactArgs({
name: 'invalid_server',
details: {
supportedServers,
},
});

validator.prefs(testArgs, testOptions).should.be.equal(false);
mockprefsOutput.verify();
done();
}