Fix part of #2132: Chapter List Page Issues #2232

viktoriias · 2020-12-07T02:43:11Z

@anandwana001 PTAL

Explanation

Fixes part of #2132

Elevate the inactive cards
Active cards show full description.
Inactive cards should show Chapter name and description show prerequisites to unlock it.
Blur thumbnails of inactive cards

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.

anandwana001

Thanks, @viktoriias for your first Pull Request.

Added a few changes, PTAL

app/src/main/res/layout/story_chapter_view.xml

app/src/main/res/values/strings.xml

prayutsu · 2020-12-07T09:08:49Z

@viktoriias Can you please update the PR title to follow our conventional format?
e.g. - Fix part of #2132: Chapter List Page Issues

anandwana001 · 2020-12-08T07:25:53Z

@viktoriias no changes required in .idea/codeStyles/Project.xml file.

viktoriias · 2020-12-08T07:39:44Z

@viktoriias no changes required in .idea/codeStyles/Project.xml file.
How do I remove this file from here?

anandwana001 · 2020-12-08T08:44:22Z

@viktoriias no changes required in .idea/codeStyles/Project.xml file.
How do I remove this file from here?

https://stackoverflow.com/questions/36763274/how-to-remove-a-file-from-git-pull-request/42405903
This might help, removing an unwanted file from a pull request.

viktoriias · 2020-12-08T17:10:41Z

@viktoriias no changes required in .idea/codeStyles/Project.xml file.
How do I remove this file from here?

https://stackoverflow.com/questions/36763274/how-to-remove-a-file-from-git-pull-request/42405903
This might help, removing an unwanted file from a pull request.

Ok thanks, I think I removed it.

BenHenning

Thanks @viktoriias! I think this is a great start. I have a few comments, one of which will required changes to part of the overall approach.

Beyond that, can you also add a test to verify that the previous chapter text is correctly computed & used in the expected cases?

...src/main/java/org/oppia/android/app/story/storyitemviewmodel/StoryChapterSummaryViewModel.kt

app/src/main/res/layout/story_chapter_view.xml

app/src/main/res/values/strings.xml

Remove blurring thumbnails of inactive cards - this change will be done separately.

BenHenning

Thanks @viktoriias! As a reminder, please also add a test to verify these changes. And, can you please take a look at @anandwana001's reply to my comment thread above about the data potentially being empty in some cases? (I think this might be intentional & correct, but I haven't looked the case closely).

model/src/main/proto/topic.proto

domain/src/main/java/org/oppia/android/domain/topic/TopicController.kt

app/src/main/res/layout-land/story_chapter_view.xml

viktoriias · 2020-12-15T03:19:04Z

Thanks @viktoriias. Overall, this looks great! I have one nit, and one main comment: you added a test for TopicControllerTest, but can you also add a UI-facing test to make sure the binding logic in story_chapter_view is tested? I think that would require a new test in StoryFragmentTest. Please add a test for both portrait & landscape (the latter can be done by changing orientation--see testStoryFragment_changeConfiguration_chapterSummaryIsShownCorrectly).

I looked into this test file initially, per @anandwana001's advice. It marks first story as completed, so prerequisite didn't show up. I'll need to remove markPartialStoryProgressForFractions to be able to test summary for the second story.

anandwana001 · 2020-12-15T03:27:05Z

Thanks @viktoriias. Overall, this looks great! I have one nit, and one main comment: you added a test for TopicControllerTest, but can you also add a UI-facing test to make sure the binding logic in story_chapter_view is tested? I think that would require a new test in StoryFragmentTest. Please add a test for both portrait & landscape (the latter can be done by changing orientation--see testStoryFragment_changeConfiguration_chapterSummaryIsShownCorrectly).

I looked into this test file initially, per @anandwana001's advice. It marks first story as completed, so prerequisite didn't show up. I'll need to remove markPartialStoryProgressForFractions to be able to test summary for the second story.

Removing markPartialStoryProgressForFractions might cause failure for other test. I think what we can do here is to shift it from the @Before to all the required test before launching the test. This way we can have test specific weather or not we require partial progress.

viktoriias · 2020-12-15T03:40:09Z

Thanks @viktoriias. Overall, this looks great! I have one nit, and one main comment: you added a test for TopicControllerTest, but can you also add a UI-facing test to make sure the binding logic in story_chapter_view is tested? I think that would require a new test in StoryFragmentTest. Please add a test for both portrait & landscape (the latter can be done by changing orientation--see testStoryFragment_changeConfiguration_chapterSummaryIsShownCorrectly).

I looked into this test file initially, per @anandwana001's advice. It marks first story as completed, so prerequisite didn't show up. I'll need to remove markPartialStoryProgressForFractions to be able to test summary for the second story.

Removing markPartialStoryProgressForFractions might cause failure for other test. I think what we can do here is to shift it from the @Before to all the required test before launching the test. This way we can have test specific weather or not we require partial progress.

