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

Fixes part of #123, #116, and #130: Cellular Data Controller #194

Merged
merged 11 commits into from
Oct 6, 2019
Merged

Fixes part of #123, #116, and #130: Cellular Data Controller #194

merged 11 commits into from
Oct 6, 2019

Conversation

jamesxu0
Copy link
Contributor

@jamesxu0 jamesxu0 commented Sep 27, 2019 •
edited
Loading

Explanation

Fixes part of #123, #116, and #130. Adds a cellular data controller to store and retrieve a preference on whether to show the CellularDataFragment.

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 assigned to an appropriate reviewer.

@jamesxu0 jamesxu0 requested a review from BenHenning September 27, 2019 22:05
@jamesxu0 jamesxu0 assigned jamesxu0 and BenHenning and unassigned jamesxu0 Sep 27, 2019
@BenHenning BenHenning changed the title Fixes part of #17: Cellular Data Controller Fixes part of #123 & #116: Cellular Data Controller Sep 27, 2019
@BenHenning
Copy link
Member

This is fixing parts of #123, #116, and #130.

@BenHenning BenHenning changed the title Fixes part of #123 & #116: Cellular Data Controller Fixes part of #123, #116, and #130: Cellular Data Controller Sep 27, 2019
BenHenning
BenHenning requested changes Sep 27, 2019
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 @jamesxu0! Structurally this looks great, but I have some concerns about whether we're modeling the right things here. See my comment on the proto change for context.

app/src/main/java/org/oppia/app/player/state/StateFragment.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/domain/audio/CellularDialogController.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/audio/CellularDialogControllerTest.kt Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/audio/CellularDialogControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/audio/CellularDialogControllerTest.kt Outdated Show resolved Hide resolved
domain/src/test/java/org/oppia/domain/audio/CellularDialogControllerTest.kt Show resolved Hide resolved
model/src/main/proto/topic.proto Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned jamesxu0 and unassigned BenHenning Sep 27, 2019
@jamesxu0
Copy link
Contributor Author

@BenHenning PTAL. Added a use_cellular_data preference that is used to determine whether to play audio if you don't want to show the dialog again. Also, updated test cases.

@jamesxu0 jamesxu0 assigned BenHenning and unassigned jamesxu0 Sep 30, 2019
BenHenning
BenHenning requested changes Oct 1, 2019
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 @jamesxu0! I'm hoping there are a few UI tests that can be added and I have one concern with saving preferences, but otherwise the PR looks good to me.

app/src/main/java/org/oppia/app/player/state/StateFragment.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/domain/audio/CellularDialogController.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragment.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned jamesxu0 and unassigned BenHenning Oct 1, 2019
@jamesxu0
Copy link
Contributor Author

jamesxu0 commented Oct 3, 2019

@BenHenning PTAL. Added a suite of test cases for StateFragment.

@jamesxu0 jamesxu0 assigned BenHenning and unassigned jamesxu0 Oct 3, 2019
BenHenning
BenHenning approved these changes Oct 4, 2019
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @jamesxu0 --excellent tests! This generally looks good. Can you also record a few videos in Android Studio with your emulator showing the different flows? That would be nice to help provide visual context for this work.

Feel free to submit once comments are resolved, or re-assign to me if you want me to do one more pass. Generally I think this looks good though.

app/src/main/java/org/oppia/app/player/state/testing/StateFragmentTestActivityPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/state_fragment_test_activity.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentTest.kt Outdated Show resolved Hide resolved
domain/src/main/java/org/oppia/domain/audio/CellularDialogController.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Oct 4, 2019
@jamesxu0 jamesxu0 merged commit cf4b52c into develop Oct 6, 2019
@jamesxu0 jamesxu0 deleted the cellular-data-controller branch October 6, 2019 20:00
@rt4914
Copy link
Contributor

rt4914 commented Oct 7, 2019

@BenHenning @jamesxu0
This PR has one testing folder here: https://github.com/oppia/oppia-android/tree/develop/app/src/main/java/org/oppia/app/player/state

Do we actually need this?

@jamesxu0
Copy link
Contributor Author

jamesxu0 commented Oct 7, 2019

@BenHenning @jamesxu0
This PR has one testing folder here: https://github.com/oppia/oppia-android/tree/develop/app/src/main/java/org/oppia/app/player/state

Do we actually need this?

@rt4914 @BenHenning I made a testing folder to contain the activity and presenter that is used in the espresso StateFragment tests. Ben said he ran into a lot of problems using FragmentScenario, so I created a separate activity to start StateFragment for testing purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

Assignees

@jamesxu0 jamesxu0

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jamesxu0 @BenHenning @rt4914