Skip to content

Commit

Permalink
Merge pull request #4 from JishnuGoyal/profiles-cant-have-internation…
Browse files Browse the repository at this point in the history
…alised-names

Profiles cant have internationalised names
  • Loading branch information
BenHenning authored Sep 8, 2022
2 parents 78f4f52 + b90bf80 commit e8f5d6c
Show file tree
Hide file tree
Showing 12 changed files with 241 additions and 12 deletions.
1 change: 1 addition & 0 deletions app/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -932,6 +932,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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import org.oppia.android.databinding.ProfileRenameFragmentBinding
import org.oppia.android.domain.profile.ProfileManagementController
import org.oppia.android.util.data.AsyncResult
import org.oppia.android.util.data.DataProviders.Companion.toLiveData
import org.oppia.android.util.profile.ProfileNameValidator
import javax.inject.Inject

/** The presenter for [ProfileRenameFragment]. */
Expand All @@ -27,7 +28,8 @@ class ProfileRenameFragmentPresenter @Inject constructor(
private val fragment: Fragment,
private val profileManagementController: ProfileManagementController,
private val viewModelProvider: ViewModelProvider<ProfileRenameViewModel>,
private val resourceHandler: AppLanguageResourceHandler
private val resourceHandler: AppLanguageResourceHandler,
private val profileNameValidator: ProfileNameValidator
) {
private val renameViewModel: ProfileRenameViewModel by lazy {
getProfileRenameViewModel()
Expand All @@ -52,7 +54,7 @@ class ProfileRenameFragmentPresenter @Inject constructor(
}
binding.profileRenameSaveButton.setOnClickListener {
renameViewModel.nameErrorMsg.set("")
if (binding.profileRenameInputEditText.text.toString().isEmpty()) {
if (!profileNameValidator.isNameValid(binding.profileRenameInputEditText.text.toString())) {
renameViewModel
.nameErrorMsg
.set(
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@
<string name="add_profile_pin_info">With a PIN, nobody else can access a profile besides this assigned user.</string>
<string name="add_profile_error_image_failed_store">We failed to store your avatar image. Please try again.</string>
<string name="add_profile_error_name_not_unique">This name is already in use by another profile.</string>
<string name="add_profile_error_name_empty">Please enter a name for this profile.</string>
<string name="add_profile_error_name_empty">Please enter a valid name for this profile.</string>
<string name="add_profile_error_name_only_letters">Names can only have letters. Try another name?</string>
<string name="add_profile_error_pin_length">Your PIN should be 3 digits long.</string>
<string name="add_profile_error_pin_confirm_wrong">Please make sure that both PINs match.</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,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
Expand Down Expand Up @@ -120,6 +121,9 @@ class ProfileEditFragmentTest {
@Inject
lateinit var profileManagementController: ProfileManagementController

@Inject
lateinit var dataProviderTestMonitorFactory: DataProviderTestMonitor.Factory

@Inject
lateinit var testCoroutineDispatchers: TestCoroutineDispatchers

Expand Down Expand Up @@ -210,14 +214,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>(
ProfileEditFragmentTestActivity.createProfileEditFragmentTestActivity(
context = context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,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
Expand Down Expand Up @@ -76,7 +77,8 @@ class ProfileManagementController @Inject constructor(
private val machineLocale: OppiaLocale.MachineLocale,
private val loggingIdentifierController: LoggingIdentifierController,
private val learnerAnalyticsLogger: LearnerAnalyticsLogger,
@LearnerStudyAnalytics private val learnerStudyAnalytics: PlatformParameterValue<Boolean>
@LearnerStudyAnalytics private val learnerStudyAnalytics: PlatformParameterValue<Boolean>,
private val profileNameValidator: ProfileNameValidator
) {
private var currentProfileId: Int = -1
private val profileDataStore =
Expand Down Expand Up @@ -205,7 +207,7 @@ class ProfileManagementController @Inject constructor(
val deferred = profileDataStore.storeDataWithCustomChannelAsync(
updateInMemoryCache = true
) {
if (!onlyLetters(name)) {
if (!profileNameValidator.isNameValid(name)) {
return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME)
}
if (!isNameUnique(name, it)) {
Expand Down Expand Up @@ -323,7 +325,7 @@ class ProfileManagementController @Inject constructor(
val deferred = profileDataStore.storeDataWithCustomChannelAsync(
updateInMemoryCache = true
) {
if (!onlyLetters(newName)) {
if (!profileNameValidator.isNameValid(newName)) {
return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME)
}
if (!isNameUnique(newName, it)) {
Expand Down Expand Up @@ -804,10 +806,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)!!
Expand Down
1 change: 1 addition & 0 deletions scripts/assets/file_content_validation_checks.textproto
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ file_content_checks {
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/profile/ProfileNameValidatorTest.kt"
exempted_file_patterns: "testing/src/main/java/org/oppia/android/testing/junit/.+?\\.kt"
}
file_content_checks {
Expand Down
2 changes: 2 additions & 0 deletions utility/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ kt_android_library(
"//utility/src/main/java/org/oppia/android/util/logging:console_logger",
"//utility/src/main/java/org/oppia/android/util/logging:exception_logger",
"//utility/src/main/java/org/oppia/android/util/logging:prod_module",
"//utility/src/main/java/org/oppia/android/util/profile:profile_name_validator",
"//utility/src/main/java/org/oppia/android/util/system:oppia_clock",
"//utility/src/main/java/org/oppia/android/util/system:prod_module",
"//utility/src/main/java/org/oppia/android/util/threading:prod_module",
Expand Down Expand Up @@ -116,6 +117,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 &
Expand Down
14 changes: 14 additions & 0 deletions utility/src/main/java/org/oppia/android/util/profile/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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()
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
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 noRepeatedAllowedSymbolsRegex by lazy { Regex("[.'-]{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() || isAllowedSymbol(it))) {
return false
}
}
return true
}

private fun containsNoRepeatedUseOfAllowedSymbols(name: String): Boolean {
return !name.contains(noRepeatedAllowedSymbolsRegex)
}

private fun isAllowedSymbol(symbol: Char): Boolean {
return symbol == '.' || symbol == '-' || symbol == '\''
}

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
}
}
}
32 changes: 32 additions & 0 deletions utility/src/test/java/org/oppia/android/util/profile/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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()
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
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_returnsFalse() {
val nameWithSpaces = "Ben Henning"
assertThat(profileNameValidator.isNameValid(nameWithSpaces)).isFalse()
}

@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")
)
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)
}
}

0 comments on commit e8f5d6c

Please sign in to comment.