Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350

amey-kudari · 2019-02-28T05:52:32Z

Explanation

Fixes #6322 by adding an extra option to allow exploration creator provide feedback to user for answers that are a proper superset.

What I did:

1. Added option, is proper subset of, into Item selection, and updated all related files

2. Added tests for same

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.
- If you're a new contributor, please ask on Gitter for someone to assign a reviewer.
- If you're not sure who the appropriate reviewer is, please assign to the issue's "owner" -- see the "talk-to" label on the issue.

… page from showing up before angular gets loaded

This reverts commit 9935d05. got back base.html

undid previous pull request

remove changes

* add issue template for server errors * improve about section * review changes * minor nits

… page from showing up before angular gets loaded

…ate module. (#6305) * Add a script for updating indexes. Move gcloud functions into a separate file. * Further changes needed to use gcloud for deployment. * Remove unused code. * Add ability to specify custom versions. * Do not switch to new version after uploading. * Update fileoverview docstring.

This reverts commit 9935d05. got back base.html

seanlip

Hi @amey-kudari, if you want the PR to be merged, it is really important to ensure that what you are writing is logically correct, otherwise we will keep going back and forth on reviews. PTAL at the inline comments and make sure to carefully read and analyze what your code is doing, otherwise you run the risk of breaking the production code. It's really important to reason through the resulting code and make sure that it's doing what it's supposed to.

seanlip · 2019-03-11T23:43:47Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationService.js

+          rules.forEach(function(rule, ruleIndex) {
+            var ruleInputs = rule.inputs.x;
+            ruleInputs.forEach(function(ruleInput) {
+              var choiceIndex = answerChoiceToIndex[ruleInput];


Isn't answerChoiceToIndex empty? It seems to be declared on line 148 as an empty dict and then never populated. So wouldn't this end up being undefined?

I removed this code here by mistake, I will added it back and checked, (I will probably remove that case now, (#6350 (comment)).

149 seenChoices.forEach(function(seenChoice, choiceIndex) { 150 answerChoiceToIndex[seenChoice] = choiceIndex; 151 });

seanlip · 2019-03-11T23:46:55Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationService.js

+            });
+          });
+        });
+        areAllChoicesCovered = handledAnswers.every(function(handledAnswer) {


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 == 1 case in this PR.

amey-kudari · 2019-03-12T05:50:08Z

@seanlip ,
I was thinking about this, #6350 (comment), but yeah It is getting very confusing and I am not very sure about this, So Ill probably do this,
Add IsProperSubsetOf inside the if condition (we know case ===1 is handled, and make an else condition and verify this isProperSubsetOf without extending that part of code) . This way I am sure I know what is happening.

amey-kudari · 2019-03-14T10:09:21Z

Hi @seanlip ,

And I added another else condition, and made sure I do not modify areAllOptionsCovered, and I rechecked my code. Can you please take a look.

And this was a messy PR, I did not know what I was doing initially as this was my first major PR to any big org. I will plan my code and PR, and make sure I know what every change in any PR is doing before pushing it from now.

Thanks!

seanlip · 2019-03-15T09:32:25Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputRulesServiceSpec.js

+      expect(isirs.IsProperSubsetOf(
+        ['a', 'b', 'c', 'd'], RULE_INPUT)).toBe(false);
+      expect(isirs.IsProperSubsetOf(
+        ['b', 'c', 'd', 'e'], RULE_INPUT)).toBe(false);


@amey-kudari did you miss this comment?

seanlip · 2019-03-15T09:33:34Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationService.js

-          areAllChoicesCovered = handledAnswers.every(function(handledAnswer) {
-            return handledAnswer;
+        } else {
+          answerGroups.forEach(function(answerGroup, answerIndex) {


Why does this check only apply for maxAllowedCount > 1? Does it not apply generally?

Sorry I missed that (I put it inside both, the if and the else condition. I will add it right now.

In general, we should not be duplicating code. Why not run it as a separate check, outside both clauses? This seems like a separate check altogether from areAllChoicesCovered.

In other words, keep the "handledAnswers" stuff inside the existing "maxAllowedCount == 1" clause, but pull out the ">= 2 options" check into its own separate check.

Yeah @seanlip , I agree, I will do it. I should have done this, it will make #6418 easier.
Thanks!

seanlip · 2019-03-15T09:34:21Z

...nsions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationServiceSpec.js

@@ -165,4 +176,22 @@ describe('ItemSelectionInputValidationService', function() {
      message: 'Please ensure the choices are unique.'
    }]);
  });
+
+  it(
+    'should expect more that 1 element to be  in the rule input, if the ' +


Drop extra space between "be" and "in".

done, changed it, will push now

seanlip · 2019-03-15T09:35:17Z

...nsions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationServiceSpec.js

+    '"proper subset" rule is used.',
+    function() {
+      // Modify values of customization arguments to get warning.
+      customizationArguments.minAllowableSelectionCount.value = 0;


Why does the proper subset check depend on minAllowableSelectionCount? I thought the check was for the validity of the rule, not the student's answer.

It is not needed (I kept it before, did not remove it (I thought it will trigger other tests before, removed it now).

…set function, to remove duplicate code

seanlip

Almost there!

seanlip · 2019-03-16T18:04:54Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationService.js

        }
+        answerGroups.forEach(function(answerGroup, answerIndex) {


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).

Yeah, I agree, pushed that part of code down.
Thanks!

seanlip · 2019-03-17T08:25:04Z

extensions/interactions/ItemSelectionInput/directives/ItemSelectionInputValidationService.js

        }
+        areAllChoicesCovered = handledAnswers.every(function(handledAnswer) {


Why has this moved from inside the { ... } to outside? (Look at the diff in the "files changed" tab.)

I think I took it out when I was trying to extend it for isProperSubset, (including all cases of maxAllowedCount). Shall I put it back in?

What is the correct thing to do? Please analyze.

If we are not going to extend it later, (I thought of doing it for all functions in the other issue, but I saw there isnt much use of that, and it would make the code more messy)
I will add it back inside the brackets. I will do it now
Thanks!

seanlip

Looks good, I have no further comments. Thanks @amey-kudari!

seanlip · 2019-03-17T18:17:23Z

@kevinlee12 @bansalnitish This requires your review as code-owners.

kevinlee12

lgtm, thanks @amey-kudari!

amey-kudari · 2019-03-18T04:52:53Z

Thank you so much!
@seanlip @kevinlee12

apb7 · 2019-03-21T17:24:05Z

Hi @amey-kudari, please bring your branch up-to-date with develop. Also, @bansalnitish, can you please take another pass here? Thanks!

update branch with develop

amey-kudari · 2019-03-22T13:38:31Z

Hi @amey-kudari, please bring your branch up-to-date with develop. Also, @bansalnitish, can you please take another pass here? Thanks!

done, @apb7

nithusha21

Standin approval for the changes to the interaction project (Kevin is away for a while).

vibhor98 · 2019-03-26T17:00:36Z

Hi @bansalnitish, can you also take a look at this PR? Thanks!

bansalnitish

LGTM! Thanks @amey-kudari

amey-kudari · 2019-03-27T14:39:25Z

Thank you so much
@bansalnitish for merging it!
And all reviewers for helping me through this PR!

Buahahahah and others added 30 commits February 23, 2019 19:40

added ng-cloak to prevent DEV-MODE from showing up. ng-cloak prevents…

53f0db1

… page from showing up before angular gets loaded

fixed dev mode showing up, by binding it

6f67904

fixed linting issues

3c16759

fixed linting issues(1)

2e930f8

fixed linting issues(1)

2fc32cb

Delete docstrings_checker

53c3dc7

Delete re

9e56386

Delete base.html

9935d05

Revert "Delete base.html"

e2bff3c

This reverts commit 9935d05. got back base.html

deleted extra files

7dc63ef

Merge https://github.com/oppia/oppia into fix6322

9ca34f4

added option - IsProperSubsetOf, to Item-selection

6fbd579

fixed linting errors

202e4b2

fixed linting errors

7eab79a

fixed karma tests

8281ecc

undo previous pull request

a5b9d39

undid previous pull request

remove changes

ebbd172

remove changes

try fix, add feedback 4 option

69d96e5

try fix, wrong addition

242f959

Add issue template for server errors (#6308)

d8b5a2a

* add issue template for server errors * improve about section * review changes * minor nits

added ng-cloak to prevent DEV-MODE from showing up. ng-cloak prevents…

ba9fff5

… page from showing up before angular gets loaded

fixed dev mode showing up, by binding it

2accfe9

fixed linting issues

3889e6b

fixed linting issues(1)

535f968

fixed linting issues(1)

096bda1

Delete docstrings_checker

031f4b7

Delete re

bf2191c

Delete base.html

dd9d991

Revert "Delete base.html"

37e2468

This reverts commit 9935d05. got back base.html

YashJipkate requested a review from seanlip March 11, 2019 17:17

seanlip requested changes Mar 11, 2019

View reviewed changes

Buahahahah added 3 commits March 13, 2019 00:03

added a >1 checking and did not extend areAllOptionsCovered

f5dd3e7

fix lint issues

6cc668a

fixed brace style

4125b98

seanlip requested changes Mar 15, 2019

View reviewed changes

Buahahahah added 2 commits March 15, 2019 19:30

corrected validation file and removed double space

b8c8454

removed the else condition and created a seperate check for ProperSub…

7054f4a

…set function, to remove duplicate code

seanlip reviewed Mar 16, 2019

View reviewed changes

Buahahahah added 2 commits March 17, 2019 07:25

push added code block for 'isProperSubsetOf' down

59e1772

push added code block for 'isProperSubsetOf' down

4e052b2

seanlip reviewed Mar 17, 2019

View reviewed changes

push areAllChoicesCovered back into brackets

0465d4f

seanlip approved these changes Mar 17, 2019

View reviewed changes

kevinlee12 approved these changes Mar 17, 2019

View reviewed changes

vojtechjelinek assigned bansalnitish Mar 17, 2019

Merge branch 'develop' into fix6322

5b6c66c

update branch with develop

amey-kudari requested a review from nithusha21 as a code owner March 22, 2019 13:36

nithusha21 approved these changes Mar 24, 2019

View reviewed changes

bansalnitish approved these changes Mar 27, 2019

View reviewed changes

bansalnitish merged commit 11c73a7 into oppia:develop Mar 27, 2019

amey-kudari deleted the fix6322 branch March 27, 2019 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350

Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350

amey-kudari commented Feb 28, 2019 •

edited

seanlip left a comment •

edited

seanlip Mar 11, 2019

amey-kudari Mar 12, 2019

seanlip Mar 11, 2019

amey-kudari commented Mar 12, 2019

amey-kudari commented Mar 14, 2019

seanlip Mar 15, 2019

seanlip Mar 15, 2019

amey-kudari Mar 15, 2019

seanlip Mar 15, 2019 •

edited

amey-kudari Mar 16, 2019 •

edited

seanlip Mar 15, 2019

amey-kudari Mar 15, 2019

seanlip Mar 15, 2019

amey-kudari Mar 15, 2019

seanlip left a comment

seanlip Mar 16, 2019

amey-kudari Mar 17, 2019

seanlip Mar 17, 2019

amey-kudari Mar 17, 2019

seanlip Mar 17, 2019

amey-kudari Mar 17, 2019

seanlip left a comment

seanlip commented Mar 17, 2019

kevinlee12 left a comment

amey-kudari commented Mar 18, 2019

apb7 commented Mar 21, 2019

amey-kudari commented Mar 22, 2019

nithusha21 left a comment

vibhor98 commented Mar 26, 2019

bansalnitish left a comment

amey-kudari commented Mar 27, 2019

		}
		areAllChoicesCovered = handledAnswers.every(function(handledAnswer) {

Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350

Fix #6322: Add IsProperSubsetOf to item-selection interaction #6350

Conversation

amey-kudari commented Feb 28, 2019 • edited

Explanation

Checklist

seanlip left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amey-kudari commented Mar 12, 2019

amey-kudari commented Mar 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip Mar 15, 2019 • edited

Choose a reason for hiding this comment

amey-kudari Mar 16, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanlip left a comment

Choose a reason for hiding this comment

seanlip commented Mar 17, 2019

kevinlee12 left a comment

Choose a reason for hiding this comment

amey-kudari commented Mar 18, 2019

apb7 commented Mar 21, 2019

amey-kudari commented Mar 22, 2019

nithusha21 left a comment

Choose a reason for hiding this comment

vibhor98 commented Mar 26, 2019

bansalnitish left a comment

Choose a reason for hiding this comment

amey-kudari commented Mar 27, 2019

amey-kudari commented Feb 28, 2019 •

edited

seanlip left a comment •

edited

seanlip Mar 15, 2019 •

edited

amey-kudari Mar 16, 2019 •

edited