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 #134: HomeFragment- Continue playing Low-fi #304

Merged
merged 19 commits into from
Nov 6, 2019
Merged

Fix #134: HomeFragment- Continue playing Low-fi #304

merged 19 commits into from
Nov 6, 2019

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Nov 5, 2019

Explanation

This PR is a part of HomeFragment and finished the "ContinuePlayingActivity"

Corresponding Mock: https://xd.adobe.com/spec/e2239cf4-9cde-4c08-5296-25316c1f0a14-9412/screen/2b6c8e95-5ac7-4472-bc2b-1c318a3f7e5c/HP-Continue-Playing-

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 rt4914 changed the title Fix #134: HomeFragment- Continue playing Fix #134: HomeFragment- Continue playing Low-fi Nov 5, 2019
@rt4914 rt4914 requested review from BenHenning, nikitamarysolomanpvt and veena14cs and removed request for BenHenning November 5, 2019 13:01
@rt4914
Copy link
Contributor Author

rt4914 commented Nov 5, 2019

@veena14cs @nikitamarysolomanpvt @BenHenning @jamesxu0 @vinitamurthi

While writing test-cases I was actually not able to run all test-cases at once but I was able to run each and every individual test case. (Maybe some android-studio related issue.)
So, I suggest please pay extra attention to test cases in this PR.

veena14cs
veena14cs approved these changes Nov 5, 2019
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.

Looks good. Please address nit change.

@veena14cs
Copy link
Contributor

@veena14cs @nikitamarysolomanpvt @BenHenning @jamesxu0 @vinitamurthi

While writing test-cases I was actually not able to run all test-cases at once but I was able to run each and every individual test case. (Maybe some android-studio related issue.)
So, I suggest please pay extra attention to test cases in this PR.

I was able to run all the testcases at once in my system.

@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Nov 5, 2019
@rt4914
Copy link
Contributor Author

rt4914 commented Nov 5, 2019

@veena14cs @nikitamarysolomanpvt @BenHenning @jamesxu0 @vinitamurthi
While writing test-cases I was actually not able to run all test-cases at once but I was able to run each and every individual test case. (Maybe some android-studio related issue.)
So, I suggest please pay extra attention to test cases in this PR.

I was able to run all the testcases at once in my system.

Okay. Thanks.

@rt4914
Copy link
Contributor Author

rt4914 commented Nov 5, 2019

I have covered three cases:

  1. Recent stories as well as Old Stories
    Screenshot_1572966595

  2. Only Recent stories
    Screenshot_1572966640

  3. Only Old stories
    Screenshot_1572966661

@rt4914 rt4914 removed their assignment Nov 5, 2019
nikitamarysolomanpvt
nikitamarysolomanpvt suggested changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

Tests failing:PTAL
small

on land scape mode.PTAL
small

app/src/main/java/org/oppia/app/home/UserAppHistoryViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/home/continueplaying/ContinuePlayingFragmentPresenter.kt Show resolved Hide resolved
BenHenning
BenHenning approved these changes Nov 5, 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 @rt4914! Overall, the PR looks really good to me. Please re-assign if you want me to take another look, but once these comments are addressed feel free to submit.

app/src/main/java/org/oppia/app/home/continueplaying/ContinuePlayingViewModel.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/home/continueplaying/ContinuePlayingFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/home/continueplaying/OngoingListAdapter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/home/continueplaying/OngoingListAdapter.kt Show resolved Hide resolved
app/src/main/java/org/oppia/app/home/continueplaying/OngoingStoryViewModel.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/continue_playing_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/continue_playing_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/ongoing_story_card.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/home/ContinuePlayingActivityTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/home/ContinuePlayingActivityTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Nov 5, 2019
@rt4914
Copy link
Contributor Author

rt4914 commented Nov 5, 2019 •
edited
Loading

Tests failing:PTAL
small

on land scape mode.PTAL
small

@nikitamarysolomanpvt All these test-cases are actually passing in portrait as well as landscape mode. Can you please mention error from one of the failing test-case?

Also, you are testing HomeActivityTest but this PR has test cases in ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest.

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Nov 5, 2019
@nikitamarysolomanpvt
Copy link
Contributor

Tests failing:PTAL
small
on land scape mode.PTAL
small

@nikitamarysolomanpvt All these test-cases are actually passing in portrait as well as landscape mode. Can you please mention error from one of the failing test-case?

Also, you are testing HomeActivityTest but this PR has test cases in ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest.

I checked all test classes which are modified...
ContinuePlayingActivityTest seems modified after i tested. (ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest. classes(files with these name) where not there when i was doing test)

@rt4914
Copy link
Contributor Author

rt4914 commented Nov 6, 2019 •
edited
Loading

Tests failing:PTAL
small
on land scape mode.PTAL
small

@nikitamarysolomanpvt All these test-cases are actually passing in portrait as well as landscape mode. Can you please mention error from one of the failing test-case?
Also, you are testing HomeActivityTest but this PR has test cases in ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest.

I checked all test classes which are modified...
ContinuePlayingActivityTest seems modified after i tested. (ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest. classes(files with these name) where not there when i was doing test)

Actually these all are same test-cases which were introduced earlier but now I have just divided them in two separate files.

Also, your tests were failing in HomeActivityTest and not in ContinuePlayingActivityTest, so we can say that this PR is correct.

The HomeActivityTest might have been broken (not-sure yet) in #302 because last checked it was correctly working in #289 .

We will solve this in next PR.

nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM

@rt4914
Copy link
Contributor Author

rt4914 commented Nov 6, 2019

Merging this PR.

@rt4914 rt4914 merged commit 4613ff8 into develop Nov 6, 2019
@rt4914 rt4914 deleted the continue-playing-part-1 branch November 6, 2019 05:14
@rt4914
Copy link
Contributor Author

rt4914 commented Nov 6, 2019

Tests failing:PTAL
small
on land scape mode.PTAL
small

@nikitamarysolomanpvt All these test-cases are actually passing in portrait as well as landscape mode. Can you please mention error from one of the failing test-case?
Also, you are testing HomeActivityTest but this PR has test cases in ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest.

I checked all test classes which are modified...
ContinuePlayingActivityTest seems modified after i tested. (ContinuePlayingFragmentTest and ContinuePlayingFragmentTestActivityTest. classes(files with these name) where not there when i was doing test)

Actually these all are same test-cases which were introduced earlier but now I have just divided them in two separate files.

Also, your tests were failing in HomeActivityTest and not in ContinuePlayingActivityTest, so we can say that this PR is correct.

The HomeActivityTest might have been broken (not-sure yet) in #302 because last checked it was correctly working in #289 .

We will solve this in next PR.

@nikitamarysolomanpvt I did check all the HomeActivityTest test-cases in landscape and orientation mode on develop and they are passing for me.

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

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

@veena14cs veena14cs veena14cs approved these changes

@BenHenning BenHenning BenHenning approved these changes

@jamesxu0 jamesxu0 Awaiting requested review from jamesxu0

@vinitamurthi vinitamurthi Awaiting requested review from vinitamurthi

Assignees

@nikitamarysolomanpvt nikitamarysolomanpvt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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