Fix #936 & #2297 : Oppia Android Promoted Stories App Layer. #2320

veena14cs · 2020-12-26T11:50:20Z

Explanation

This PR is the app layer implementation of promoted stories and coming soon topics on the home screen. This PR fixes issues #936 & #2297 .

Design Document
https://docs.google.com/document/d/1o7hOz1rP-hJaKn1f5x6LV6ms0bzhyCpsTifJQY5crCc/edit?ts=5f9fc81b#heading=h.ovlu0iazlug6

Mock Links

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.

app/src/main/java/org/oppia/android/app/home/HomeFragmentPresenter.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicsListView.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt

rt4914

@veena14cs I did the very initial pass but there are lots of nit changes and lint issues. I suggest doing a self-PR-Review or solving the lint-test fail errors, so that it becomes easy to review and more focused towards implementation.

The initial implementation approach looks good and works correctly too.

Thanks @veena14cs

.idea/gradle.xml

model/src/main/proto/topic.proto

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

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicsViewModel.kt

anandwana001

Did review on few files.
Tried running app but didn't see the list item.

Aalso for bazel, you need to add any new listener, viewmodel to app/BUILD.bazel file

app/src/main/java/org/oppia/android/app/home/HomeFragmentPresenter.kt

app/src/main/java/org/oppia/android/app/home/HomeItemViewModel.kt

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicsViewModel.kt

veena14cs · 2020-12-30T18:57:27Z

Did review on few files.
Tried running app but didn't see the list item.

Aalso for bazel, you need to add any new listener, viewmodel to app/BUILD.bazel file

Did you try playing any of the explorations. Initially, we don't have any learner history we don't no what topics to recommend , so when user selects any topic and plays, based on this selection we recommend him next topic's first chapter.

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt

anandwana001

add ComingSoonTopicListViewModel ComingSoonTopicsViewModel to

oppia-android/app/BUILD.bazel

Line 146 in 2511715

