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 81 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 === | ||
|
@@ -144,10 +146,30 @@ oppia.factory('ItemSelectionInputValidationService', [ | |
}); | ||
}); | ||
}); | ||
areAllChoicesCovered = handledAnswers.every(function(handledAnswer) { | ||
return handledAnswer; | ||
}); | ||
} | ||
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.') | ||
}); | ||
} | ||
} | ||
}); | ||
}); | ||
}); | ||
|
||
areAllChoicesCovered = handledAnswers.every(function(handledAnswer) { | ||
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. Are you sure this is correct? You've only considered one type of rule (for the != 1 case). I am not sure you need this actually. 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! @seanlip, 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. Also sorry for late replies again, This will not happen after 16th (sem officially gets over, (ill have end sem exams but most of those are cramming night before)), I will be much more active from 16th. 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. What I'm trying to say is, I'm not sure this check works correctly for the != 1 case. (That's why it was originally only implemented for the == 1 case.) That's because you're populating handledAnswers only using IsProperSubsetOf and nothing else. 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 this was another problem, I am not sure how is handled ( I tried checking what is happening in OutcomeObjectFactory,) and how we are checking if all options are handled. The handled choices is used to see if some choice is not handled in the ==1 case, what I am doing is adding all the other choices, but verifying that all are checked (if they have proper subset rule). I think we can solve this in #6418 , by adding other options and validating them. and that way we very each choice is addressed. 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. Also, I think I got the question wrong, Yeah the check works for !=1 case, as areAllChoicesCovered will only be true if All choices have been checked by some rule. 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 don't understand the logic here. In the maxAllowedCount > 1 case, handledAnswers is initialized here, the entire block from lines 111-147 is skipped (since maxAllowedCount > 1), and then it seems to me like all rule types apart from IsProperSubsetOf are being ignored. So, why do you say that "areAllChoicesCovered will only be true if all choices have been checked by some rule"? There are other rules besides isProperSubsetOf. I recommend that you do not try to extend that check beyond the 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. Well Yeah I agree, I am confused now, I will remove that for !=1 case, if (!areAllChoicesCovered) { // Now, this will happen if all choices are not covered by = == 1 case or properSubset rule, I am checking this by not including "selection3"
179 // if (!defaultOutcome || defaultOutcome.isConfusing(stateName)) { //this I am guessing che cks if user has given default feedback (if the user has given oppia something to say, I tried backtracking this, so now Im assuming user has not given a def feedback, So I shuld get this error
180 warningsList.push({
181 type: WARNING_TYPES.ERROR,
182 message: (
183 'Please add something for Oppia to say in the ' +
184 '\"All other answers\" response.')
185 });
186 // }
187 } and added this check in the Spec file, 92 it('If everything goes how I except, this case should be handled.', function() {
93 customizationArguments.maxAllowableSelectionCount.value=1; //this is the case handled by ===1 ca se,
94 var warnings = validatorService.getAllWarnings(currentState, customizationArguments, IsProperSubs etValidOption,goodDefaultOutcome);
95 console.log(warnings);
96 });
97 And checked when options cover everything and when they do not (adding and removing option 3 and checking for warnings) . But yeah this was getting confusing, So I think It is better I drop that, and check for in the ===1 for ProperSubsetRule also case, and make another !=1 case for the proper subset rule. |
||
return handledAnswer; | ||
}); | ||
|
||
if (!areAllChoicesCovered) { | ||
if (!defaultOutcome || defaultOutcome.isConfusing(stateName)) { | ||
|
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))