It only affects testStoryFragment_correctStoryCountLoadedInHeader test: 1 of 2 Chapters Completed -> 0 of 2 Chapters Completed. I think it's fine to remove it, let me know what you think.

anandwana001 · 2020-12-15T03:51:35Z

Thanks @viktoriias. Overall, this looks great! I have one nit, and one main comment: you added a test for TopicControllerTest, but can you also add a UI-facing test to make sure the binding logic in story_chapter_view is tested? I think that would require a new test in StoryFragmentTest. Please add a test for both portrait & landscape (the latter can be done by changing orientation--see testStoryFragment_changeConfiguration_chapterSummaryIsShownCorrectly).

I looked into this test file initially, per @anandwana001's advice. It marks first story as completed, so prerequisite didn't show up. I'll need to remove markPartialStoryProgressForFractions to be able to test summary for the second story.

Removing markPartialStoryProgressForFractions might cause failure for other test. I think what we can do here is to shift it from the @Before to all the required test before launching the test. This way we can have test specific weather or not we require partial progress.

It only affects testStoryFragment_correctStoryCountLoadedInHeader test: 1 of 2 Chapters Completed -> 0 of 2 Chapters Completed. I think it's fine to remove it, let me know what you think.

Sounds good, we can shift it to the test before the launch and check if all the tests are passing on espresso and robolectric.

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

BenHenning

Thanks @viktoriias! Overall, the PR LGTM. Just have one more comment from me to address.

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

...src/main/java/org/oppia/android/app/story/storyitemviewmodel/StoryChapterSummaryViewModel.kt

viktoriias · 2020-12-16T02:54:28Z

@anandwana001 please review

anandwana001

Reviewed the UI test cases, Thanks @viktoriias

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

anandwana001

last nit change. Fix ktlint issues.
LGTM, Thanks @viktoriias

Also, just for the future reference, please add 2 screenshots showing that all the tests are passing on Espresso and Robolectric.

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

viktoriias · 2020-12-16T22:26:11Z

Passing tests:

anandwana001 · 2020-12-17T01:14:05Z

Adding @BenHenning for final merge.

viktoriias · 2020-12-17T01:55:59Z

StoryFragmentTest Robolectric:

BenHenning

Thanks @viktoriias! This looks great! Just one small comment, but otherwise LGTM.

@rt4914 or @anandwana001 feel free to merge this after the last comment is resolved if I miss it.

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt

