Skip to content

Commit

Permalink
Fix oppia#6418: Added Validation checks to avoid infinite loops expla…
Browse files Browse the repository at this point in the history
…ined in issue and comments (oppia#6520)

* Added Validation checks to avoid infinite loops explained in issue and my comments following the issue, and added frontend tests for the same, to verify that the checks work perfectly

* extended the validation check to make sure correct options in case of 'equal' rule is more than max selections

* Fix nit, and remove unnessesory modifications in check for containsAtleastOneOf rule

* Forgot to run lint tests, fixed lint issues

* .

* Minor nits

* Set Copyright date to 2014

* make tests run
  • Loading branch information
amey-kudari authored and vibhor98 committed Apr 11, 2019
1 parent 0b4e6cf commit 74aed34
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ oppia.factory('ItemSelectionInputValidationService', [
var handledAnswers = seenChoices.map(function(item) {
return false;
});

var minAllowedCount =
customizationArgs.minAllowableSelectionCount.value;
var maxAllowedCount =
customizationArgs.maxAllowableSelectionCount.value;

Expand Down Expand Up @@ -177,8 +180,32 @@ oppia.factory('ItemSelectionInputValidationService', [
'rule must include at least 2 options.')
});
}
} else if (rule.type === 'Equals') {
if (minAllowedCount > ruleInputs.length ||
maxAllowedCount < ruleInputs.length) {
warningsList.push({
type: WARNING_TYPES.ERROR,
message: (
'In answer group ' + (answerIndex + 1) + ', ' +
'rule ' + (ruleIndex + 1) + ', the number of correct ' +
'options in the "Equals" rule should be between ' +
minAllowedCount + ' and ' + maxAllowedCount +
' (the minimum and maximum allowed selection counts).')
});
}
}
});
if (ruleInputs.length === 0) {
if (rule.type === 'ContainsAtLeastOneOf') {
warningsList.push({
type: WARNING_TYPES.ERROR,
message: (
'In answer group ' + (answerIndex + 1) + ', rule ' +
(ruleIndex + 1) + ', the "ContainsAtLeastOneOf" rule ' +
'should have at least one option.')
});
}
}
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe('ItemSelectionInputValidationService', function() {
var goodAnswerGroups, goodDefaultOutcome;
var customizationArguments;
var oof, agof, rof;
var badAnswerGroup;

beforeEach(function() {
module('oppia');
Expand Down Expand Up @@ -68,6 +69,39 @@ describe('ItemSelectionInputValidationService', function() {
false,
null)
];
ThreeInputsAnswerGroups = [agof.createNew(
[rof.createFromBackendDict({
rule_type: 'Equals',
inputs: {
x: ['Selection 1', 'Selection 2', 'Selection 3']
}
})],
goodDefaultOutcome,
false,
null)
];
OneInputAnswerGroups = [agof.createNew(
[rof.createFromBackendDict({
rule_type: 'Equals',
inputs: {
x: ['Selection 1']
}
})],
goodDefaultOutcome,
false,
null)
];
NoInputAnswerGroups = [agof.createNew(
[rof.createFromBackendDict({
rule_type: 'ContainsAtLeastOneOf',
inputs: {
x: []
}
})],
goodDefaultOutcome,
false,
null)
];
IsProperSubsetValidOption = [agof.createNew(
[rof.createFromBackendDict({
rule_type: 'IsProperSubsetOf',
Expand Down Expand Up @@ -102,14 +136,14 @@ describe('ItemSelectionInputValidationService', function() {
customizationArguments.minAllowableSelectionCount.value = 3;

var warnings = validatorService.getAllWarnings(
currentState, customizationArguments, goodAnswerGroups,
currentState, customizationArguments, ThreeInputsAnswerGroups,
goodDefaultOutcome);
expect(warnings).toEqual([{
expect(warnings).toContain({
type: WARNING_TYPES.CRITICAL,
message: (
'Please ensure that the max allowed count is not less than the ' +
'min count.')
}]);
});
});

it(
Expand Down Expand Up @@ -142,7 +176,7 @@ describe('ItemSelectionInputValidationService', function() {
customizationArguments.maxAllowableSelectionCount.value = 3;

var warnings = validatorService.getAllWarnings(
currentState, customizationArguments, goodAnswerGroups,
currentState, customizationArguments, ThreeInputsAnswerGroups,
goodDefaultOutcome);
expect(warnings).toEqual([{
type: WARNING_TYPES.CRITICAL,
Expand Down Expand Up @@ -191,4 +225,38 @@ describe('ItemSelectionInputValidationService', function() {
'rule 1, the "proper subset" rule must include at least 2 options.')
}]);
});

it(
'should expect number of correct options to be in between the maximum ' +
'and minimum allowed selections when the "Equals" rule is used.',
function() {
// Make min allowed selections greater than correct answers.
customizationArguments.minAllowableSelectionCount.value = 2;

var warnings = validatorService.getAllWarnings(
currentState, customizationArguments, OneInputAnswerGroups,
goodDefaultOutcome);
expect(warnings).toEqual([{
type: WARNING_TYPES.ERROR,
message: (
'In answer group 1, rule 1, the number of correct options in ' +
'the "Equals" rule should be between 2 and 2 (the ' +
'minimum and maximum allowed selection counts).')
}]);
});

it(
'should expect at least one option when ' +
'"ContainsAtLeastOneOf" rule is used.',
function() {
var warnings = validatorService.getAllWarnings(
currentState, customizationArguments, NoInputAnswerGroups,
goodDefaultOutcome);
expect(warnings).toEqual([{
type: WARNING_TYPES.ERROR,
message: (
'In answer group 1, rule 1, the "ContainsAtLeastOneOf" rule ' +
'should have at least one option.')
}]);
});
});

0 comments on commit 74aed34

Please sign in to comment.