VIEW_MODELS = [
add ComingSoonTopicsListView to

oppia-android/app/BUILD.bazel

Line 309 in 2511715

VIEWS = [
Tests are failing, let me know if we can fix it together and save time as the number of test cases is big.

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

aggarwalpulkit596 · 2020-12-31T06:05:19Z

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

-   * (see [PromotedStoryListViewModel]), or null if this profile does not have any promoted stories.
-   * Promoted stories are determined by any recent stories started by this profile.
+   * Returns a [HomeItemViewModel] corresponding to the promoted stories[PromotedStoryListViewModel] and Upcoming topics
+   * [ComingSoonTopicListViewModel]  to be displayed for this learner or null if this profile does not have any promoted stories.


Can we change the context to recommended here instead of using keywords like promoted i was a bit confused at first seeing promoted and recommended both.

I would defer this to @rt4914 and @BenHenning.

Recommended and Promoted have different meanings.
Promoted simply meaning anything which is getting promoted to home-screen carousel. It can be either a Recommended Story, RecentlyPlayed Story or Upcoming Topic.

Recommended, means we are suggesting user something which will be the best logical step for them.

Thanks @rt4914 for making this clear.

aggarwalpulkit596 · 2020-12-31T06:07:54Z

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

-      )
-    } else null
+    when (recommendedActivityList.recommendationTypeCase) {
+      RecommendedActivityList.RecommendationTypeCase.RECOMMENDED_STORY_LIST -> {


Is RecommendationTypeCase is an enum ? if yes then we don't need to handle the else case here

Its not an enum but oneOf at most one field will be set at the same time. There can be field called TYPE_NOT_SET. To handle this I have added else case.

Protobuf enum can always, unfortunately, be unspecified so we can't rely on exhaustive when statements. :( Hopefully this gets addressed when there's a Kotlin-compatible protobuf compiler.

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicsListView.kt

aggarwalpulkit596 · 2020-12-31T06:15:41Z

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt

  /** Returns the padding placed at the end of the promoted stories list based on the number of promoted stories. */
  val endPadding =
    if (promotedStoryList.size > 1)
      activity.resources.getDimensionPixelSize(R.dimen.home_padding_end)
    else activity.resources.getDimensionPixelSize(R.dimen.home_padding_start)

  /** Determines and returns the visibility for the "View All" button. */
+  fun getHeader(): String {
+    recommendedActivityList.recommendedStoryList.let {


Oh instead of using let we could with as that would be more appropriate here and elsewhere as it is an scope function which would change the scope and then we won't require to write even it.

@aggarwalpulkit596 can you please suggest me how I can use with here instead of let.

with(recommendedActivityList.recommendedStoryList) { return when { suggestedStoryList.isNotEmpty() -> { if (recentlyPlayedStoryList.isNotEmpty() || olderPlayedStoryList.isNotEmpty()) { activity.getString(R.string.stories_for_you) } else activity.getString(R.string.recommended_stories) } recentlyPlayedStoryList.isNotEmpty() -> { activity.getString(R.string.recently_played_stories) } else -> { activity.getString(R.string.last_played_stories) } } }

with changes the current scope to the object so here this is referring to recommendedActivityList.recommendedStoryList and since it is implicitly applied here we don't have to write this here. Does that make sense ?

Yes, got it done. Thanks Pulkit! :)

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt

aggarwalpulkit596 · 2020-12-31T06:20:57Z

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

+      type="org.oppia.android.app.home.promotedlist.ComingSoonTopicListViewModel" />
+  </data>
+
+  <LinearLayout


Just for consistency, we should use constraint layout @rt4914 @BenHenning

Deferring it to @rt4914 @BenHenning

@aggarwalpulkit596 Makes sense. @veena14cs Can you update this and other files to use ConstraintLayout

anandwana001 · 2021-01-04T03:41:46Z

@veena14cs Looks like tests are failing.
One example

fun testHomeActivity_recyclerViewIndex4_topicSummary_configurationChange_lessonCountIsCorrect()

 androidx.test.espresso.base.DefaultFailureHandler$AssertionFailedWithCauseError: 'with text: a string containing "1 Lesson"' doesn't match the selected view.
    Expected: with text: a string containing "1 Lesson"

rt4914

@veena14cs the base branch of this PR should be promoted-story right?

app/BUILD.bazel

rt4914 · 2021-01-04T04:48:12Z

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

-   * (see [PromotedStoryListViewModel]), or null if this profile does not have any promoted stories.
-   * Promoted stories are determined by any recent stories started by this profile.
+   * Returns a [HomeItemViewModel] corresponding to the promoted stories[PromotedStoryListViewModel] and Upcoming topics
+   * [ComingSoonTopicListViewModel]  to be displayed for this learner or null if this profile does not have any promoted stories.


Recommended and Promoted have different meanings.
Promoted simply meaning anything which is getting promoted to home-screen carousel. It can be either a Recommended Story, RecentlyPlayed Story or Upcoming Topic.

Recommended, means we are suggesting user something which will be the best logical step for them.

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicListViewModel.kt

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt

app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt

rt4914 · 2021-01-04T04:56:29Z

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

+      type="org.oppia.android.app.home.promotedlist.ComingSoonTopicListViewModel" />
+  </data>
+
+  <LinearLayout


@aggarwalpulkit596 Makes sense. @veena14cs Can you update this and other files to use ConstraintLayout

app/src/main/res/layout-sw600dp-land/coming_soon_topic_view.xml

…oppia-android into promoted-story-part-1

…art-1

FareesHussain

LGTM, thanks.
All the tests in HomeActivityTest are passing for tablet now.

veena14cs · 2021-02-12T10:09:23Z

LGTM, thanks.
All the tests in HomeActivityTest are passing for tablet now.

Thanks @FareesHussain .

veena14cs · 2021-02-12T10:18:05Z

@anandwana001 I have addressed all your comments. PTAL.

anandwana001

LGTM, Thanks @veena14cs

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt

…yTest.kt Co-authored-by: Ben Henning <[email protected]>

BenHenning · 2021-02-12T18:05:24Z

@veena14cs just a few comments left, plus we need to update this after #2707 is merged in. Note that we can ignore the non-required failing Bazel tests & merge it once the Bazel build workflow is passing again.

…yTest.kt Co-authored-by: Ben Henning <[email protected]>

…art-1

BenHenning

Thanks @veena14cs! PR LGTM, and the latest changes are passing. I needed to update the branch to develop, but it seems solid. Will ensure this gets merged quickly (since the Bazel tests aren't required yet).

Dismissing as a non-required reviewer & this is a time-sensitive PR.

BenHenning · 2021-02-13T03:25:52Z

@aggarwalpulkit596 FYI I cleared your review since you were requesting changes & this PR is a bit time sensitive. Please follow up with an issue for any additional changes you think are needed here.

BenHenning · 2021-02-13T04:00:11Z

(Cancelled the Bazel tests sort of accidentally due to cancelling other workflows to free up CI resources, but it should be fine).

veena14cs requested a review from rt4914 as a code owner December 26, 2020 11:50

veena14cs changed the base branch from develop to refactor-topic-list-adapter-to-use-bindable-adapter-veena December 26, 2020 18:48

veena14cs requested review from BenHenning and anandwana001 December 26, 2020 19:04

veena14cs assigned BenHenning, aggarwalpulkit596 and rt4914 Dec 26, 2020

aggarwalpulkit596 reviewed Dec 26, 2020

View reviewed changes

app/src/main/java/org/oppia/android/app/home/HomeFragmentPresenter.kt Show resolved Hide resolved

aggarwalpulkit596 reviewed Dec 26, 2020

View reviewed changes

app/src/main/java/org/oppia/android/app/home/promotedlist/ComingSoonTopicsListView.kt Show resolved Hide resolved

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt Outdated Show resolved Hide resolved

rt4914 suggested changes Dec 28, 2020

View reviewed changes

rt4914 assigned veena14cs and unassigned rt4914 Dec 28, 2020

veena14cs requested review from aggarwalpulkit596 and rt4914 December 30, 2020 10:59

veena14cs assigned rt4914 and anandwana001 and unassigned veena14cs Dec 30, 2020

veena14cs mentioned this pull request Dec 30, 2020

Oppia Android Promoted Stories Domain Layer #2272

Merged

8 tasks

anandwana001 suggested changes Dec 30, 2020

View reviewed changes

veena14cs requested a review from anandwana001 December 30, 2020 18:51

anandwana001 reviewed Dec 31, 2020

View reviewed changes

app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModel.kt Outdated Show resolved Hide resolved

anandwana001 suggested changes Dec 31, 2020

View reviewed changes

aggarwalpulkit596 previously requested changes Dec 31, 2020

View reviewed changes

veena14cs requested review from anandwana001 and aggarwalpulkit596 December 31, 2020 09:16

anandwana001 removed their assignment Jan 4, 2021

rt4914 suggested changes Jan 4, 2021

View reviewed changes

rt4914 removed their assignment Jan 4, 2021

veena14cs added 2 commits February 12, 2021 15:07

Merge branch 'promoted-story-part-1' of https://github.com/veena14cs/…

0ccb5e5

…oppia-android into promoted-story-part-1

Merge remote-tracking branch 'upstream/develop' into promoted-story-p…

2269334

…art-1

FareesHussain approved these changes Feb 12, 2021

View reviewed changes

FareesHussain removed their assignment Feb 12, 2021

anandwana001 approved these changes Feb 12, 2021

View reviewed changes

anandwana001 removed their assignment Feb 12, 2021

BenHenning reviewed Feb 12, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved

BenHenning reviewed Feb 12, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved

BenHenning reviewed Feb 12, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved

Update app/src/sharedTest/java/org/oppia/android/app/home/HomeActivit…

a6f342e

…yTest.kt Co-authored-by: Ben Henning <[email protected]>

veena14cs and others added 4 commits February 13, 2021 00:28

Update app/src/sharedTest/java/org/oppia/android/app/home/HomeActivit…

67f5664

…yTest.kt Co-authored-by: Ben Henning <[email protected]>

Update app/src/sharedTest/java/org/oppia/android/app/home/HomeActivit…

252014a

…yTest.kt Co-authored-by: Ben Henning <[email protected]>

Merge remote-tracking branch 'upstream/develop' into promoted-story-p…

353a3a4

…art-1

Update TopicListController.kt

45a6b18

veena14cs mentioned this pull request Feb 12, 2021

Fix initial recommendation #2708

Merged

8 tasks

veena14cs requested a review from BenHenning February 12, 2021 19:50

veena14cs assigned BenHenning and unassigned veena14cs Feb 12, 2021

Merge branch 'develop' into promoted-story-part-1

d42f6c8

BenHenning enabled auto-merge (squash) February 13, 2021 03:23

BenHenning approved these changes Feb 13, 2021

View reviewed changes

BenHenning added 2 commits February 12, 2021 19:42

Merge branch 'develop' into promoted-story-part-1

d524dac

Merge branch 'develop' into promoted-story-part-1

e5c047b

BenHenning merged commit d2d5501 into oppia:develop Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #936 & #2297 : Oppia Android Promoted Stories App Layer. #2320

Fix #936 & #2297 : Oppia Android Promoted Stories App Layer. #2320

veena14cs commented Dec 26, 2020 •

edited

Loading

rt4914 left a comment

anandwana001 left a comment •

edited

Loading

veena14cs commented Dec 30, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading

aggarwalpulkit596 Dec 31, 2020

veena14cs Dec 31, 2020

rt4914 Jan 4, 2021

veena14cs Jan 5, 2021

aggarwalpulkit596 Dec 31, 2020

veena14cs Dec 31, 2020 •

edited

Loading

BenHenning Jan 9, 2021 •

edited

Loading

aggarwalpulkit596 Dec 31, 2020

veena14cs Jan 6, 2021 •

edited

Loading

aggarwalpulkit596 Jan 6, 2021

aggarwalpulkit596 Jan 6, 2021 •

edited

Loading

veena14cs Jan 6, 2021

aggarwalpulkit596 Dec 31, 2020

veena14cs Dec 31, 2020

rt4914 Jan 4, 2021

veena14cs Jan 5, 2021

anandwana001 commented Jan 4, 2021

rt4914 left a comment

rt4914 Jan 4, 2021

rt4914 Jan 4, 2021

FareesHussain left a comment •

edited

Loading

veena14cs commented Feb 12, 2021

veena14cs commented Feb 12, 2021

anandwana001 left a comment

BenHenning commented Feb 12, 2021

BenHenning left a comment

BenHenning commented Feb 13, 2021

BenHenning commented Feb 13, 2021

Fix #936 & #2297 : Oppia Android Promoted Stories App Layer. #2320

Fix #936 & #2297 : Oppia Android Promoted Stories App Layer. #2320

Conversation

veena14cs commented Dec 26, 2020 • edited Loading

Explanation

Checklist

rt4914 left a comment

Choose a reason for hiding this comment

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

veena14cs commented Dec 30, 2020 • edited Loading

anandwana001 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veena14cs Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

BenHenning Jan 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

veena14cs Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aggarwalpulkit596 Jan 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandwana001 commented Jan 4, 2021

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FareesHussain left a comment • edited Loading

Choose a reason for hiding this comment

veena14cs commented Feb 12, 2021

veena14cs commented Feb 12, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

BenHenning commented Feb 12, 2021

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Feb 13, 2021

BenHenning commented Feb 13, 2021

veena14cs commented Dec 26, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading

veena14cs commented Dec 30, 2020 •

edited

Loading

anandwana001 left a comment •

edited

Loading

veena14cs Dec 31, 2020 •

edited

Loading

BenHenning Jan 9, 2021 •

edited

Loading

veena14cs Jan 6, 2021 •

edited

Loading

aggarwalpulkit596 Jan 6, 2021 •

edited

Loading

FareesHussain left a comment •

edited

Loading