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.

Sign up for GitHub

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

Jump to bottom

Fix Part #565: Domain on-boarding flag #618

Merged
merged 13 commits into from
Feb 17, 2020
Merged

Fix Part #565: Domain on-boarding flag #618

merged 13 commits into from
Feb 17, 2020

Conversation

nikitamarysolomanpvt
Copy link
Contributor

Explanation

Duplicate of #574
Onboarding flow should be visible only once irrespective of learner profile. Introduced domain layer code to control this feature.
Oppia app now showing On-boarding on initial app open, and directly to choose profile page upon returning to the app. Both of these states are also properly retained across configuration changes due to Dagger not leading to activities trying to recreate their dependent controllers.

From a testing side, testcase for OnboardingFlowControllerTest is introduced to test OnboardingFlowController.

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 PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@nikitamarysolomanpvt
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
50eb1f4 
… material-cardview-issue-fix-approach-2

# Conflicts:
#	app/build.gradle
@nikitamarysolomanpvt nikitamarysolomanpvt changed the title Fix #565: Domain on-boarding flag Fix #565: Domain on-boarding flag-part 1 Jan 21, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt changed the title Fix #565: Domain on-boarding flag-part 1 Fix #565: Domain on-boarding flag Jan 21, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt mentioned this pull request Jan 21, 2020
Closed
8 tasks
@nikitamarysolomanpvt nikitamarysolomanpvt changed the title Fix #565: Domain on-boarding flag Fix Part #565: Domain on-boarding flag Jan 21, 2020
veena14cs
veena14cs suggested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

Test cases are passing but it gives some errors.

Screenshot 2020-01-22 at 11 22 54 AM

@veena14cs veena14cs removed their assignment Jan 22, 2020
rt4914
rt4914 approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks

@nikitamarysolomanpvt
Copy link
Contributor Author

Test cases are passing but it gives some errors.
Screenshot 2020-01-22 at 11 22 54 AM

+1

PTAL on #599. This was discussed in meeting and was told to raise as an issue.

veena14cs
veena14cs approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

LGTM.

@veena14cs veena14cs removed their assignment Jan 22, 2020
BenHenning
BenHenning requested changes Feb 11, 2020
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @nikitamarysolomanpvt! This generally looks canonical to me. I have some questions about the API & proto that I'd like addressed before I approve. Also, please put the controller/test/proto in final locations.

domain/src/main/java/org/oppia/domain/OnBoardingFlowController.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/domain/OnBoardingFlowController.kt Outdated Show resolved Hide resolved
model/src/main/proto/example.proto Outdated Show resolved Hide resolved
@oppia oppia deleted a comment from rt4914 Feb 11, 2020
rt4914
rt4914 approved these changes Feb 11, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

rt4914
rt4914 reviewed Feb 11, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Nit changes

model/src/main/proto/onboarding.proto Outdated Show resolved Hide resolved
model/src/main/proto/onboarding.proto Show resolved Hide resolved
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Feb 12, 2020
BenHenning
BenHenning approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @nikitamarysolomanpvt! Thanks for cleaning up the location & proto. Please remove the test-only clear function, and then this LGTM for submission. Feel free to follow up with me via chat if you have any questions about removing that function.

domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
@nikitamarysolomanpvt
Copy link
Contributor Author

Thanks @nikitamarysolomanpvt! Thanks for cleaning up the location & proto. Please remove the test-only clear function, and then this LGTM for submission. Feel free to follow up with me via chat if you have any questions about removing that function.

PTAL i removed it from controller and added in test class

rt4914
rt4914 approved these changes Feb 14, 2020
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

Nit changes suggested.

domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/OnboardingFlowControllerTest.kt Outdated Show resolved Hide resolved
@nikitamarysolomanpvt nikitamarysolomanpvt merged commit 1efb989 into develop Feb 17, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt deleted the domain-onboarding-flag-part-1 branch February 17, 2020 06:10
@rt4914 rt4914 mentioned this pull request Apr 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these changes

@veena14cs veena14cs veena14cs approved these changes

Assignees

@nikitamarysolomanpvt nikitamarysolomanpvt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nikitamarysolomanpvt @rt4914 @BenHenning @veena14cs