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
Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350
Changes from all commits
53f0db1
6f67904
3c16759
2e930f8
2fc32cb
53c3dc7
9e56386
9935d05
e2bff3c
7dc63ef
9ca34f4
6fbd579
202e4b2
7eab79a
8281ecc
a5b9d39
ebbd172
69d96e5
242f959
d8b5a2a
ba9fff5
2accfe9
3889e6b
535f968
096bda1
1b73f79
031f4b7
bf2191c
dd9d991
37e2468
f81de7d
8fe4738
b71d0c8
3406256
bcb1f35
59fac23
3936da7
30ef871
d6efd32
5210a32
abe9585
524d1ec
c1a2bee
9f65674
e3976ec
32ab85e
dad139d
8a83e28
a6a2448
2603e4f
e005656
5d333a5
fc6c718
9f4b615
fdd76ef
93c62ae
106f366
5e048f5
72c00af
099b1b4
14e4ff7
97e1271
0d73e7a
00c7334
21579af
66086a1
14eb83d
5b0993b
ed45178
8c92410
2d61b9a
efe39c7
524e0ba
1bebe87
af3984c
cf48203
f5dd3e7
6cc668a
4125b98
b8c8454
7054f4a
59e1772
4e052b2
0465d4f
5b6c66c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -131,6 +131,8 @@ oppia.factory('ItemSelectionInputValidationService', [ | |
'please select only one answer choice.') | ||
}); | ||
} | ||
} else if (rule.type === 'IsProperSubsetOf') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only the case if the rule's input contains >= 2 objects, right? ['a'] is not a proper subset of ['a']. Given that this error slipped through, I suggest that you add Karma tests for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @seanlip, I am sorry I did not understand the error here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To modify code, you'll need to start by understanding it. Can you explain, in your own words and in detail, what (a) the getAllWarnings() function is doing, and (b) what this block of code (from line 111 onwards) is doing, conceptually? Once you've done that I can talk you through the error but I need to make sure we have a common baseline first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how I had understood it, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks for explaining. I think you might need to read more of the codebase to understand what customizationArgs and answerGroups are. Your onboarding mentor @ankita240796, or other folks, might be able to point you in the right direction. Also /cc @vibhor98, who is an expert on interactions and should be able to help. Please ask him if you have specific questions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, Thanks @seanlip, I will read through them, and ask @ankita240796 and @vibhor98 in case I get stuck trying to understand it |
||
handledAnswers[choiceIndex] = true; | ||
} else if (rule.type === 'ContainsAtLeastOneOf') { | ||
handledAnswers[choiceIndex] = true; | ||
} else if (rule.type === | ||
|
@@ -160,6 +162,26 @@ oppia.factory('ItemSelectionInputValidationService', [ | |
} | ||
} | ||
|
||
answerGroups.forEach(function(answerGroup, answerIndex) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This for-each loop is redundant. You can add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @vibhor98 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The recent changes are wrong. It looks like you haven't understood the code properly yet. I meant moving this if condition entirely inside the for-each loop of line no: 121 (where all other rules are present) and there is no need of this redundant for-each loop. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi! @vibhor98 , I am not sure, but based on what I understood, ruleInputs.forEach(function(ruleInput) { is inside an if loop, if (maxAllowedCount === 1) { And this warning does not need maxAllowedCount to be 1. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, yes! I think I missed maxAllowedCount customisation arg. In that case, please add the loop that you've removed by mistake and everything else should be fine then. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done, added it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would put this after the "areAllChoicesCovered" bit. That bit relates to the computation that was previously done. So currently it looks like you are doing check A, then switching midway to check B, then going back to check A again (conceptually). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I agree, pushed that part of code down. |
||
var rules = answerGroup.rules; | ||
rules.forEach(function(rule, ruleIndex) { | ||
var ruleInputs = rule.inputs.x; | ||
ruleInputs.forEach(function(ruleInput) { | ||
if (rule.type === 'IsProperSubsetOf') { | ||
if (ruleInputs.length < 2) { | ||
warningsList.push({ | ||
type: WARNING_TYPES.ERROR, | ||
message: ( | ||
'In answer group ' + (answerIndex + 1) + ', ' + | ||
'rule ' + (ruleIndex + 1) + ', the "proper subset" ' + | ||
'rule must include at least 2 options.') | ||
}); | ||
} | ||
} | ||
}); | ||
}); | ||
}); | ||
|
||
return warningsList; | ||
} | ||
}; | ||
|
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.
Add one more test to ensure empty set is also considered as a proper subset.
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.
@bansalnitish , Done, Thanks for reviewing!
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.
and also one more case where the answer is not in the subset at all.
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.
@amey-kudari did you miss this comment?
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.
@seanlip I added this test
in line 84
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.
Yes, but you need to reply to the comment too.
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.
Yeah, sorry I will make sure I reply to all reviews individually (I replied in the end saying l did all of them, #6350 (comment))