-
Notifications
You must be signed in to change notification settings - Fork 514
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 #4938: Profile Configuration and Migration [Blocked on #5457] #5387
base: onboarding-language-domain-config
Are you sure you want to change the base?
Changes from 74 commits
772d126
f0e9f23
430e995
027c745
ae9886f
5257e30
edd2151
c1698ed
441b2d0
bcc2cbf
6c1a28f
822e1fb
d71ed56
1e9c136
9c2e1d6
0b74249
db36244
35a839e
59c4665
ab36c03
c327be9
fccac67
4cd3918
2c33188
026ff1d
7304449
2cb0469
c32d2e9
cb8b21e
c827a68
5a570b2
381c883
bdcf5c1
0a59cfc
5ee3a20
df789ea
a1b0964
8d8ab17
61a7724
5512216
a0fe5c6
2bf2eda
d76307b
844c0e2
9ba0b9b
36f3cf6
de5783c
7564eba
56f668a
7a97741
8efa8b9
318334f
0bf61cc
cd25696
eb64975
e7ee78a
e942263
9a8024b
ef1fd91
666ad36
9ad283a
5503aee
0133cd0
d982d06
0fc0078
e06d28f
6cf179f
1cc21bb
946c79b
ca334d6
831fa1c
b94a0f8
b3d6ee3
daea15f
0397de7
063b97a
73105a0
1a4d82d
0ceb9fc
28d2742
6ae7968
6a5634e
7d2c1a7
0047916
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package org.oppia.android.app.classroom | |
import android.view.LayoutInflater | ||
import android.view.View | ||
import android.view.ViewGroup | ||
import androidx.activity.OnBackPressedCallback | ||
import androidx.appcompat.app.AppCompatActivity | ||
import androidx.compose.foundation.ExperimentalFoundationApi | ||
import androidx.compose.foundation.background | ||
|
@@ -37,6 +38,7 @@ import org.oppia.android.app.classroom.promotedlist.PromotedStoryList | |
import org.oppia.android.app.classroom.topiclist.AllTopicsHeaderText | ||
import org.oppia.android.app.classroom.topiclist.TopicCard | ||
import org.oppia.android.app.classroom.welcome.WelcomeText | ||
import org.oppia.android.app.home.ExitProfileListener | ||
import org.oppia.android.app.home.HomeItemViewModel | ||
import org.oppia.android.app.home.RouteToTopicPlayStoryListener | ||
import org.oppia.android.app.home.WelcomeViewModel | ||
|
@@ -50,6 +52,9 @@ import org.oppia.android.app.model.AppStartupState | |
import org.oppia.android.app.model.ClassroomSummary | ||
import org.oppia.android.app.model.LessonThumbnail | ||
import org.oppia.android.app.model.LessonThumbnailGraphic | ||
import org.oppia.android.app.model.Profile | ||
import org.oppia.android.app.model.ProfileId | ||
import org.oppia.android.app.model.ProfileType | ||
import org.oppia.android.app.model.TopicSummary | ||
import org.oppia.android.app.translation.AppLanguageResourceHandler | ||
import org.oppia.android.app.utility.datetime.DateTimeUtil | ||
|
@@ -66,6 +71,8 @@ import org.oppia.android.util.data.DataProviders.Companion.toLiveData | |
import org.oppia.android.util.locale.OppiaLocale | ||
import org.oppia.android.util.parser.html.StoryHtmlParserEntityType | ||
import org.oppia.android.util.parser.html.TopicHtmlParserEntityType | ||
import org.oppia.android.util.platformparameter.EnableOnboardingFlowV2 | ||
import org.oppia.android.util.platformparameter.PlatformParameterValue | ||
import org.oppia.android.util.profile.CurrentUserProfileIdIntentDecorator.extractCurrentUserProfileId | ||
import javax.inject.Inject | ||
|
||
|
@@ -88,8 +95,11 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
private val machineLocale: OppiaLocale.MachineLocale, | ||
private val appStartupStateController: AppStartupStateController, | ||
private val analyticsController: AnalyticsController, | ||
@EnableOnboardingFlowV2 | ||
private val enableOnboardingFlowV2: PlatformParameterValue<Boolean> | ||
) { | ||
private val routeToTopicPlayStoryListener = activity as RouteToTopicPlayStoryListener | ||
private val exitProfileListener = activity as ExitProfileListener | ||
private lateinit var binding: ClassroomListFragmentBinding | ||
private lateinit var classroomListViewModel: ClassroomListViewModel | ||
private var internalProfileId: Int = -1 | ||
|
@@ -134,7 +144,8 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
sender: ObservableList<HomeItemViewModel>, | ||
positionStart: Int, | ||
itemCount: Int | ||
) {} | ||
) { | ||
} | ||
|
||
override fun onItemRangeInserted( | ||
sender: ObservableList<HomeItemViewModel>, | ||
|
@@ -149,17 +160,23 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
fromPosition: Int, | ||
toPosition: Int, | ||
itemCount: Int | ||
) {} | ||
) { | ||
} | ||
|
||
override fun onItemRangeRemoved( | ||
sender: ObservableList<HomeItemViewModel>, | ||
positionStart: Int, | ||
itemCount: Int | ||
) {} | ||
) { | ||
} | ||
} | ||
) | ||
|
||
logAppOnboardedEvent() | ||
if (enableOnboardingFlowV2.value) { | ||
subscribeToProfileResult(profileId) | ||
} else { | ||
logAppOnboardedEvent(profileId) | ||
} | ||
|
||
return binding.root | ||
} | ||
|
@@ -265,7 +282,7 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
} | ||
} | ||
|
||
private fun logAppOnboardedEvent() { | ||
private fun logAppOnboardedEvent(profileId: ProfileId) { | ||
val startupStateProvider = appStartupStateController.getAppStartupState() | ||
val liveData = startupStateProvider.toLiveData() | ||
liveData.observe( | ||
|
@@ -274,7 +291,7 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
override fun onChanged(startUpStateResult: AsyncResult<AppStartupState>?) { | ||
when (startUpStateResult) { | ||
null, is AsyncResult.Pending -> { | ||
// Do nothing. | ||
// Do nothing | ||
} | ||
is AsyncResult.Success -> { | ||
liveData.removeObserver(this) | ||
|
@@ -297,12 +314,71 @@ class ClassroomListFragmentPresenter @Inject constructor( | |
) | ||
} | ||
|
||
private fun subscribeToProfileResult(profileId: ProfileId) { | ||
profileManagementController.getProfile(profileId).toLiveData().observe(fragment) { | ||
processProfileResult(it) | ||
} | ||
} | ||
|
||
private fun processProfileResult(result: AsyncResult<Profile>) { | ||
when (result) { | ||
is AsyncResult.Success -> { | ||
val profile = result.value | ||
handleProfileOnboardingState(profile) | ||
handleBackPress(profile.profileType) | ||
} | ||
is AsyncResult.Failure -> { | ||
oppiaLogger.e( | ||
"ClassroomListFragment", "Failed to fetch profile with id:$profileId", result.error | ||
) | ||
Profile.getDefaultInstance() | ||
} | ||
is AsyncResult.Pending -> { | ||
Profile.getDefaultInstance() | ||
} | ||
} | ||
} | ||
|
||
private fun handleProfileOnboardingState(profile: Profile) { | ||
// App onboarding is completed by the first profile on the app(SOLE_LEARNER or SUPERVISOR), | ||
// while profile onboarding is completed by each profile. | ||
if (!profile.completedProfileOboarding) { | ||
markProfileOnboardingEnded(profileId) | ||
if (profile.profileType == ProfileType.SOLE_LEARNER || | ||
profile.profileType == ProfileType.SUPERVISOR | ||
) { | ||
appStartupStateController.markOnboardingFlowCompleted() | ||
logAppOnboardedEvent(profileId) | ||
} | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm quite uncomfortable, in general, with logging events in the app layer because its lifecycle is incredibly complex. It's very easily to accidentally duplicate logs (e.g. due to activity recreation, etc.). Can we log this in the domain layer, instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have moved this to the controller. |
||
|
||
private fun markProfileOnboardingEnded(profileId: ProfileId) { | ||
profileManagementController.markProfileOnboardingEnded(profileId) | ||
|
||
analyticsController.logLowPriorityEvent( | ||
oppiaLogger.createProfileOnboardingEndedContext(profileId), | ||
profileId = profileId | ||
) | ||
} | ||
|
||
private fun logHomeActivityEvent() { | ||
analyticsController.logImportantEvent( | ||
oppiaLogger.createOpenHomeContext(), | ||
profileId | ||
) | ||
} | ||
|
||
private fun handleBackPress(profileType: ProfileType) { | ||
activity.onBackPressedDispatcher.addCallback( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the profile provider changes multiple times, it will lead to multiple callbacks being registered. Does this code correctly handle that scenario? |
||
fragment, | ||
object : OnBackPressedCallback(true) { | ||
override fun handleOnBackPressed() { | ||
exitProfileListener.exitProfile(profileType) | ||
} | ||
} | ||
) | ||
} | ||
} | ||
|
||
/** Adds a grid of items to a LazyListScope with specified arrangement and item content. */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
package org.oppia.android.app.home | ||
|
||
import org.oppia.android.app.model.ProfileType | ||
/** Listener for when a user wishes to exit their profile. */ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: empty line above this please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed |
||
interface ExitProfileListener { | ||
/** | ||
* Called when back press is clicked on the HomeScreen. | ||
* | ||
* A SOLE_LEARNER exits the app completely while other [ProfileType]s are routed to the | ||
* [ProfileChooserActivity]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest instead saying that routing behavior may change based on the profile type rather than specifying the actual implementation behavior (which seems a bit too specific at the interface level). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated. |
||
*/ | ||
fun exitProfile(profileType: ProfileType) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to log this for all cases where the user successfully finishes the onboarding flow (there's a bug with it currently whereby it logs for every home screen, and we're looking into it).
It acts as a top-level app health metric, so please make sure to account for it with the new onboarding changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscribeToProfileResult()
>processProfileResult()
>handleProfileOnboardingState(profile: Profile)
ensures that all users who complete the new onboarding flow log astarted_profile_onboarding
andcompleted_profile_onboarding
event. At the same time, the first profile on the app(supervisor or sole learner) will additionally log the app onboarding complete event(existing event).In this PR, I have retained the legacy onboarding flow to only log the app onboarding event.
I implemented this per TDD, although it is different from your comment(sounds like we want all profiles to log all 3 events?). Could you please clarify the intended behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seperately, I experienced the bug you mention, and given some context I may be able to pick up debugging from my prior attempt at solving the #5434 (closed due to reverting adjacent changes that may have triggered the bug for me).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the discussion in the meeting, I left the implementation of the logAppOnboardedEvent as is.