-
Notifications
You must be signed in to change notification settings - Fork 527
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 #5225: Remove the EnableLanguageSelectionUi Feature Flag #5239
Fix #5225: Remove the EnableLanguageSelectionUi Feature Flag #5239
Conversation
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.
Hi @kmanikanta335, thanks for your PR.
According to the CI results(see: https://github.com/oppia/oppia-android/wiki/Interpreting-CI-Results), you still have leftover refrences to the removed platform param. Use Ctrl+Shift+F to search for all usages.
Additionally, please deploy the app locally to confirm nothing is broken, before pushing new commits.
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.
@kmanikanta335 per the CI results I can see two failing tests for this platform param being disabled. These tests should be removed because we nolonger have this feature flag. testOptionsFragment_featureDisabled_appLanguageOptionIsNotDisplayed
.
We have two OptionsFragmentTest
files.
…anta335/oppia-android into EnableLanguageSelectionUi
@kmanikanta335, please resolve the merge conflict, and also upload a screenshot confirming that the failing test is passing locally on your machine. Once done, please assign the PR back to me. |
@kmanikanta335, could you please focus on completing this PR before beginning to work on other issues? Please ask questions in case you need some assistance. |
@adhiamboperes |
I left a comment here explaining what needs to be done. Please let me know of any specific questions that I clarify on. |
@kmanikanta335, I reviewed your changes and you still haven't fully addressed my previous comment. You still have a failing test. |
@adhiamboperes I hope now its done |
@adhiamboperes pls review it |
Hi @kmanikanta335, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
Hi @kmanikanta335, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
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.
Thanks @kmanikanta335, this LGTM!
Explanation
Fix #5225
Remove declarations and usages of the EnableLanguageSelectionUi PlatformParameter.
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).