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 part of #42: Exploration player base (Part 1) #42 #100

Merged
merged 12 commits into from
Sep 18, 2019

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Sep 13, 2019

Explanation

This PR contains basic activities/fragment structure for exploration player. Currently the controllers are not functional which will be covered in subsequent PRs. Also this PR also contains one class for FakeDataProvider, this class is for couple of PRs only and later on it can be removed once domain / model layers are finished

Project structure:
Screenshot 2019-09-13 at 1 49 59 PM

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.

@rt4914
Copy link
Contributor Author

rt4914 commented Sep 13, 2019

@veena14cs @nikitamarysolomanpvt @BenHenning PTAL.

Also, @veena14cs @nikitamarysolomanpvt use this branch as base branch for your subsequent work on exploration-player.

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 @rt4914! Generally LGTM, but I had one concern around the fake data. Other than that, can you also add the initial tests for the exploration activity so that we have a place to add more as the activity grows? At some point, we need to figure out how to divide activity and fragment specific tests, but for now we can just keep UI tests activity-specific.

import com.squareup.moshi.Moshi
import org.oppia.data.backends.gae.model.GaeExplorationContainer

// Remove this file at later stage once domain and model module are working.
Copy link
Member

Choose a reason for hiding this comment

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

I actually think we should be faking data at a higher level than this (e.g. in domain logic controllers). We should spec out those interfaces and submit them with stubs that return structured fake data, then directly rely on those for the UI.

What fake data do we need for this PR? Can we just check in the structure and hook up to domain controllers in a subsequent PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I created this file but we are not using it anywhere, so we can remove this right now.

But still there is one issue, we will be using models which are present in data module like GaeSubtitledHtml GaeState etc. in subsequent PRs and as far as I understand the Technical Document, we should not be interacting directly with data module from app module. So for that case I think domain module will need something similar to models defined in data module.

Any comments or suggestions on this?

Copy link
Member

Choose a reason for hiding this comment

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

If this is for testing purposes, we can use the mock GAE services directly in tests to influence the data returned by domain controllers. We can extend those services to actually set up state rather than returning the same fixed data.

If this isn't for tests, app code does not need to rely on GaeSubtitledHtml, etc. since it will receive data via the domain layer defined in the model module (see how topic list data is piped in #106).

Does this help clarify?

app/src/main/res/layout/exploration_fragment.xml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@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.

Made changes related to all the files. (Test cases are pending)

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 @rt4914! This LGTM. Once my last few comments are resolved, feel free to submit.

app/src/main/res/layout/exploration_fragment.xml Outdated Show resolved Hide resolved
import com.squareup.moshi.Moshi
import org.oppia.data.backends.gae.model.GaeExplorationContainer

// Remove this file at later stage once domain and model module are working.
Copy link
Member

Choose a reason for hiding this comment

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

If this is for testing purposes, we can use the mock GAE services directly in tests to influence the data returned by domain controllers. We can extend those services to actually set up state rather than returning the same fixed data.

If this isn't for tests, app code does not need to rely on GaeSubtitledHtml, etc. since it will receive data via the domain layer defined in the model module (see how topic list data is piped in #106).

Does this help clarify?

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Sep 17, 2019
@nikitamarysolomanpvt
Copy link
Contributor

LGTM

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Sep 17, 2019
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 and veena14cs Sep 17, 2019
@rt4914
Copy link
Contributor Author

rt4914 commented Sep 17, 2019

@BenHenning Check the error related to test cases.

@BenHenning
Copy link
Member

Ack, will follow up with a fix. Thanks for pointing this out.

@BenHenning
Copy link
Member

#131 is now out for review, so sending this back to you to merge once ready.

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Sep 18, 2019
@BenHenning BenHenning changed the title Fix part of #42: Exploration player base (Part 1) [Blocked: #15, #17, #19, #26, #27, #41] #42 Fix part of #42: Exploration player base (Part 1) #42 Sep 18, 2019
@rt4914
Copy link
Contributor Author

rt4914 commented Sep 18, 2019

This PR can be merged now and everything has been clarified in respective comments.

@rt4914
Copy link
Contributor Author

rt4914 commented Sep 18, 2019

#131 is now out for review, so sending this back to you to merge once ready.

Merging this code now.

@rt4914 rt4914 merged commit d256804 into develop Sep 18, 2019
@BenHenning BenHenning deleted the exploration-player-1-base branch June 10, 2020 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Essential This work item must be completed for its milestone.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants