Migrates text input rules from NormalizedString to SetOfNormalizedString #10882

shavavo · 2020-10-06T03:38:23Z

Overview

This PR does the following:
These changes are required to make rule inputs translatable.

Migrates text input rules from NormalizedString to SetOfNormalizedString
State migration of existing text input rules

oppiabot · 2020-10-06T03:38:25Z

Assigning @DubeySandeep for the first-pass review of this pull request. Thanks!

seanlip · 2020-10-06T03:52:18Z

Hi @shavavo! FYI -- frontend tests are failing? (Not sure how that happened -- aren't they checked pre-push?)

seanlip

Thanks, @shavavo! Took a pass.

seanlip · 2020-10-06T06:28:13Z

extensions/interactions/TextInput/directives/text-input-rules.service.ts

-          editDistance[i][j] = Math.min(
-            editDistance[i - 1][j - 1], editDistance[i][j - 1],
-            editDistance[i - 1][j]) + 1;
+    const levenshtein = (inputString, matchString) => {


Start with a verb, and also be clearer that this is returning a boolean rather than the distance: hasEditDistanceEqualToOne or isDifferentByOneCharacter or hasExactlyOneTypo or similar.

seanlip · 2020-10-06T06:31:34Z

extensions/interactions/TextInput/directives/text-input-validation.service.ts

+          warningsList.push({
+            type: AppConstants.WARNING_TYPES.ERROR,
+            message: (
+              `Answer group ${answerGroupIndex + 1} has multiple rules on ` +


on --> with

seanlip · 2020-10-06T06:32:22Z

extensions/interactions/TextInput/directives/text-input-validation.service.ts

        if (rule.type === 'Contains') {
          // Check if the current string contains any of the previously seen
          // strings as a substring.
          if (seenStringsContains.some(
-            (seenString) => currentString.includes(seenString)) ||
+            (seenString) => currentStrings.includes(seenString)) ||


This seems wrong? Previously, the variable was a string, now it's an array. I think you're checking for something different here?

seanlip · 2020-10-06T06:32:28Z

extensions/interactions/TextInput/directives/text-input-validation.service.ts

        } else if (rule.type === 'StartsWith') {
          // Check if the current string contains any of the previously seen
          // strings as a prefix.
          if (seenStringsStartsWith.concat(seenStringsContains).some(
-            (seenString) => currentString.indexOf(seenString) === 0)) {
+            (seenString) => currentStrings.indexOf(seenString) === 0)) {


seanlip · 2020-10-06T06:33:43Z

extensions/interactions/TextInput/directives/text-input-validation.service.ts

            warningsList.push({
              type: AppConstants.WARNING_TYPES.ERROR,
              message: `Rule ${ruleIndex + 1} from answer group ` +
                `${answerGroupIndex + 1} will never be matched because it ` +
                'is preceded by a \'FuzzyEquals\' rule with a matching input.'
            });
          }
-          seenStringsFuzzyEquals.push(currentString);
+          seenStringsFuzzyEquals.push(...currentStrings);


Don't you need to update the tests...?

seanlip · 2020-10-06T06:34:00Z

extensions/interactions/interaction_specs.json

-      "Contains": "contains {{x|NormalizedString}}",
-      "Equals": "is equal to {{x|NormalizedString}}, without taking case into account",
-      "FuzzyEquals": "is equal to {{x|NormalizedString}}, misspelled by at most one character"
+      "StartsWith": "starts with one of {{x|SetOfNormalizedString}}",


Maybe change to "at least one of", ditto below where appropriate

DubeySandeep

LGTM for the codeowner files: core/domain/draft_upgrade_services*.py

oppiabot · 2020-10-06T23:14:07Z

Unassigning @DubeySandeep since they have already approved the PR.

oppiabot · 2020-10-06T23:14:10Z

Assigning @vojtechjelinek, @nithusha21, @aks681 for code owner reviews. Thanks!

seanlip

Also, confirming this looks good from a test-run perspective! Therefore giving LGTM, but @shavavo please note #10882 (review).

aks681

Lgtm as codeowner

…s-migration

oppiabot · 2020-10-26T01:36:45Z

Assigning @BenHenning for code owner reviews. Thanks!

seanlip · 2020-10-26T02:53:59Z

Hi @shavavo -- could you please try running the coreEditorAndPlayerFeatures and extensions e2e suites locally? Those suites fail for me locally too and I think it's caused by this PR (i.e. it's not a flake).

Thanks!

BenHenning · 2020-10-27T06:25:50Z

Hmm I think oppiabot mistook me for a code owner for some reason. @jameesjohn do you have any idea why that might be? I was a code owner at the beginning of this PR, but have since changed.

I'll cancel my review to avoid confusing it further. @shavavo feel free to re-assign me if you'd like me to do another review pass.

…s-migration

kevintab95 · 2020-10-30T22:29:55Z

Removing "LGTM" label since 1 more approval is required for this PR. Thanks!

kevintab95 · 2020-10-30T22:31:53Z

@srijanreddy98 PTAL!

…s-migration

…ing (oppia#10882) * review comment from PR10246 * migration and frontend changes * linter * backend test fixes * frontend tests * styling * coverage * review & fixed e2e tests * fit * review * tests * todo comment * fix comment * e2e tests * fix e2e tests * fix e2e tests Co-authored-by: shavavo <[email protected]>

shavavo added 13 commits August 14, 2020 11:51

review comment from PR10246

d5a171d

Merge remote-tracking branch 'upstream/develop' into develop

2860751

Merge remote-tracking branch 'upstream/develop' into develop

02fe647

Merge remote-tracking branch 'upstream/develop' into develop

e877af4

Merge remote-tracking branch 'upstream/develop' into develop

7ae6af1

Merge remote-tracking branch 'upstream/develop' into develop

36e0fb3

Merge remote-tracking branch 'upstream/develop' into develop

be3be6d

Merge remote-tracking branch 'upstream/develop' into develop

980b3a6

Merge remote-tracking branch 'upstream/develop' into develop

59bf51c

Merge remote-tracking branch 'upstream/develop' into develop

db78ca8

Merge remote-tracking branch 'upstream/develop' into develop

6d9372a

migration and frontend changes

e94b9b2

linter

4a1d1d8

shavavo added the PR CHANGELOG: Translations -- @DubeySandeep label Oct 6, 2020

shavavo requested review from aks681, DubeySandeep, nithusha21, seanlip and vojtechjelinek as code owners October 6, 2020 03:38

oppiabot bot assigned DubeySandeep Oct 6, 2020

shavavo assigned seanlip Oct 6, 2020

seanlip requested changes Oct 6, 2020

View reviewed changes

seanlip assigned shavavo and unassigned seanlip Oct 6, 2020

DubeySandeep approved these changes Oct 6, 2020

View reviewed changes

oppiabot bot unassigned DubeySandeep Oct 6, 2020

seanlip approved these changes Oct 20, 2020

View reviewed changes

kevintab95 approved these changes Oct 21, 2020

View reviewed changes

kevintab95 removed their assignment Oct 21, 2020

aks681 approved these changes Oct 24, 2020

View reviewed changes

aks681 removed their assignment Oct 24, 2020

oppiabot bot added the PR: LGTM label Oct 24, 2020

shavavo added 3 commits October 24, 2020 21:07

fix comment

0137a92

Merge remote-tracking branch 'upstream/develop' into text-input-equal…

835da1e

…s-migration

e2e tests

70d9f53

shavavo requested a review from BenHenning October 25, 2020 01:51

shavavo removed their assignment Oct 25, 2020

oppiabot bot assigned BenHenning Oct 26, 2020

seanlip assigned shavavo Oct 26, 2020

BenHenning removed their request for review October 27, 2020 06:26

BenHenning removed their assignment Oct 27, 2020

shavavo added 2 commits October 30, 2020 16:22

Merge remote-tracking branch 'upstream/develop' into text-input-equal…

ab87b03

…s-migration

fix e2e tests

bae2f43

shavavo requested a review from srijanreddy98 as a code owner October 30, 2020 21:18

kevintab95 removed the PR: LGTM label Oct 30, 2020

shavavo added 2 commits November 1, 2020 19:55

fix e2e tests

82d2b75

Merge remote-tracking branch 'upstream/develop' into text-input-equal…

f95acc3

…s-migration

srijanreddy98 approved these changes Nov 2, 2020

View reviewed changes

srijanreddy98 added the PR: LGTM label Nov 2, 2020

seanlip merged commit e69e919 into oppia:develop Nov 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrates text input rules from NormalizedString to SetOfNormalizedString #10882

Migrates text input rules from NormalizedString to SetOfNormalizedString #10882

shavavo commented Oct 6, 2020

oppiabot bot commented Oct 6, 2020

seanlip commented Oct 6, 2020

seanlip left a comment

seanlip Oct 6, 2020

shavavo Oct 8, 2020

seanlip Oct 6, 2020

shavavo Oct 8, 2020

seanlip Oct 6, 2020

shavavo Oct 8, 2020

seanlip Oct 6, 2020

shavavo Oct 8, 2020

seanlip Oct 6, 2020

shavavo Oct 8, 2020

seanlip Oct 6, 2020

shavavo Oct 8, 2020

DubeySandeep left a comment

oppiabot bot commented Oct 6, 2020

oppiabot bot commented Oct 6, 2020

seanlip left a comment

aks681 left a comment

oppiabot bot commented Oct 26, 2020

seanlip commented Oct 26, 2020

BenHenning commented Oct 27, 2020

kevintab95 commented Oct 30, 2020

kevintab95 commented Oct 30, 2020

Migrates text input rules from NormalizedString to SetOfNormalizedString #10882

Migrates text input rules from NormalizedString to SetOfNormalizedString #10882

Conversation

shavavo commented Oct 6, 2020

Overview

oppiabot bot commented Oct 6, 2020

seanlip commented Oct 6, 2020

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

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

DubeySandeep left a comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 6, 2020

oppiabot bot commented Oct 6, 2020

seanlip left a comment

Choose a reason for hiding this comment

aks681 left a comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 26, 2020

seanlip commented Oct 26, 2020

BenHenning commented Oct 27, 2020

kevintab95 commented Oct 30, 2020

kevintab95 commented Oct 30, 2020