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.

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

Technical Analytics: Milestone 1 - Add Feature Flag Statuses and Ability To Sync Them to Cache Store #5203

Merged
merged 37 commits into from
Nov 29, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
3fee528
feat: Add a FeatureFlagConstants file to host feature flags
kkmurerwa Oct 18, 2023
9c09fbf
feat: Add constant names for easy retrieval of the feature flags from…
kkmurerwa Oct 18, 2023
b0d1572
feat: Add newly status flags to the Platform Parameter dependency mod…
kkmurerwa Oct 18, 2023
7e13530
feat: Add ability to insert sync statuses to the cache store
kkmurerwa Oct 20, 2023
6c06ea5
feat: Add ability to save flag statuses for every feature flag
kkmurerwa Oct 23, 2023
5e26d8e
Merge branch 'oppia:develop' into technical-analytics-milestone-1
kkmurerwa Oct 23, 2023
1f0fa64
fix: Fix linting and styling checks
kkmurerwa Oct 23, 2023
f4f5ff2
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Oct 24, 2023
795e90c
fix: Fix comments made on the previous review
kkmurerwa Oct 26, 2023
dd5c5b2
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Oct 26, 2023
da852b4
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Oct 29, 2023
9da1607
fix: Fix nit
kkmurerwa Oct 29, 2023
0c0cd75
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Oct 30, 2023
4dae83c
fix: Fix lint issues causing build failure
kkmurerwa Oct 30, 2023
81d968b
fix: Move test sync status flag booleans to the TestBooleanPlatformPa…
kkmurerwa Oct 30, 2023
d8f14bd
chore: Move all test constants from individual type-organized files t…
kkmurerwa Nov 1, 2023
7918da5
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 1, 2023
7bcbdcf
fix: Fix failing text file checks
kkmurerwa Nov 1, 2023
ed2b7ac
Merge branch 'technical-analytics-milestone-1' of github.com:kkmurerw…
kkmurerwa Nov 1, 2023
5638b31
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 1, 2023
a1846ce
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 6, 2023
b49df60
feat: Add a isSynced variable in the platform parameter variable to s…
kkmurerwa Nov 7, 2023
7ee1fd8
fix: Fix failing EventBundleCreator tests
kkmurerwa Nov 7, 2023
b9f28ba
feat: Make Sync status an enum
kkmurerwa Nov 14, 2023
c0338b0
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 14, 2023
501ae40
feat: Add feature flag names to all flags lacking one
kkmurerwa Nov 14, 2023
91c526d
Merge branch 'technical-analytics-milestone-1' of github.com:kkmurerw…
kkmurerwa Nov 14, 2023
551fd4a
fix: Fix failing check and linting issue
kkmurerwa Nov 15, 2023
e6d748f
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 15, 2023
3b19e3f
merge: Merge from develop and fix conflicts
kkmurerwa Nov 17, 2023
df983b9
chore: Remove unused feature flag
kkmurerwa Nov 17, 2023
d3ca9d0
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 21, 2023
7b6a4f7
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 22, 2023
8f1d1cc
fix: Fix minor linting issues
kkmurerwa Nov 24, 2023
0a58483
Merge branch 'develop' into technical-analytics-milestone-1
BenHenning Nov 29, 2023
92c4703
Merge branch 'develop' into technical-analytics-milestone-1
kkmurerwa Nov 29, 2023
3e203c4
chore: Modify the PlatformParameterValue default sync status name
kkmurerwa Nov 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class PlatformParameterSingletonImpl @Inject constructor() : PlatformParameterSi
return object : PlatformParameterValue<String> {
override val value: String
get() = parameter.string
override val isSynced: Boolean
get() = parameter.isSynced
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -38,6 +40,8 @@ class PlatformParameterSingletonImpl @Inject constructor() : PlatformParameterSi
return object : PlatformParameterValue<Int> {
override val value: Int
get() = parameter.integer
override val isSynced: Boolean
get() = parameter.isSynced
}
}

