From cd91a6b15a7c6caddfc440fcb7a7d76ba86f304a Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Tue, 16 Jul 2024 21:21:22 +0300 Subject: [PATCH 01/10] Add the raw checks file --- scripts/feature_flags_logger_check.sh | 148 ++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 scripts/feature_flags_logger_check.sh diff --git a/scripts/feature_flags_logger_check.sh b/scripts/feature_flags_logger_check.sh new file mode 100644 index 00000000000..300cd1c10a0 --- /dev/null +++ b/scripts/feature_flags_logger_check.sh @@ -0,0 +1,148 @@ +#!/bin/bash + +echo "********************************" +echo "Performing feature flag checks" +echo "********************************" + +failed_checks=0 + +item_in_array() { + local item="$1" + shift + local array=("$@") + + for element in "${array[@]}"; do + if [[ "$element" == "$item" ]]; then + echo 1 + return + fi + done + + echo 0 +} + +function get_classes_from_constants_file() { + # Get the file path of the FeatureFlagConstants File + file_path=$(find . -name FeatureFlagConstants.kt) + + # Get a list of feature flag annotation classes + annotation_classes=$(grep -F -E '[\s\w]*annotation\s*class' "$file_path") + + # Convert the string into an array, splitting by newline + IFS=$'\n' read -rd '' -a array <<<"$annotation_classes" + + # Create an empty array to hold individual class names + class_names=() + + # Iterate through each line and take the last word of the list + for line in "${array[@]}"; do + # Convert each line into an array of words, splitting by space + IFS=' ' read -ra words <<<"$line" + + # Get last element of the list + last_element="${words[${#words[@]}-1]}" + + # Add the element into class_names + class_names+=("$last_element") + done + + echo "${class_names[@]}" +} + +function get_imported_classes_in_logger() { + # File path containing the block of text + file_path=$(find . -name FeatureFlagsLogger.kt) + + # Use sed to extract the block based on a starting pattern and an ending pattern + extracted_block=$(sed -n '/class FeatureFlagsLogger @Inject constructor(/,/^)/p' "$file_path") + + # Get annotation classes + logged_classes=$(grep -E '@Enable[A-Za-z0-9_]+' "$file_path") + + # Print the logged classes + echo "$logged_classes" +} + +function get_flags_added_into_the_logging_map() { + # File path containing the block of text + file_path=$(find . -name FeatureFlagsLogger.kt) + + # Use sed to extract the block based on a starting pattern and an ending pattern + extracted_block=$(sed -n '/Map> = mapOf(/,/)$/p' "$file_path") + + # Get any word beginning with enable from the extracted block + enable_flags=$(grep -E 'enable[A-Za-z0-9_]+' <<<"$extracted_block") + + added_flags=() + + # Convert the string into an array, splitting by newline + IFS=$'\n' read -rd '' -a added_flags <<<"$enable_flags" + + # Create an empty array to hold individual class names + class_names=() + + # Iterate through each line and take the last word of the list + for line in "${added_flags[@]}"; do + # Remove the comma at the end of the line + line=$(echo "$line" | awk '{sub(/,$/, ""); print}') + + # Convert each line into an array of words, splitting by space + IFS=' ' read -ra words <<<"$line" + + # Get last element of the list + last_element="${words[${#words[@]}-1]}" + + # Capitalize last element using sed + last_element=$(echo "$last_element" | awk '{for(i=1;i<=NF;i++) $i=toupper(substr($i,1,1)) substr($i,2)}1') + + # Add the element into class_names + class_names+=("$last_element") + done + + echo "${class_names[@]}" +} + +function ensure_all_flags_are_imported() { + feature_flags=($(get_classes_from_constants_file)) + imported_classes=($(get_imported_classes_in_logger)) + flags_added_to_map=($(get_flags_added_into_the_logging_map)) + + # Replace the @ symbol and spaces within each element of the list + for i in "${!imported_classes[@]}"; do + imported_classes[$i]=$(echo "${imported_classes[$i]}" | tr -d '@' | tr -d ' ') + done + + # Iterate through each element in the feature_flags array to check if it is imported + # in the FeatureFlagsLogger.kt + for element in "${feature_flags[@]}"; do + # Check if the element is in the imported_classes array + in_array=$(item_in_array "$element" "${imported_classes[@]}") + if [[ $in_array -ne 1 ]]; then + failed_checks=$((failed_checks + 1)) + echo "ERROR: $element is not imported in FeatureFlagsLogger.kt" + fi + done + + # Iterate through each element in the feature_flags array to check if it is added to the logging map + for element in "${feature_flags[@]}"; do + # Check if the element is in the feature_flags array + in_array=$(item_in_array "$element" "${flags_added_to_map[@]}") + if [[ $in_array -ne 1 ]]; then + failed_checks=$((failed_checks + 1)) + echo "ERROR: $element is not added to the logging map in FeatureFlagsLogger.kt" + fi + done + + if [[ $failed_checks -eq 0 ]]; then + echo "All feature flags are imported and added to the logging map in FeatureFlagsLogger.kt" + exit 0 + else + echo "********************************" + echo "$failed_checks Feature flag issues found." + echo "Please fix the above issues." + echo "********************************" + exit 1 + fi +} + +ensure_all_flags_are_imported From 51821b6167c2a9c36a7349515296f1381298bb35 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Wed, 17 Jul 2024 21:06:44 +0300 Subject: [PATCH 02/10] Add the feature flag checks file to CI and fix failing issues --- .github/workflows/static_checks.yml | 4 +++ .../analytics/FeatureFlagsLogger.kt | 9 +++++-- ...logger_check.sh => feature_flags_check.sh} | 27 +++++++++---------- scripts/static_checks.sh | 4 +++ .../platformparameter/FeatureFlagConstants.kt | 7 ----- 5 files changed, 28 insertions(+), 23 deletions(-) rename scripts/{feature_flags_logger_check.sh => feature_flags_check.sh} (82%) diff --git a/.github/workflows/static_checks.yml b/.github/workflows/static_checks.yml index b312a43f067..7aaa1ee11ab 100644 --- a/.github/workflows/static_checks.yml +++ b/.github/workflows/static_checks.yml @@ -84,6 +84,10 @@ jobs: run: | bash /home/runner/work/oppia-android/oppia-android/scripts/ktlint_lint_check.sh $HOME + - name: Feature flag checks + run: | + bash /home/runner/work/oppia-android/oppia-android/scripts/feature_flags_check.sh $HOME + - name: Protobuf lint check run: | bash /home/runner/work/oppia-android/oppia-android/scripts/buf_lint_check.sh $HOME diff --git a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt index 1175075751e..d1fb0d5b732 100644 --- a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt +++ b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt @@ -30,6 +30,8 @@ import org.oppia.android.util.platformparameter.PlatformParameterValue import org.oppia.android.util.platformparameter.SPOTLIGHT_UI import javax.inject.Inject import javax.inject.Singleton +import org.oppia.android.util.platformparameter.ENABLE_MULTIPLE_CLASSROOMS +import org.oppia.android.util.platformparameter.EnableMultipleClassrooms /** * Convenience logger for feature flags. @@ -64,7 +66,9 @@ class FeatureFlagsLogger @Inject constructor( @EnableNpsSurvey private val enableNpsSurvey: PlatformParameterValue, @EnableOnboardingFlowV2 - private val enableOnboardingFlowV2: PlatformParameterValue + private val enableOnboardingFlowV2: PlatformParameterValue, + @EnableMultipleClassrooms + private val enableMultipleClassrooms: PlatformParameterValue, ) { /** * A variable containing a list of all the feature flags in the app. @@ -83,7 +87,8 @@ class FeatureFlagsLogger @Inject constructor( INTERACTION_CONFIG_CHANGE_STATE_RETENTION to enableInteractionConfigChangeStateRetention, APP_AND_OS_DEPRECATION to enableAppAndOsDeprecation, ENABLE_NPS_SURVEY to enableNpsSurvey, - ENABLE_ONBOARDING_FLOW_V2 to enableOnboardingFlowV2 + ENABLE_ONBOARDING_FLOW_V2 to enableOnboardingFlowV2, + ENABLE_MULTIPLE_CLASSROOMS to enableMultipleClassrooms ) /** diff --git a/scripts/feature_flags_logger_check.sh b/scripts/feature_flags_check.sh similarity index 82% rename from scripts/feature_flags_logger_check.sh rename to scripts/feature_flags_check.sh index 300cd1c10a0..b5f79824b2d 100644 --- a/scripts/feature_flags_logger_check.sh +++ b/scripts/feature_flags_check.sh @@ -1,7 +1,7 @@ #!/bin/bash echo "********************************" -echo "Performing feature flag checks" +echo "Running feature flag checks" echo "********************************" failed_checks=0 @@ -59,6 +59,11 @@ function get_imported_classes_in_logger() { # Get annotation classes logged_classes=$(grep -E '@Enable[A-Za-z0-9_]+' "$file_path") + # Replace the @ symbol and spaces within each element of the list + for i in "${!logged_classes[@]}"; do + logged_classes[$i]=$(echo "${logged_classes[$i]}" | tr -d '@' | tr -d ' ') + done + # Print the logged classes echo "$logged_classes" } @@ -107,38 +112,32 @@ function ensure_all_flags_are_imported() { imported_classes=($(get_imported_classes_in_logger)) flags_added_to_map=($(get_flags_added_into_the_logging_map)) - # Replace the @ symbol and spaces within each element of the list - for i in "${!imported_classes[@]}"; do - imported_classes[$i]=$(echo "${imported_classes[$i]}" | tr -d '@' | tr -d ' ') - done + file_path=$(find . -name FeatureFlagsLogger.kt) + imports_line_number=$(grep -n 'class FeatureFlagsLogger @Inject constructor(' "$file_path" | head -n 1 | awk -F: '{print $1}') + flags_map_line_number=$(grep -n 'Map> = mapOf(' "$file_path" | head -n 1 | awk -F: '{print $1}') - # Iterate through each element in the feature_flags array to check if it is imported - # in the FeatureFlagsLogger.kt for element in "${feature_flags[@]}"; do - # Check if the element is in the imported_classes array in_array=$(item_in_array "$element" "${imported_classes[@]}") if [[ $in_array -ne 1 ]]; then failed_checks=$((failed_checks + 1)) - echo "ERROR: $element is not imported in FeatureFlagsLogger.kt" + echo "$element is not imported in the constructor argument in $file_path at line $imports_line_number" fi done - # Iterate through each element in the feature_flags array to check if it is added to the logging map for element in "${feature_flags[@]}"; do - # Check if the element is in the feature_flags array in_array=$(item_in_array "$element" "${flags_added_to_map[@]}") if [[ $in_array -ne 1 ]]; then failed_checks=$((failed_checks + 1)) - echo "ERROR: $element is not added to the logging map in FeatureFlagsLogger.kt" + echo "$element is not added to the logging map in $file_path at line $flags_map_line_number" fi done if [[ $failed_checks -eq 0 ]]; then - echo "All feature flags are imported and added to the logging map in FeatureFlagsLogger.kt" + echo "Feature flag checks completed successfully" exit 0 else echo "********************************" - echo "$failed_checks Feature flag issues found." + echo "Feature flag issues found." echo "Please fix the above issues." echo "********************************" exit 1 diff --git a/scripts/static_checks.sh b/scripts/static_checks.sh index f7ad2a39d8e..0a0d08206cb 100644 --- a/scripts/static_checks.sh +++ b/scripts/static_checks.sh @@ -19,6 +19,10 @@ echo "" bash scripts/ktlint_lint_check.sh echo "" +# Run feature flag checks +bash scripts/feature_flags_check.sh +echo "" + # Run protobuf lint checks bash scripts/buf_lint_check.sh echo "" diff --git a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt index e125d2a1fc6..c30ae97206c 100644 --- a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt +++ b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt @@ -24,13 +24,6 @@ const val DOWNLOADS_SUPPORT = "android_enable_downloads_support" /** Default value for feature flag corresponding to [EnableDownloadsSupport]. */ const val ENABLE_DOWNLOADS_SUPPORT_DEFAULT_VALUE = false -/** Qualifier for the feature flag corresponding to enabling the language selection UI. */ -@Qualifier -annotation class EnableLanguageSelectionUi - -/** Default value for the feature flag corresponding to [EnableLanguageSelectionUi]. */ -const val ENABLE_LANGUAGE_SELECTION_UI_DEFAULT_VALUE = true - /** * Qualifier for the feature flag corresponding to enabling the extra topic tabs: practice and info. */ From 60927845b4caca361674ebe0eca7e60cd19168e2 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Wed, 17 Jul 2024 21:10:00 +0300 Subject: [PATCH 03/10] Fix conflict with develop --- .../android/domain/oppialogger/analytics/FeatureFlagsLogger.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt index d1fb0d5b732..267ca4bf215 100644 --- a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt +++ b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt @@ -88,7 +88,7 @@ class FeatureFlagsLogger @Inject constructor( APP_AND_OS_DEPRECATION to enableAppAndOsDeprecation, ENABLE_NPS_SURVEY to enableNpsSurvey, ENABLE_ONBOARDING_FLOW_V2 to enableOnboardingFlowV2, - ENABLE_MULTIPLE_CLASSROOMS to enableMultipleClassrooms + ENABLE_MULTIPLE_CLASSROOMS to enableMultipleClassrooms, ) /** From 352db8d5e83537e65c2c94568257176953b9b103 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Wed, 17 Jul 2024 22:18:22 +0300 Subject: [PATCH 04/10] Fix failing static check --- .../android/domain/oppialogger/analytics/FeatureFlagsLogger.kt | 2 -- 1 file changed, 2 deletions(-) diff --git a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt index e53f042bd88..f6983f25aee 100644 --- a/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt +++ b/domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/FeatureFlagsLogger.kt @@ -32,8 +32,6 @@ import org.oppia.android.util.platformparameter.PlatformParameterValue import org.oppia.android.util.platformparameter.SPOTLIGHT_UI import javax.inject.Inject import javax.inject.Singleton -import org.oppia.android.util.platformparameter.ENABLE_MULTIPLE_CLASSROOMS -import org.oppia.android.util.platformparameter.EnableMultipleClassrooms /** * Convenience logger for feature flags. From 2a3aa8ac86af89e8071f36bee8871243ee887ad7 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Wed, 17 Jul 2024 22:31:55 +0300 Subject: [PATCH 05/10] Fix failing to-do check --- scripts/static_checks.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/static_checks.sh b/scripts/static_checks.sh index 0a0d08206cb..d6486e769e6 100644 --- a/scripts/static_checks.sh +++ b/scripts/static_checks.sh @@ -111,7 +111,7 @@ echo "********************************" bazel run //scripts:license_texts_check -- $(pwd)/app/src/main/res/values/third_party_dependencies.xml echo "" -# TODO checks. +# TO-DO checks. echo "********************************" echo "Running TODO correctness checks" echo "********************************" From 1dfcce6aec4fc76ef4b807fec7e2aff3904e60a7 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Wed, 17 Jul 2024 22:49:35 +0300 Subject: [PATCH 06/10] Fix failing to-do check --- scripts/assets/todo_open_exemptions.textproto | 2 +- scripts/static_checks.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/assets/todo_open_exemptions.textproto b/scripts/assets/todo_open_exemptions.textproto index 715fdf0bf32..8dd53a51ce9 100644 --- a/scripts/assets/todo_open_exemptions.textproto +++ b/scripts/assets/todo_open_exemptions.textproto @@ -336,7 +336,7 @@ todo_open_exemption { } todo_open_exemption { exempted_file_path: "scripts/static_checks.sh" - line_number: 110 + line_number: 114 } todo_open_exemption { exempted_file_path: "wiki/Coding-style-guide.md" diff --git a/scripts/static_checks.sh b/scripts/static_checks.sh index d6486e769e6..0a0d08206cb 100644 --- a/scripts/static_checks.sh +++ b/scripts/static_checks.sh @@ -111,7 +111,7 @@ echo "********************************" bazel run //scripts:license_texts_check -- $(pwd)/app/src/main/res/values/third_party_dependencies.xml echo "" -# TO-DO checks. +# TODO checks. echo "********************************" echo "Running TODO correctness checks" echo "********************************" From ac5cbb1e6e8e77a9b7bf93c5c277082f56b90220 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Thu, 18 Jul 2024 07:06:59 +0300 Subject: [PATCH 07/10] Break CI to confirm the feature flags check is working as expected --- .../android/util/platformparameter/FeatureFlagConstants.kt | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt index c30ae97206c..e125d2a1fc6 100644 --- a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt +++ b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt @@ -24,6 +24,13 @@ const val DOWNLOADS_SUPPORT = "android_enable_downloads_support" /** Default value for feature flag corresponding to [EnableDownloadsSupport]. */ const val ENABLE_DOWNLOADS_SUPPORT_DEFAULT_VALUE = false +/** Qualifier for the feature flag corresponding to enabling the language selection UI. */ +@Qualifier +annotation class EnableLanguageSelectionUi + +/** Default value for the feature flag corresponding to [EnableLanguageSelectionUi]. */ +const val ENABLE_LANGUAGE_SELECTION_UI_DEFAULT_VALUE = true + /** * Qualifier for the feature flag corresponding to enabling the extra topic tabs: practice and info. */ From bb9b1a7b15968b1fc1f7a621270ee37ce07ebd01 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Thu, 18 Jul 2024 07:13:36 +0300 Subject: [PATCH 08/10] Try to fix grep error in the feature flags check --- scripts/feature_flags_check.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/feature_flags_check.sh b/scripts/feature_flags_check.sh index b5f79824b2d..b5cdfb512ea 100644 --- a/scripts/feature_flags_check.sh +++ b/scripts/feature_flags_check.sh @@ -26,7 +26,7 @@ function get_classes_from_constants_file() { file_path=$(find . -name FeatureFlagConstants.kt) # Get a list of feature flag annotation classes - annotation_classes=$(grep -F -E '[\s\w]*annotation\s*class' "$file_path") + annotation_classes=$(grep -E '[\s\w]*annotation\s*class' "$file_path") # Convert the string into an array, splitting by newline IFS=$'\n' read -rd '' -a array <<<"$annotation_classes" From cd9ee5f12ff86c899ff68e07a7669d4aa0620519 Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Thu, 18 Jul 2024 12:40:05 +0300 Subject: [PATCH 09/10] Fix failing check in the feature flags check --- .../android/util/platformparameter/FeatureFlagConstants.kt | 7 ------- 1 file changed, 7 deletions(-) diff --git a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt index e125d2a1fc6..c30ae97206c 100644 --- a/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt +++ b/utility/src/main/java/org/oppia/android/util/platformparameter/FeatureFlagConstants.kt @@ -24,13 +24,6 @@ const val DOWNLOADS_SUPPORT = "android_enable_downloads_support" /** Default value for feature flag corresponding to [EnableDownloadsSupport]. */ const val ENABLE_DOWNLOADS_SUPPORT_DEFAULT_VALUE = false -/** Qualifier for the feature flag corresponding to enabling the language selection UI. */ -@Qualifier -annotation class EnableLanguageSelectionUi - -/** Default value for the feature flag corresponding to [EnableLanguageSelectionUi]. */ -const val ENABLE_LANGUAGE_SELECTION_UI_DEFAULT_VALUE = true - /** * Qualifier for the feature flag corresponding to enabling the extra topic tabs: practice and info. */ From 0adea61af3b8004cd9428a672d4ff9ade3dd989f Mon Sep 17 00:00:00 2001 From: Kenneth Murerwa Date: Fri, 19 Jul 2024 08:21:10 +0300 Subject: [PATCH 10/10] Convert item_in_array to a named function --- scripts/feature_flags_check.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/feature_flags_check.sh b/scripts/feature_flags_check.sh index b5cdfb512ea..48e61153a70 100644 --- a/scripts/feature_flags_check.sh +++ b/scripts/feature_flags_check.sh @@ -6,7 +6,7 @@ echo "********************************" failed_checks=0 -item_in_array() { +function item_in_array() { local item="$1" shift local array=("$@") @@ -107,7 +107,7 @@ function get_flags_added_into_the_logging_map() { echo "${class_names[@]}" } -function ensure_all_flags_are_imported() { +function perform_checks_on_feature_flags() { feature_flags=($(get_classes_from_constants_file)) imported_classes=($(get_imported_classes_in_logger)) flags_added_to_map=($(get_flags_added_into_the_logging_map)) @@ -144,4 +144,4 @@ function ensure_all_flags_are_imported() { fi } -ensure_all_flags_are_imported +perform_checks_on_feature_flags