Skip to content
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 #3857: Add support for HasSpecificTermEqualTo #3859

Merged

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Sep 30, 2021

Fix #3857

Explanation

It was discovered during lesson import that a nerw classifier type was added for ratio input earlier in the year on web: HasSpecificTermEqualTo. The lesson content we want to ship in MR3 was very recently updated to make use of this classifier, so we needed to implement it (or nerf the lesson) in order to unblock MR3. Since introducing new classifiers is fairly streamlined, we decided to implement it as part of MR3.

Note that this work is not technically part of the other MR3-tied localization work, but it's part of its PR chain for easier management & merging.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

Note that this isn't actually a UI-specific PR, but a video has been included using real alpha content to demonstrate it working in a real scenario:

has_specific_term_equal_to_demo.mp4

The above feedback is based on the following state rule:

        outcome {
          dest_state_name: "Research on best recipe"
          feedback {
            html: "<p>No, that\'s not correct. If you look at the ratio of milk to yogurt in your answer, it\'s not equivalent to 1:3.</p>\n\n<p>Take a careful look at Uncle Berry\'s explanation, and see if you can figure out what went wrong. Make sure you\'re working with a pencil and paper -- this can be a bit tricky to do in your head. You can do it!</p>"
            content_id: "feedback_10"
          }
        }
        rule_specs {
          input {
            key: "y"
            value {
              non_negative_int: 5
            }
          }
          input {
            key: "x"
            value {
              non_negative_int: 2
            }
          }
          rule_type: "HasSpecificTermEqualTo"
        }
      }

(i.e. it matches if the second component of the ratio contains '5' regardless of any other component).

Because this isn't actually a UI change, none of the other UI artifacts are included which are normally required for such changes per the PR template.

BenHenning and others added 30 commits August 31, 2021 23:33
There are a lot of details to cover here--see the PR for the complete
context.
- Add missing codeowner
- Add support for configuring base branch reference
- Update CI for dev/alpha AAB builds to use 'develop' since there's no
  origin configured by default in the workflows
This is needed to open a PR on GitHub. This commit is being made so that
the PR can start off in a broken Actions state.

This also initially disables most non-Bazel workflows to make workflow
iteration faster and less impacting on other team members.
This introduces a new mechanism for passing lists of tests to sharded
test targets in CI, and hooks it up. No actual sharding is occurring
yet. This led to some simplifications in the CI workflow since the
script can be more dynamic in computing the full list of targets (which
also works around a previous bug with instrumentation tests being run).
Java proto lite also needed to be upgraded for the scripts to be able to
use it.

More testing/documentation needed as this functionality continues to
expand.
This simply partitions bucketed groups of targets into chunks of 10 for
each run. Only 3 buckets are currently retained to test sharding in CI
before introducing full support.
Fixes some caching bucket and output bugs. Also, introduces while loop &
keep_going to introduce resilience against app test build failures (or
just test failures in general).
Also, enable other workflows.

Note that CI shouldn't fully pass yet since some documentation and
testing needs to be added yet, but this is meant to be a more realistic
test of the CI environment before the PR is finished.
Adds a human-readable prefix to make the shards look a bit nicer.

Also, adds more fine-tuned partitioning to bucket & reduce shard counts
to improve overall timing. Will need to be tested in CI.
A newly computed variable wasn't updated to be used in an earlier
change.
Add docstrings for proto.
…/oppia-android into add-bundles-proguard-build-flavors
Neither 'mv -t' nor piping to mv work on OSX so we needed to find an
alternative (in this case just trying to move everything). This actually
works a bit better since it's doing a per-file move rather than
accommodating for files that shouldn't be moved (which isn't an issue
since the destination directory is different than the one containing the
AAB file).
Documentation, thorough tests, and detailed description of these changes
are still needed.
This demonstrates working string selection for system-based and
overwritten app languages, including necessary activity recreation &
layout direction overwriting.

This also includes a bunch of Dagger infra refactoring so that some app
layer packages can now be modularized (including the new packages).
…o localization-part5-introduce-app-string-translations-support-and-refactor
This involves MANY broad changes to ensure consistent string retrieval
(for arrays and plurals), formatting, and string transformations
throughout the codebase. Some extra patterns to added to fix things that
were needed, and a few issues were fixed along the way.
@BenHenning
Copy link
Sponsor Member Author

Reopening to force CI to fully restart (to avoid false passing CI check for base branch since it wasn't set when CI was kicked off).

@BenHenning BenHenning modified the milestones: Alpha MR4, Alpha MR3 Sep 30, 2021
@BenHenning BenHenning marked this pull request as ready for review September 30, 2021 11:15
@BenHenning
Copy link
Sponsor Member Author

@anandwana001 PTAL for review.

@BenHenning BenHenning linked an issue Oct 4, 2021 that may be closed by this pull request
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oppiabot oppiabot bot added the PR: LGTM label Oct 5, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 5, 2021

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

…answer-translations-support

Conflicts:
	domain/BUILD.bazel
…tions-support' into localization-part7-add-gate-for-selecting-written-language-and-add-todos
Base automatically changed from localization-part7-add-gate-for-selecting-written-language-and-add-todos to develop October 6, 2021 22:15
…age-and-add-todos' into localization-part8-add-support-for-new-ratio-classifier
@BenHenning BenHenning changed the title Fix #3857: Add support for HasSpecificTermEqualTo [Blocked: #3797] Fix #3857: Add support for HasSpecificTermEqualTo Oct 6, 2021
@BenHenning
Copy link
Sponsor Member Author

Thanks @anandwana001!

@BenHenning BenHenning merged commit 5daaa9e into develop Oct 6, 2021
@BenHenning BenHenning deleted the localization-part8-add-support-for-new-ratio-classifier branch October 6, 2021 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for HasSpecificTermEqualTo
3 participants