…s of inactive cards. (#2422) * Lock and blur * Replace color/white with #FFFFFF * Add \n * Move isBlurred to LessonThumbnailImageView. * Remove unused import. * Move dependency on Glide away from from ImageLoader. * Remove unneeded line. * Refactor Transformation-related code, add KDocs. * Renaming. * Remove unused import. * Reformat. * Add Mockk test * Add Mockk test - lint * Add blurring tests. * Mock ImageLoader in StoryActivityTest. * Add delay before image loading. * Nits and improvements. * Remove unused import. * Remove unused import. * Set image resources in LessonThumbnailImageView in addition to calling GlideImageLoader. * BlurTransformation re-write. * BlurTransformation re-write. * Add TestImageLoaderModule to some tests. * Lint * Add TestImageLoaderModule to more tests. * Add TestImageLoaderModule to more tests. * Update utility/src/main/java/org/oppia/android/util/parser/BlurTransformation.kt Co-authored-by: Ben Henning <[email protected]> * Update utility/src/main/java/org/oppia/android/util/parser/BlurTransformation.kt Co-authored-by: Ben Henning <[email protected]> * TestGlideImageLoader Refactor. * Reformat. * Reformat. * Reformat. * Nits and comments. * Revert changes in landscape design. * Change BlurTransformation to return a copy instead of 'toTransform'. * Update app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt Co-authored-by: Ben Henning <[email protected]> * Refactor TestGlideImageLoader. * Nits Co-authored-by: Ben Henning <[email protected]>

Fix part of #2132

a940f35

anandwana001 self-requested a review December 7, 2020 03:11

anandwana001 assigned viktoriias and anandwana001 Dec 7, 2020

anandwana001 suggested changes Dec 7, 2020

View reviewed changes

anandwana001 removed their assignment Dec 7, 2020

viktoriias changed the title ~~Fix part of #2132~~ Fix part of #2132: Chapter List Page Issues Dec 7, 2020

viktoriias added 2 commits December 7, 2020 23:18

Fix part of #2132

ba223f0

Fix part of #2132

e564d55

viktoriias requested a review from BenHenning as a code owner December 8, 2020 07:22

viktoriias added 2 commits December 7, 2020 23:29

Fix part of #2132

332f42d

Fix part of #2132

7c381a7

viktoriias added 2 commits December 8, 2020 09:05

Fix part of #2132

f5a6630

Fix part of #2132

e5ce031

BenHenning self-assigned this Dec 10, 2020

BenHenning requested changes Dec 10, 2020

View reviewed changes

...src/main/java/org/oppia/android/app/story/storyitemviewmodel/StoryChapterSummaryViewModel.kt Outdated Show resolved Hide resolved

app/src/main/res/layout/story_chapter_view.xml Outdated Show resolved Hide resolved

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved

BenHenning removed their assignment Dec 10, 2020

Pass a ChapterSummary object as a prerequisite to unlock this chapter.

981cf40

Remove blurring thumbnails of inactive cards - this change will be done separately.

viktoriias requested review from BenHenning and anandwana001 December 11, 2020 02:00

BenHenning assigned BenHenning and anandwana001 and unassigned viktoriias Dec 12, 2020

BenHenning requested changes Dec 12, 2020

View reviewed changes

model/src/main/proto/topic.proto Outdated Show resolved Hide resolved

domain/src/main/java/org/oppia/android/domain/topic/TopicController.kt Outdated Show resolved Hide resolved

app/src/main/res/layout-land/story_chapter_view.xml Outdated Show resolved Hide resolved

BenHenning removed their assignment Dec 12, 2020

Add a UI-facing test.

5fbfd27

viktoriias requested a review from BenHenning December 15, 2020 05:14

anandwana001 reviewed Dec 15, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt Outdated Show resolved Hide resolved

BenHenning self-assigned this Dec 15, 2020

Add more test coverage for a completed chapter.

8ebc0b9

viktoriias requested a review from anandwana001 December 16, 2020 00:59

BenHenning approved these changes Dec 16, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt Outdated Show resolved Hide resolved

...src/main/java/org/oppia/android/app/story/storyitemviewmodel/StoryChapterSummaryViewModel.kt Outdated Show resolved Hide resolved

BenHenning removed their assignment Dec 16, 2020

Move landscape test below the portrait one.

51e84b0

anandwana001 suggested changes Dec 16, 2020

View reviewed changes

Added long summary test + reformatting.

87ad1de

viktoriias requested a review from anandwana001 December 16, 2020 07:53

anandwana001 approved these changes Dec 16, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt Outdated Show resolved Hide resolved

viktoriias added 2 commits December 16, 2020 08:25

Formatting.

91f3c26

Lint

fc4f9c2

anandwana001 removed their assignment Dec 16, 2020

anandwana001 assigned BenHenning Dec 17, 2020

BenHenning approved these changes Dec 17, 2020

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/story/StoryFragmentTest.kt Outdated Show resolved Hide resolved

Renaming

9864716

anandwana001 merged commit f80a8c2 into oppia:develop Dec 17, 2020

viktoriias deleted the branch2 branch December 17, 2020 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix part of #2132: Chapter List Page Issues #2232

Fix part of #2132: Chapter List Page Issues #2232

viktoriias commented Dec 7, 2020 •

edited by anandwana001

Loading

anandwana001 left a comment

prayutsu commented Dec 7, 2020

anandwana001 commented Dec 8, 2020 •

edited

Loading

viktoriias commented Dec 8, 2020

anandwana001 commented Dec 8, 2020

viktoriias commented Dec 8, 2020 •

edited

Loading

BenHenning left a comment •

edited

Loading

BenHenning left a comment

viktoriias commented Dec 15, 2020

anandwana001 commented Dec 15, 2020

viktoriias commented Dec 15, 2020 •

edited

Loading

anandwana001 commented Dec 15, 2020 •

edited

Loading

BenHenning left a comment

viktoriias commented Dec 16, 2020

anandwana001 left a comment

anandwana001 left a comment •

edited

Loading

viktoriias commented Dec 16, 2020

anandwana001 commented Dec 17, 2020

viktoriias commented Dec 17, 2020

BenHenning left a comment •

edited

Loading

Fix part of #2132: Chapter List Page Issues #2232

Fix part of #2132: Chapter List Page Issues #2232

Conversation

viktoriias commented Dec 7, 2020 • edited by anandwana001 Loading

Explanation

Checklist

anandwana001 left a comment

Choose a reason for hiding this comment

prayutsu commented Dec 7, 2020

anandwana001 commented Dec 8, 2020 • edited Loading

viktoriias commented Dec 8, 2020

anandwana001 commented Dec 8, 2020

viktoriias commented Dec 8, 2020 • edited Loading

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

viktoriias commented Dec 15, 2020

anandwana001 commented Dec 15, 2020

viktoriias commented Dec 15, 2020 • edited Loading

anandwana001 commented Dec 15, 2020 • edited Loading

BenHenning left a comment

Choose a reason for hiding this comment

viktoriias commented Dec 16, 2020

anandwana001 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

viktoriias commented Dec 16, 2020

anandwana001 commented Dec 17, 2020

viktoriias commented Dec 17, 2020

BenHenning left a comment • edited Loading

Choose a reason for hiding this comment

viktoriias commented Dec 7, 2020 •

edited by anandwana001

Loading

anandwana001 commented Dec 8, 2020 •

edited

Loading

viktoriias commented Dec 8, 2020 •

edited

Loading

BenHenning left a comment •

edited

Loading

viktoriias commented Dec 15, 2020 •

edited

Loading

anandwana001 commented Dec 15, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading

BenHenning left a comment •

edited

Loading