Expand All @@ -50,6 +54,8 @@ class PlatformParameterSingletonImpl @Inject constructor() : PlatformParameterSi
return object : PlatformParameterValue<Boolean> {
override val value: Boolean
get() = parameter.boolean
override val isSynced: Boolean
get() = parameter.isSynced
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ class PlatformParameterSyncUpWorker private constructor(
*/
private fun parseNetworkResponse(response: Map<String, Any>): List<PlatformParameter> {
return response.map {
val platformParameter = PlatformParameter.newBuilder().setName(it.key)
val platformParameter = PlatformParameter.newBuilder().setName(it.key).setIsSynced(true)
when (val value = it.value) {
is String -> platformParameter.string = value
is Int -> platformParameter.integer = value
Expand Down Expand Up @@ -111,6 +111,7 @@ class PlatformParameterSyncUpWorker private constructor(
if (response != null) {
val responseBody = checkNotNull(response.body())
val platformParameterList = parseNetworkResponse(responseBody)

if (platformParameterList.isEmpty()) {
throw IllegalArgumentException(EMPTY_RESPONSE_EXCEPTION_MSG)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,7 @@ class AudioPlayerControllerTest {
val enableFeature = enableLearnerStudyAnalytics
return object : PlatformParameterValue<Boolean> {
override val value: Boolean = enableFeature
override val isSynced: Boolean = false
}
}

Expand All @@ -854,6 +855,7 @@ class AudioPlayerControllerTest {
val enableFeature = enableLearnerStudyAnalytics
return object : PlatformParameterValue<Boolean> {
override val value: Boolean = enableFeature
override val isSynced: Boolean = false
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,21 +95,25 @@ class PlatformParameterSyncUpWorkerTest {
private val expectedTestStringParameter = PlatformParameter.newBuilder()
.setName(TEST_STRING_PARAM_NAME)
.setString(TEST_STRING_PARAM_SERVER_VALUE)
.setIsSynced(true)
.build()

private val expectedTestIntegerParameter = PlatformParameter.newBuilder()
.setName(TEST_INTEGER_PARAM_NAME)
.setInteger(TEST_INTEGER_PARAM_SERVER_VALUE)
.setIsSynced(true)
.build()

private val defaultTestIntegerParameter = PlatformParameter.newBuilder()
.setName(TEST_INTEGER_PARAM_NAME)
.setInteger(TEST_INTEGER_PARAM_DEFAULT_VALUE)
.setIsSynced(false)
.build()

private val expectedTestBooleanParameter = PlatformParameter.newBuilder()
.setName(TEST_BOOLEAN_PARAM_NAME)
.setBoolean(TEST_BOOLEAN_PARAM_SERVER_VALUE)
.setIsSynced(true)
.build()

// Not including "expectedTestBooleanParameter" in this list to prove that a refresh took place
Expand Down Expand Up @@ -322,6 +326,89 @@ class PlatformParameterSyncUpWorkerTest {
.containsEntry(TEST_INTEGER_PARAM_NAME, defaultTestIntegerParameter)
}

@Test
fun testSyncUpWorker_getFeatureFlags_addSyncStatusFlags_verifyCorrectStatusReturned() {
// Set up versionName to get correct network response from mock platform parameter service.
setUpApplicationForContext(MockPlatformParameterService.appVersionForCorrectResponse)

// Empty the Platform Parameter Database to simulate the execution of first SyncUp Work request.
platformParameterController.updatePlatformParameterDatabase(listOf())

val workManager = WorkManager.getInstance(context)

val inputData = Data.Builder().putString(
PlatformParameterSyncUpWorker.WORKER_TYPE_KEY,
PlatformParameterSyncUpWorker.PLATFORM_PARAMETER_WORKER
).build()

val request: OneTimeWorkRequest = OneTimeWorkRequestBuilder<PlatformParameterSyncUpWorker>()
.setInputData(inputData)
.build()

// Enqueue the Work Request to fetch and cache the Platform Parameters from Remote Service.
workManager.enqueue(request)
testCoroutineDispatchers.runCurrent()

val workInfo = workManager.getWorkInfoById(request.id)
assertThat(workInfo.get().state).isEqualTo(WorkInfo.State.SUCCEEDED)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved

// Retrieve the previously cached Platform Parameters from Cache Store.
monitorFactory.ensureDataProviderExecutes(platformParameterController.getParameterDatabase())

// Values retrieved from Cache store will be sent to Platform Parameter Singleton by the
// Controller in the form of a Map, therefore verify the retrieved values from that Map.
val platformParameterMap = platformParameterSingleton.getPlatformParameterMap()

// Previous String Platform Parameter is still same in the Database.
assertThat(platformParameterMap)
.containsEntry(TEST_STRING_PARAM_NAME, expectedTestStringParameter)

// Is synced boolean of the platform parameter is true
assertThat(platformParameterMap[TEST_STRING_PARAM_NAME]!!.isSynced).isEqualTo(true)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
fun testSyncUpWorker_databaseNotEmpty_getEmptyResponse_verifySyncStatusNotUpdated() {
// Set up versionName to get incorrect network response from mock platform parameter service.
setUpApplicationForContext(MockPlatformParameterService.appVersionForEmptyResponse)

// Fill the Platform Parameter Database with mock values to simulate the execution of a SyncUp
// Work request that is not first.
platformParameterController.updatePlatformParameterDatabase(mockPlatformParameterList)

val workManager = WorkManager.getInstance(context)

val inputData = Data.Builder().putString(
PlatformParameterSyncUpWorker.WORKER_TYPE_KEY,
PlatformParameterSyncUpWorker.PLATFORM_PARAMETER_WORKER
).build()

val request: OneTimeWorkRequest = OneTimeWorkRequestBuilder<PlatformParameterSyncUpWorker>()
.setInputData(inputData)
.build()

// Enqueue the Work Request to fetch and cache the Platform Parameters from Remote Service.
workManager.enqueue(request)
testCoroutineDispatchers.runCurrent()

val workInfo = workManager.getWorkInfoById(request.id)
assertThat(workInfo.get().state).isEqualTo(WorkInfo.State.FAILED)

val exceptionMessage = fakeExceptionLogger.getMostRecentException().message
assertThat(exceptionMessage)
.isEqualTo(PlatformParameterSyncUpWorker.EMPTY_RESPONSE_EXCEPTION_MSG)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved

// Retrieve the previously cached Platform Parameters from Cache Store.
monitorFactory.ensureDataProviderExecutes(platformParameterController.getParameterDatabase())

// Values retrieved from Cache store will be sent to Platform Parameter Singleton by the
// Controller in the form of a Map, therefore verify the retrieved values from that Map.
val platformParameterMap = platformParameterSingleton.getPlatformParameterMap()

// Previous integer sync status is still the same in the database
assertThat(platformParameterMap[TEST_INTEGER_PARAM_NAME]!!.isSynced).isEqualTo(false)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}

private fun setUpTestApplicationComponent() {
ApplicationProvider.getApplicationContext<TestApplication>().inject(this)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1358,6 +1358,7 @@ class ProfileManagementControllerTest {
val enableFeature = enableLearnerStudyAnalytics
return object : PlatformParameterValue<Boolean> {
override val value: Boolean = enableFeature
override val isSynced: Boolean = true
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand All @@ -1369,6 +1370,7 @@ class ProfileManagementControllerTest {
val enableFeature = enableLearnerStudyAnalytics
return object : PlatformParameterValue<Boolean> {
override val value: Boolean = enableFeature
override val isSynced: Boolean = true
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions model/src/main/proto/platform_parameter.proto
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ message PlatformParameter {
// Indicates a string-typed platform parameter.
string string = 4;
}
// Indicates that the platform parameter is synced
bool is_synced = 5;
kkmurerwa marked this conversation as resolved.
Show resolved Hide resolved
}

// Format of platform parameters stored on disk. It closely resembles the JSON response in cache.
Expand Down
5 changes: 2 additions & 3 deletions scripts/assets/test_file_exemptions.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -798,10 +798,8 @@ exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/Moc
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/MockSubtopicService.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/MockTopicService.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/network/RetrofitTestModule.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/platformparameter/TestBooleanPlatformParameter.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/platformparameter/TestIntegerPlatformParameter.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/platformparameter/TestPlatformParameterConstants.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/platformparameter/TestPlatformParameterModule.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/platformparameter/TestStringPlatformParameter.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/robolectric/IsOnRobolectric.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/robolectric/RobolectricModule.kt"
exempted_file_path: "testing/src/main/java/org/oppia/android/testing/threading/BackgroundTestDispatcher.kt"
Expand Down Expand Up @@ -887,6 +885,7 @@ exempted_file_path: "utility/src/main/java/org/oppia/android/util/parser/svg/Svg
exempted_file_path: "utility/src/main/java/org/oppia/android/util/parser/svg/SvgDecoder.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/parser/svg/SvgPictureDrawable.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/parser/svg/TextSvgDrawableTranscoder.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterConstants.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/platformparameter/PlatformParameterValue.kt"
exempted_file_path: "utility/src/main/java/org/oppia/android/util/statusbar/StatusBarColor.kt"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@ kt_android_library(
name = "test_constants",
testonly = True,
srcs = [
"TestBooleanPlatformParameter.kt",
"TestIntegerPlatformParameter.kt",
"TestStringPlatformParameter.kt",
"TestPlatformParameterConstants.kt",
],
visibility = [
"//:oppia_testing_visibility",
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package org.oppia.android.testing.platformparameter

import javax.inject.Qualifier

/**
* Qualifier for test string platform parameter. Only used in tests related to platform parameter.
*/
@Qualifier
annotation class TestStringParam

/**
* Name for the test string platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_STRING_PARAM_NAME = "test_string_param_name"

/**
* Default value for the test string platform parameter. Only used in tests related to platform
* parameter.
*/
const val TEST_STRING_PARAM_DEFAULT_VALUE = "test_string_param_default_value"

/**
* Server value for the test string platform parameter. Only used in tests related to platform
* parameter.
*/
const val TEST_STRING_PARAM_SERVER_VALUE = "test_string_param_value"

/**
* Qualifier for test boolean platform parameter. Only used in tests related to platform parameter.
*/
@Qualifier
annotation class TestBooleanParam

/**
* Name for the test boolean platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_BOOLEAN_PARAM_NAME = "test_boolean_param_name"

/**
* Default value for the test boolean platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_BOOLEAN_PARAM_DEFAULT_VALUE = false

/**
* Server value for the test boolean platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_BOOLEAN_PARAM_SERVER_VALUE = true

/**
* Qualifier for test integer platform parameter. Only used in tests related to platform parameter.
*/
@Qualifier
annotation class TestIntegerParam

/**
* Name for the test integer platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_INTEGER_PARAM_NAME = "test_integer_param_name"

/**
* Default value for the test integer platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_INTEGER_PARAM_DEFAULT_VALUE = 0

/**
* Server value for the test integer platform parameter. Only used in tests related to platform parameter.
*/
const val TEST_INTEGER_PARAM_SERVER_VALUE = 1

This file was deleted.

Loading
Loading