diff --git a/app/BUILD.bazel b/app/BUILD.bazel index 9389c372c45..9e7e6fe3218 100644 --- a/app/BUILD.bazel +++ b/app/BUILD.bazel @@ -905,6 +905,7 @@ TEST_DEPS = [ "//utility/src/main/java/org/oppia/android/util/parser/image:image_parsing_module", "//utility/src/main/java/org/oppia/android/util/parser/image:image_transformation", "//utility/src/main/java/org/oppia/android/util/parser/image:test_glide_image_loader", + "//utility/src/main/java/org/oppia/android/util/profile:profile_name_validator", ] # App module tests. Note that all tests are assumed to be tests with resources (even though not all diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index fcc62d45eb4..f2051c2c934 100755 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -270,7 +270,7 @@ With a PIN, nobody else can access a profile besides this assigned user. We failed to store your avatar image. Please try again. This name is already in use by another profile. - Please enter a name for this profile. + Please enter a valid name for this profile. Names can only have letters. Try another name? Your PIN should be 3 digits long. Please make sure that both PINs match. diff --git a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditFragmentTest.kt b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditFragmentTest.kt index 5ddf52446db..3559928233d 100644 --- a/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditFragmentTest.kt +++ b/app/src/sharedTest/java/org/oppia/android/app/settings/profile/ProfileEditFragmentTest.kt @@ -78,6 +78,7 @@ import org.oppia.android.domain.workmanager.WorkManagerConfigurationModule import org.oppia.android.testing.OppiaTestRule import org.oppia.android.testing.TestImageLoaderModule import org.oppia.android.testing.TestLogReportingModule +import org.oppia.android.testing.data.DataProviderTestMonitor import org.oppia.android.testing.junit.InitializeDefaultLocaleRule import org.oppia.android.testing.profile.ProfileTestHelper import org.oppia.android.testing.robolectric.RobolectricModule @@ -123,6 +124,9 @@ class ProfileEditFragmentTest { @Inject lateinit var profileManagementController: ProfileManagementController + @Inject + lateinit var dataProviderTestMonitorFactory: DataProviderTestMonitor.Factory + @Inject lateinit var testCoroutineDispatchers: TestCoroutineDispatchers @@ -213,14 +217,15 @@ class ProfileEditFragmentTest { @Test fun testProfileEdit_configChange_startWithUserHasDownloadAccess_checkSwitchIsChecked() { - profileManagementController.addProfile( + val addProfileProvider = profileManagementController.addProfile( name = "James", pin = "123", avatarImagePath = null, allowDownloadAccess = true, colorRgb = -10710042, isAdmin = false - ).toLiveData() + ) + dataProviderTestMonitorFactory.waitForNextSuccessfulResult(addProfileProvider) launch( ProfileEditFragmentTestActivity.createProfileEditFragmentTestActivity( context = context, diff --git a/domain/src/main/java/org/oppia/android/domain/profile/BUILD.bazel b/domain/src/main/java/org/oppia/android/domain/profile/BUILD.bazel index 8ee3f01b4ab..c3cf010ddd9 100644 --- a/domain/src/main/java/org/oppia/android/domain/profile/BUILD.bazel +++ b/domain/src/main/java/org/oppia/android/domain/profile/BUILD.bazel @@ -24,6 +24,7 @@ kt_android_library( "//utility/src/main/java/org/oppia/android/util/data:data_providers", "//utility/src/main/java/org/oppia/android/util/locale:oppia_locale", "//utility/src/main/java/org/oppia/android/util/profile:directory_management_util", + "//utility/src/main/java/org/oppia/android/util/profile:profile_name_validator", "//utility/src/main/java/org/oppia/android/util/system:oppia_clock", ], ) diff --git a/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt b/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt index e2c15df2253..718c654058d 100644 --- a/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt +++ b/domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt @@ -32,6 +32,7 @@ import org.oppia.android.util.locale.OppiaLocale import org.oppia.android.util.platformparameter.LearnerStudyAnalytics import org.oppia.android.util.platformparameter.PlatformParameterValue import org.oppia.android.util.profile.DirectoryManagementUtil +import org.oppia.android.util.profile.ProfileNameValidator import org.oppia.android.util.system.OppiaClock import java.io.File import java.io.FileOutputStream @@ -78,7 +79,8 @@ class ProfileManagementController @Inject constructor( private val machineLocale: OppiaLocale.MachineLocale, private val loggingIdentifierController: LoggingIdentifierController, private val learnerAnalyticsLogger: LearnerAnalyticsLogger, - @LearnerStudyAnalytics private val learnerStudyAnalytics: PlatformParameterValue + @LearnerStudyAnalytics private val learnerStudyAnalytics: PlatformParameterValue, + private val profileNameValidator: ProfileNameValidator ) { private var currentProfileId: Int = -1 private val profileDataStore = @@ -210,7 +212,7 @@ class ProfileManagementController @Inject constructor( val deferred = profileDataStore.storeDataWithCustomChannelAsync( updateInMemoryCache = true ) { - if (!learnerStudyAnalytics.value && !onlyLetters(name)) { + if (!learnerStudyAnalytics.value && !profileNameValidator.isNameValid(name)) { return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME) } if (!isNameUnique(name, it)) { @@ -329,7 +331,7 @@ class ProfileManagementController @Inject constructor( val deferred = profileDataStore.storeDataWithCustomChannelAsync( updateInMemoryCache = true ) { - if (!learnerStudyAnalytics.value && !onlyLetters(newName)) { + if (!learnerStudyAnalytics.value && !profileNameValidator.isNameValid(newName)) { return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME) } if (!isNameUnique(newName, it)) { @@ -836,10 +838,6 @@ class ProfileManagementController @Inject constructor( return imageFile.absolutePath } - private fun onlyLetters(name: String): Boolean { - return name.matches(Regex("^[ A-Za-z]+\$")) - } - private fun rotateAndCompressBitmap(uri: Uri, bitmap: Bitmap, cropSize: Int): Bitmap { val croppedBitmap = ThumbnailUtils.extractThumbnail(bitmap, cropSize, cropSize) val inputStream = context.contentResolver.openInputStream(uri)!! diff --git a/scripts/assets/file_content_validation_checks.textproto b/scripts/assets/file_content_validation_checks.textproto index 0c585bad36b..50c3589aa19 100644 --- a/scripts/assets/file_content_validation_checks.textproto +++ b/scripts/assets/file_content_validation_checks.textproto @@ -305,13 +305,14 @@ file_content_checks { exempted_file_name: "domain/src/test/java/org/oppia/android/domain/onboarding/AppStartupStateControllerTest.kt" exempted_file_name: "domain/src/test/java/org/oppia/android/domain/oppialogger/analytics/LearnerAnalyticsLoggerTest.kt" exempted_file_name: "scripts/src/javatests/org/oppia/android/scripts/regex/RegexPatternValidationCheckTest.kt" + exempted_file_name: "utility/src/test/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImplTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/ExpressionToComparableOperationConverterTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/MathExpressionExtensionsTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/MathExpressionParserTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/MathTokenizerTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/PolynomialExtensionsTest.kt" exempted_file_name: "utility/src/test/java/org/oppia/android/util/math/RealExtensionsTest.kt" - exempted_file_name: "utility/src/test/java/org/oppia/android/util/logging/performancemetrics/PerformanceMetricsAssessorImplTest.kt" + exempted_file_name: "utility/src/test/java/org/oppia/android/util/profile/ProfileNameValidatorTest.kt" exempted_file_patterns: "testing/src/main/java/org/oppia/android/testing/junit/.+?\\.kt" } file_content_checks { diff --git a/utility/BUILD.bazel b/utility/BUILD.bazel index 522bd9744c0..96fcde1f0a1 100644 --- a/utility/BUILD.bazel +++ b/utility/BUILD.bazel @@ -140,6 +140,7 @@ TEST_DEPS = [ "//utility/src/main/java/org/oppia/android/util/parser/html:tag_handlers", "//utility/src/main/java/org/oppia/android/util/parser/image:glide_image_loader", "//utility/src/main/java/org/oppia/android/util/parser/image:url_image_parser", + "//utility/src/main/java/org/oppia/android/util/profile:profile_name_validator", ] # Qualified file paths for test classes that have been migrated over to their own packages & diff --git a/utility/src/main/java/org/oppia/android/util/profile/BUILD.bazel b/utility/src/main/java/org/oppia/android/util/profile/BUILD.bazel index 5cf007e7ba5..84dc1073a62 100644 --- a/utility/src/main/java/org/oppia/android/util/profile/BUILD.bazel +++ b/utility/src/main/java/org/oppia/android/util/profile/BUILD.bazel @@ -2,6 +2,7 @@ General purposes utilities to manage directories. """ +load("@dagger//:workspace_defs.bzl", "dagger_rules") load("@io_bazel_rules_kotlin//kotlin:kotlin.bzl", "kt_android_library") kt_android_library( @@ -14,3 +15,16 @@ kt_android_library( "//third_party:javax_inject_javax_inject", ], ) + +kt_android_library( + name = "profile_name_validator", + srcs = [ + "ProfileNameValidator.kt", + ], + visibility = ["//:oppia_api_visibility"], + deps = [ + "//third_party:javax_inject_javax_inject", + ], +) + +dagger_rules() diff --git a/utility/src/main/java/org/oppia/android/util/profile/ProfileNameValidator.kt b/utility/src/main/java/org/oppia/android/util/profile/ProfileNameValidator.kt new file mode 100644 index 00000000000..6051e839cae --- /dev/null +++ b/utility/src/main/java/org/oppia/android/util/profile/ProfileNameValidator.kt @@ -0,0 +1,53 @@ +package org.oppia.android.util.profile + +import javax.inject.Inject + +/** Utility to validate that profile names are correctly formatted. */ +class ProfileNameValidator @Inject constructor() { + private val repeatableSymbols = listOf('.', '\'', '-', ' ') + private val noRepeatedAllowedSymbolsRegex by lazy { + Regex("[${repeatableSymbols.joinToString(separator = "") { "\\$it" } }]{2}") + } + + /** + * Validates a profile name to ensure that it isn't confusingly formatted or contains invalid + * characters (e.g. non-letter characters, per the Unicode standard for letter categories, plus + * apostrophes and dashes).. + * + * @param name name of the profile + * @return whether the profile name whether is a valid, acceptable name + */ + fun isNameValid(name: String): Boolean { + return containsOnlyLettersAndAllowedSymbols(name) && containsNoRepeatedUseOfAllowedSymbols(name) + } + + /** Validates if the character in the name is an alphabet or an allowed symbol or not. */ + private fun containsOnlyLettersAndAllowedSymbols(name: String): Boolean { + name.forEach { + if (!(it.isAlphabetic() || it in repeatableSymbols)) { + return false + } + } + return true + } + + private fun containsNoRepeatedUseOfAllowedSymbols(name: String): Boolean { + return !name.contains(noRepeatedAllowedSymbolsRegex) + } + + private fun Char.isAlphabetic(): Boolean { + /* + The following categories are based on Kotlin's Char.isLetter() and Character.isAlphabetic(). + It also adds spacing marks which can be safely ignored since they modify other Unicode + characters (which are then being verified as being letters). Note also that 'LETTER_NUMBER' + is included for roman numerals and other number-like letters since these can sometimes show + up in names. + */ + return when (category) { + CharCategory.UPPERCASE_LETTER, CharCategory.LOWERCASE_LETTER, CharCategory.TITLECASE_LETTER, + CharCategory.MODIFIER_LETTER, CharCategory.OTHER_LETTER, CharCategory.LETTER_NUMBER, + CharCategory.COMBINING_SPACING_MARK, CharCategory.NON_SPACING_MARK -> true + else -> false + } + } +} diff --git a/utility/src/test/java/org/oppia/android/util/profile/BUILD.bazel b/utility/src/test/java/org/oppia/android/util/profile/BUILD.bazel new file mode 100644 index 00000000000..83e10b86f43 --- /dev/null +++ b/utility/src/test/java/org/oppia/android/util/profile/BUILD.bazel @@ -0,0 +1,32 @@ +""" +Tests for profile utilities. +""" + +load("@dagger//:workspace_defs.bzl", "dagger_rules") +load("//:oppia_android_test.bzl", "oppia_android_test") + +oppia_android_test( + name = "ProfileNameValidatorTest", + srcs = ["ProfileNameValidatorTest.kt"], + custom_package = "org.oppia.android.util.profile", + test_class = "org.oppia.android.util.profile.ProfileNameValidatorTest", + test_manifest = "//utility:test_manifest", + deps = [ + ":dagger", + "//testing/src/main/java/org/oppia/android/testing/junit:oppia_parameterized_test_runner", + "//testing/src/main/java/org/oppia/android/testing/junit:parameterized_robolectric_test_runner", + "//testing/src/main/java/org/oppia/android/testing/robolectric:test_module", + "//testing/src/main/java/org/oppia/android/testing/threading:test_module", + "//testing/src/main/java/org/oppia/android/testing/time:test_module", + "//third_party:androidx_test_ext_junit", + "//third_party:com_google_truth_truth", + "//third_party:junit_junit", + "//third_party:org_robolectric_robolectric", + "//third_party:robolectric_android-all", + "//utility/src/main/java/org/oppia/android/util/locale:prod_module", + "//utility/src/main/java/org/oppia/android/util/logging:prod_module", + "//utility/src/main/java/org/oppia/android/util/profile:profile_name_validator", + ], +) + +dagger_rules() diff --git a/utility/src/test/java/org/oppia/android/util/profile/ProfileNameValidatorTest.kt b/utility/src/test/java/org/oppia/android/util/profile/ProfileNameValidatorTest.kt new file mode 100644 index 00000000000..129bf69807e --- /dev/null +++ b/utility/src/test/java/org/oppia/android/util/profile/ProfileNameValidatorTest.kt @@ -0,0 +1,120 @@ +package org.oppia.android.util.profile + +import android.app.Application +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import dagger.BindsInstance +import dagger.Component +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.oppia.android.testing.junit.OppiaParameterizedTestRunner +import org.oppia.android.testing.junit.OppiaParameterizedTestRunner.Iteration +import org.oppia.android.testing.junit.OppiaParameterizedTestRunner.Parameter +import org.oppia.android.testing.junit.OppiaParameterizedTestRunner.RunParameterized +import org.oppia.android.testing.junit.OppiaParameterizedTestRunner.SelectRunnerPlatform +import org.oppia.android.testing.junit.ParameterizedRobolectricTestRunner +import org.oppia.android.testing.robolectric.RobolectricModule +import org.robolectric.annotation.Config +import org.robolectric.annotation.LooperMode +import javax.inject.Inject +import javax.inject.Singleton + +@Suppress("FunctionName") +@RunWith(OppiaParameterizedTestRunner::class) +@SelectRunnerPlatform(ParameterizedRobolectricTestRunner::class) +@LooperMode(LooperMode.Mode.PAUSED) +@Config(manifest = Config.NONE) +class ProfileNameValidatorTest { + @Inject + lateinit var profileNameValidator: ProfileNameValidator + + @Parameter + lateinit var name: String + + @Before + fun setup() { + setUpTestApplicationComponent() + } + + @Test + fun testIsNameValid_nameWithSpaces_returnsTrue() { + val nameWithSpaces = "Ben Henning" + assertThat(profileNameValidator.isNameValid(nameWithSpaces)).isTrue() + } + + @Test + fun testIsNameValid_nameWithNumber_returnsFalse() { + val nameWithNumber = "Jishnu7" + assertThat(profileNameValidator.isNameValid(nameWithNumber)).isFalse() + } + + @Test + @RunParameterized( + Iteration("Ben#Henning", "name=Ben#Henning"), + Iteration("Rajay@T", "name=Rajay@T"), + Iteration("جيشنو^&&", "name=جيشنو^&&"), + Iteration("_Jishnu", "name=_Jishnu") + ) + fun testIsNameValid_nameWithDisallowedSymbol_returnsFalse() { + assertThat(profileNameValidator.isNameValid(name)).isFalse() + } + + @Test + @RunParameterized( + Iteration("Ben-Henning", "name=Ben-Henning"), + Iteration("Rajat.T", "name=Rajat.T"), + Iteration("G'Jishnu", "name=G'Jishnu") + ) + fun testIsNameValid_nameWithAllowedSymbols_returnsTrue() { + assertThat(profileNameValidator.isNameValid(name)).isTrue() + } + + @Test + @RunParameterized( + Iteration("Ben-.Henning", "name=Ben-.Henning"), + Iteration("Rajat..T", "name=Rajat..T"), + Iteration("Name WithTooManySpaces", "name=Name WithTooManySpaces") + ) + fun testIsNameValid_nameWithRepeatedAllowedSymbols_returnsFalse() { + assertThat(profileNameValidator.isNameValid(name)).isFalse() + } + + @Test + fun testIsNameValid_nameWithEnglishLetters_returnsTrue() { + val nameWithEnglishLetters = "Jerry" + assertThat(profileNameValidator.isNameValid(nameWithEnglishLetters)).isTrue() + } + + @Test + fun testIsNameValid_nameWithHindiLetters_returnsTrue() { + val nameWithHindiLetters = "जिष्णु" + assertThat(profileNameValidator.isNameValid(nameWithHindiLetters)).isTrue() + } + + @Test + fun testIsNameValid_nameWithArabicLetters_returnsTrue() { + val nameWithArabicLetters = "جيشنو" + assertThat(profileNameValidator.isNameValid(nameWithArabicLetters)).isTrue() + } + + private fun setUpTestApplicationComponent() { + DaggerProfileNameValidatorTest_TestApplicationComponent + .builder() + .setApplication(ApplicationProvider.getApplicationContext()).build().inject(this) + } + + @Singleton + @Component(modules = [RobolectricModule::class]) + interface TestApplicationComponent { + @Component.Builder + interface Builder { + @BindsInstance + fun setApplication(application: Application): Builder + + fun build(): TestApplicationComponent + } + + fun inject(test: ProfileNameValidatorTest) + } +}