Skip to content

Commit

Permalink
Address findings from #4247 and #4248.
Browse files Browse the repository at this point in the history
  • Loading branch information
BenHenning committed Mar 24, 2022
1 parent 8ca1743 commit f4ebbd8
Show file tree
Hide file tree
Showing 25 changed files with 226 additions and 326 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@ class AdministratorControlsActivity :
lastLoadedFragment = PROFILE_AND_DEVICE_ID_FRAGMENT
administratorControlsActivityPresenter.setExtraControlsTitle(
resourceHandler.getStringInLocale(R.string.profile_and_device_id_activity_title)
// TODO(LearnerAnalytics): Replace with new string
)
administratorControlsActivityPresenter.loadLearnerAnalyticsData()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,11 @@ class AdministratorControlsActivityPresenter @Inject constructor(
AdministratorControlsFragment.newInstance(isMultipane)
).commitNow()
if (isMultipane) {
val adminControlsActivity = activity as AdministratorControlsActivity
when (lastLoadedFragment) {
PROFILE_LIST_FRAGMENT -> (activity as AdministratorControlsActivity).loadProfileList()
APP_VERSION_FRAGMENT -> (activity as AdministratorControlsActivity).loadAppVersion()
PROFILE_AND_DEVICE_ID_FRAGMENT -> (
activity as AdministratorControlsActivity
).loadLearnerAnalyticsData()
PROFILE_LIST_FRAGMENT -> adminControlsActivity.loadProfileList()
APP_VERSION_FRAGMENT -> adminControlsActivity.loadAppVersion()
PROFILE_AND_DEVICE_ID_FRAGMENT -> adminControlsActivity.loadLearnerAnalyticsData()
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ class AdministratorControlsViewModel @Inject constructor(
loadProfileListListener
)
)
// TODO: Add tests to verify on/off state.
if (learnerStudyAnalytics.value) {
itemViewModelList.add(AdministratorControlsProfileAndDeviceIdViewModel(activity))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public static void setProfileLastVisitedText(@NonNull TextView textView, long ti
textView.setText(profileLastVisited);
}

// TODO: Add test for this method.
/** Binds an AndroidX KitKat-compatible drawable top to the specified text view. */
@BindingAdapter("app:drawableTopCompat")
public static void setDrawableTopCompat(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ class ProfileChooserFragmentPresenter @Inject constructor(
private fun updateLearnerIdIfAbsent(profile: Profile) {
if (profile.learnerId.isNullOrEmpty()) {
// TODO: need to block on this completing before proceeding with the login.
profileManagementController.updateLearnerId(profile.id)
profileManagementController.initializeLearnerId(profile.id)
}
}
}
1 change: 0 additions & 1 deletion domain/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ dependencies {
'androidx.exifinterface:exifinterface:1.0.0-rc01',
'androidx.lifecycle:lifecycle-livedata-ktx:2.2.0-alpha03',
'androidx.lifecycle:lifecycle-extensions:2.0.0',
'androidx.lifecycle:lifecycle-runtime-ktx:2.2.0-alpha03',
'androidx.work:work-runtime-ktx:2.4.0',
'com.google.dagger:dagger:2.24',
'com.google.firebase:firebase-analytics-ktx:17.5.0',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ kt_android_library(
"//domain/src/main/java/org/oppia/android/domain/oppialogger:oppia_logger",
"//domain/src/main/java/org/oppia/android/domain/util:extensions",
"//model/src/main/proto:event_logger_java_proto_lite",
"//third_party:com_google_firebase_firebase-installations",
"//utility/src/main/java/org/oppia/android/util/data:async_data_subscription_manager",
"//utility/src/main/java/org/oppia/android/util/data:async_result",
"//utility/src/main/java/org/oppia/android/util/data:data_providers",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import org.oppia.android.util.logging.ConsoleLogger
import javax.inject.Inject
import org.oppia.android.util.system.OppiaClock

/** Logger that handles event logging. */
/** Logger that handles general-purpose logging throughout the domain & UI layers. */
class OppiaLogger @Inject constructor(
private val analyticsController: AnalyticsController,
private val consoleLogger: ConsoleLogger,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class LogUploadWorker private constructor(
return try {
syncStatusManager.setSyncStatus(SyncStatusManager.SyncStatus.DATA_UPLOADING)
analyticsController.getEventLogStoreList().forEach { eventLog ->
eventLogger.logCachedEvent(eventLog)
eventLogger.logEvent(eventLog)
analyticsController.removeFirstEventLogFromStore()
}
syncStatusManager.setSyncStatus(SyncStatusManager.SyncStatus.DATA_UPLOADED)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import javax.inject.Inject
import javax.inject.Singleton
import org.oppia.android.domain.oppialogger.LoggingIdentifierController
import org.oppia.android.domain.oppialogger.analytics.LearnerAnalyticsLogger
import org.oppia.android.util.platformparameter.LearnerStudyAnalytics
import org.oppia.android.util.platformparameter.PlatformParameterValue

private const val GET_PROFILES_PROVIDER_ID = "get_profiles_provider_id"
private const val GET_PROFILE_PROVIDER_ID = "get_profile_provider_id"
Expand Down Expand Up @@ -73,7 +75,8 @@ class ProfileManagementController @Inject constructor(
private val oppiaClock: OppiaClock,
private val machineLocale: OppiaLocale.MachineLocale,
private val loggingIdentifierController: LoggingIdentifierController,
private val learnerAnalyticsLogger: LearnerAnalyticsLogger
private val learnerAnalyticsLogger: LearnerAnalyticsLogger,
@LearnerStudyAnalytics private val learnerStudyAnalytics: PlatformParameterValue<Boolean>
) {
private var currentProfileId: Int = -1
private val profileDataStore =
Expand Down Expand Up @@ -218,35 +221,40 @@ class ProfileManagementController @Inject constructor(
val nextProfileId = it.nextProfileId
val profileDir = directoryManagementUtil.getOrCreateDir(nextProfileId.toString())

val newProfileBuilder = Profile.newBuilder()
.setName(name)
.setPin(pin)
.setAllowDownloadAccess(allowDownloadAccess)
.setId(ProfileId.newBuilder().setInternalId(nextProfileId))
.setDateCreatedTimestampMs(oppiaClock.getCurrentTimeMs())
.setIsAdmin(isAdmin)
.setReadingTextSize(ReadingTextSize.MEDIUM_TEXT_SIZE)
.setAppLanguage(AppLanguage.ENGLISH_APP_LANGUAGE)
.setAudioLanguage(AudioLanguage.ENGLISH_AUDIO_LANGUAGE)
.setLearnerId(loggingIdentifierController.createLearnerId())

if (avatarImagePath != null) {
val imageUri =
saveImageToInternalStorage(avatarImagePath, profileDir)
?: return@storeDataWithCustomChannelAsync Pair(
it,
ProfileActionStatus.FAILED_TO_STORE_IMAGE
)
newProfileBuilder.avatar = ProfileAvatar.newBuilder().setAvatarImageUri(imageUri).build()
} else {
newProfileBuilder.avatar = ProfileAvatar.newBuilder().setAvatarColorRgb(colorRgb).build()
}
val newProfile = Profile.newBuilder().apply {
this.name = name
this.pin = pin
this.allowDownloadAccess = allowDownloadAccess
this.id = ProfileId.newBuilder().setInternalId(nextProfileId).build()
dateCreatedTimestampMs = oppiaClock.getCurrentTimeMs()
this.isAdmin = isAdmin
readingTextSize = ReadingTextSize.MEDIUM_TEXT_SIZE
appLanguage = AppLanguage.ENGLISH_APP_LANGUAGE
audioLanguage = AudioLanguage.ENGLISH_AUDIO_LANGUAGE

if (learnerStudyAnalytics.value) {
// Only set a learner ID if there's an ongoing user study.
learnerId = loggingIdentifierController.createLearnerId()
}

avatar = ProfileAvatar.newBuilder().apply {
if (avatarImagePath != null) {
val imageUri =
saveImageToInternalStorage(avatarImagePath, profileDir)
?: return@storeDataWithCustomChannelAsync Pair(
it,
ProfileActionStatus.FAILED_TO_STORE_IMAGE
)
avatarImageUri = imageUri
} else avatarColorRgb = colorRgb
}.build()
}.build()

val wasProfileEverAdded = it.profilesCount > 0

val profileDatabaseBuilder =
it.toBuilder()
.putProfiles(nextProfileId, newProfileBuilder.build())
.putProfiles(nextProfileId, newProfile)
.setWasProfileEverAdded(wasProfileEverAdded)
.setNextProfileId(nextProfileId + 1)
Pair(profileDatabaseBuilder.build(), ProfileActionStatus.SUCCESS)
Expand Down Expand Up @@ -534,11 +542,12 @@ class ProfileManagementController @Inject constructor(
}

/**
* Updates the learner ID of the profile.
* Initializes the learner ID of the specified profile (if not set), otherwise clears it if
* there's no ongoing study.
*
* @param profileId the ID corresponding to the profile being updated.
* @param profileId the ID corresponding to the profile being updated
*/
fun updateLearnerId(profileId: ProfileId): DataProvider<Any?> {
fun initializeLearnerId(profileId: ProfileId): DataProvider<Any?> {
val deferred = profileDataStore.storeDataWithCustomChannelAsync(
updateInMemoryCache = true
) {
Expand All @@ -548,7 +557,11 @@ class ProfileManagementController @Inject constructor(
ProfileActionStatus.PROFILE_NOT_FOUND
)
val updatedProfile = profile.toBuilder().apply {
learnerId = loggingIdentifierController.createLearnerId()
learnerId = when {
!learnerStudyAnalytics.value -> "" // There should be no learner ID if no ongoing study.
learnerId.isEmpty() -> loggingIdentifierController.createLearnerId() // Generate new ID.
else -> learnerId // Keep it unchanged.
}
}.build()
val profileDatabaseBuilder = it.toBuilder().putProfiles(
profileId.internalId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ kt_android_library(
kt_android_library(
name = "extensions",
srcs = [
"ContextExtensions.kt",
"InteractionObjectExtensions.kt",
"JsonExtensions.kt",
"WorkDataExtensions.kt",
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package org.oppia.android.domain.oppialogger.loguploader

import android.app.Application
import android.content.Context
import androidx.arch.core.executor.testing.InstantTaskExecutorRule
import androidx.test.core.app.ApplicationProvider
import androidx.test.ext.junit.runners.AndroidJUnit4
import androidx.test.platform.app.InstrumentationRegistry
Expand All @@ -21,11 +20,8 @@ import dagger.Component
import dagger.Module
import dagger.Provides
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.junit.MockitoJUnit
import org.mockito.junit.MockitoRule
import org.oppia.android.app.model.EventLog
import org.oppia.android.domain.oppialogger.EventLogStorageCacheSize
import org.oppia.android.domain.oppialogger.ExceptionLogStorageCacheSize
Expand Down Expand Up @@ -71,13 +67,6 @@ private const val TEST_TOPIC_ID = "test_topicId"
@LooperMode(LooperMode.Mode.PAUSED)
@Config(application = LogUploadWorkerTest.TestApplication::class)
class LogUploadWorkerTest {
@Rule
@JvmField
val mockitoRule: MockitoRule = MockitoJUnit.rule()

@get:Rule
val executorRule = InstantTaskExecutorRule()

@Inject
lateinit var networkConnectionUtil: NetworkConnectionDebugUtil

Expand Down Expand Up @@ -161,7 +150,7 @@ class LogUploadWorkerTest {
val workInfo = workManager.getWorkInfoById(request.id)

assertThat(workInfo.get().state).isEqualTo(WorkInfo.State.SUCCEEDED)
assertThat(fakeEventLogger.getMostRecentCachedEvent()).isEqualTo(eventLogTopicContext)
assertThat(fakeEventLogger.getMostRecentEvent()).isEqualTo(eventLogTopicContext)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ class ProfileManagementControllerTest {
testCoroutineDispatchers.runCurrent()

val profileId = ProfileId.newBuilder().setInternalId(2).build()
val updateProvider = profileManagementController.updateLearnerId(profileId)
val updateProvider = profileManagementController.initializeLearnerId(profileId)
monitorFactory.ensureDataProviderExecutes(updateProvider)
val profileProvider = profileManagementController.getProfile(profileId)

Expand Down Expand Up @@ -696,7 +696,6 @@ class ProfileManagementControllerTest {

@Module
class TestLoggingIdentifierModule {

companion object {
const val applicationIdSeed = 1L
}
Expand Down
5 changes: 4 additions & 1 deletion model/src/main/proto/profile.proto
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ message Profile {
// Represents user selected app-language.
AppLanguage app_language = 12;

// Represents user learner id.
// TODO: Make sure this is only populated when the feature is enabled.
// Represents a generally study-unique ID that helps identify this profile for analytics logs that
// are enabled as part of a user study. In non-user study cases this is expected to either be
// empty or a meaningless value.
string learner_id = 13;
}

Expand Down
1 change: 0 additions & 1 deletion scripts/assets/file_content_validation_checks.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@ file_content_checks {
failure_message: "String formatting and resource retrieval should go through AppLanguageResourceHandler, OppiaLocale.DisplayLocale, or OppiaLocale.MachineLocale depending on the context (see each class's documentation for details on when each should be used)."
exempted_file_name: "domain/src/main/java/org/oppia/android/domain/locale/DisplayLocaleImpl.kt"
exempted_file_name: "domain/src/main/java/org/oppia/android/domain/locale/OppiaLocale.kt"
exempted_file_name: "domain/src/main/java/org/oppia/android/domain/util/ContextExtensions.kt"
exempted_file_name: "domain/src/main/java/org/oppia/android/domain/util/JsonExtensions.kt"
exempted_file_name: "domain/src/main/java/org/oppia/android/domain/util/WorkDataExtensions.kt"
exempted_file_name: "domain/src/test/java/org/oppia/android/domain/onboarding/AppStartupStateControllerTest.kt"
Expand Down
21 changes: 10 additions & 11 deletions scripts/assets/maven_dependencies.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,16 @@ maven_dependency {
}
}
}
maven_dependency {
artifact_name: "com.google.auto.value:auto-value-annotations:1.8.1"
artifact_version: "1.8.1"
license {
license_name: "The Apache Software License, Version 2.0"
scrapable_link {
url: "https://www.apache.org/licenses/LICENSE-2.0.txt"
}
}
}
maven_dependency {
artifact_name: "com.google.code.findbugs:jsr305:3.0.2"
artifact_version: "3.0.2"
Expand Down Expand Up @@ -579,17 +589,6 @@ maven_dependency {
}
}
}
maven_dependency {
artifact_name: "com.google.firebase:firebase-installations:17.0.0"
artifact_version: "17.0.0"
license {
license_name: "The Apache Software License, Version 2.0"
original_link: "https://www.apache.org/licenses/LICENSE-2.0.txt"
scrapable_link {
url: "https://www.apache.org/licenses/LICENSE-2.0.txt"
}
}
}
maven_dependency {
artifact_name: "com.google.guava:failureaccess:1.0.1"
artifact_version: "1.0.1"
Expand Down
1 change: 0 additions & 1 deletion scripts/assets/test_file_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,6 @@ exempted_file_path: "domain/src/main/java/org/oppia/android/domain/topic/PrimeTo
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/topic/PrimeTopicAssetsControllerImpl.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/topic/PrimeTopicAssetsControllerModule.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/topic/RevisionCardRetriever.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/util/ContextExtensions.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/util/JsonAssetRetriever.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/util/JsonExtensions.kt"
exempted_file_path: "domain/src/main/java/org/oppia/android/domain/util/WorkDataExtensions.kt"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,43 +4,25 @@ import org.oppia.android.app.model.EventLog
import org.oppia.android.util.logging.EventLogger
import javax.inject.Inject
import javax.inject.Singleton
import org.oppia.android.util.logging.SyncStatusManager

/** A test specific fake for the event logger. */
@Singleton
class FakeEventLogger @Inject constructor() : EventLogger {
private val eventList = ArrayList<EventLog>()
private val cachedEventList = mutableListOf<EventLog>()

override fun logEvent(eventLog: EventLog) {
eventList.add(eventLog)
}

override fun logCachedEvent(eventLog: EventLog) {
cachedEventList.add(eventLog)
}

/** Returns the most recently logged event. */
fun getMostRecentEvent(): EventLog = eventList.last()

/** Clears all the events that are currently logged.. */
/** Clears all the events that are currently logged. */
fun clearAllEvents() = eventList.clear()

/** Checks if a certain event has been logged or not. */
fun hasEventLogged(eventLog: EventLog): Boolean = eventList.contains(eventLog)

/** Returns true if there are no events logged. */
fun noEventsPresent(): Boolean = eventList.isEmpty()

/** Returns the most recently logged cached event. */
fun getMostRecentCachedEvent(): EventLog = cachedEventList.last()

/** Clears all the cached events that are currently logged. */
fun clearAllCachedEvents() = cachedEventList.clear()

/** Checks if a certain cached event has been logged or not. */
fun hasCachedEventLogged(eventLog: EventLog): Boolean = cachedEventList.contains(eventLog)

/** Returns true if there are no cached events logged. */
fun noCachedEventsPresent(): Boolean = cachedEventList.isEmpty()
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import org.oppia.android.util.logging.SyncStatusModule
@Config(manifest = Config.NONE)
class FakeEventLoggerTest {

// TODO: Update & finalize tests in this suite.
@Inject
lateinit var fakeEventLogger: FakeEventLogger

Expand Down
Loading

0 comments on commit f4ebbd8

Please sign in to comment.