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

Fix #4002: Profiles cant have internationalised names #4081

Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
305b621
modify regex pattern to accept international characters, hyphens, dot…
JishnuGoyal Jan 6, 2022
8992c97
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Jan 6, 2022
e51dc53
add test
JishnuGoyal Jan 15, 2022
f94bd79
add regex functions
JishnuGoyal Jan 16, 2022
5aa3cc9
add regex functions
JishnuGoyal Jan 16, 2022
ebc2057
add tests for different profile names
JishnuGoyal Jan 16, 2022
c48c11b
modify regex pattern to allow .'- characters
JishnuGoyal Jan 16, 2022
b07c3ed
lint
JishnuGoyal Jan 16, 2022
717aa8b
create name validator class
JishnuGoyal Jan 19, 2022
48dc947
move name validation functions to NameValidator class
JishnuGoyal Jan 19, 2022
22d1fd2
add nameValidatorTest
JishnuGoyal Jan 19, 2022
5ec615f
lint
JishnuGoyal Jan 19, 2022
a7958b2
add more descriptive variable names
JishnuGoyal Jan 19, 2022
2f13f70
move NameValidator.kt to profile package
JishnuGoyal Jan 24, 2022
b3298dc
add new android_library for name validator
JishnuGoyal Jan 24, 2022
3e38e30
replace assertThat with Truth assertion
JishnuGoyal Jan 24, 2022
7bf7b72
modify name validation logic, make class injectable, refactor variabl…
JishnuGoyal Jan 24, 2022
9fbdaeb
lint
JishnuGoyal Jan 24, 2022
98f145d
add kdoc
JishnuGoyal Jan 24, 2022
71e099a
refactor variable and function names
JishnuGoyal Jan 30, 2022
b7a714a
refactor variable and function names
JishnuGoyal Jan 30, 2022
760e194
inject nameValidator
JishnuGoyal Jan 30, 2022
b0813c5
lint
JishnuGoyal Jan 30, 2022
36ed90f
edit BUILD.bazel for nameValidator
JishnuGoyal Jan 30, 2022
528d14c
add NameValidatorUtil.kt
JishnuGoyal Feb 3, 2022
eaa8f9d
move and refactor variables
JishnuGoyal Feb 4, 2022
ca403f6
modify BUILD.bazel visibility for nameValidator
JishnuGoyal Feb 4, 2022
018e920
lint
JishnuGoyal Feb 4, 2022
a3e5c77
add tests specific to behaviour
JishnuGoyal Feb 9, 2022
ff87114
rename
JishnuGoyal Feb 17, 2022
0371c00
use isAlphabetic instead of regex
JishnuGoyal Feb 26, 2022
2fc8db2
make profile name validator injectable and inject it
JishnuGoyal Feb 26, 2022
0c2afe5
modify test cases for using isAlphabetic instead of regex
JishnuGoyal Feb 26, 2022
5a4319b
lint, refactor file names, correct variable name
JishnuGoyal Feb 26, 2022
8691d1b
add JavaDoc
JishnuGoyal Feb 26, 2022
4e2ca03
bazel config for profile name validator
JishnuGoyal Feb 27, 2022
0cd03fc
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Mar 2, 2022
413c56f
add dep [bazel] for profile_name_validator
JishnuGoyal Mar 2, 2022
56e12a9
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Mar 2, 2022
f446528
fix build.bazel
JishnuGoyal Mar 2, 2022
f2688a8
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Apr 3, 2022
63340a3
lint
JishnuGoyal Apr 3, 2022
cf4e720
use suggested implementation
JishnuGoyal Apr 7, 2022
d093816
lint
JishnuGoyal Apr 14, 2022
c0f2ae0
remove ProfileNameValidator and profile name validator module
JishnuGoyal Apr 14, 2022
abaf905
update bazel config
JishnuGoyal Apr 14, 2022
1f0a3e8
rename ProfileNameValidatorImpl to ProfileNameValidator.kt
JishnuGoyal Apr 14, 2022
eefe4b7
update bazel config for these changes
JishnuGoyal Apr 14, 2022
287f436
add module annotation
JishnuGoyal Apr 14, 2022
48eed64
use as a singleton
JishnuGoyal Apr 15, 2022
94bb630
correct the improper merge with develop
JishnuGoyal Apr 15, 2022
75b72da
lint
JishnuGoyal Apr 15, 2022
2b1e044
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Apr 15, 2022
e5fc606
remove tests from ProfileManagementControllerTest.kt that were duplic…
JishnuGoyal Apr 15, 2022
e6d2b53
remove duplicate line
JishnuGoyal Apr 25, 2022
8307978
remove Singleton annotation (nit)
JishnuGoyal Apr 25, 2022
1f87748
nits
JishnuGoyal Apr 25, 2022
70177aa
add test for "Jerry"
JishnuGoyal Apr 25, 2022
1b9f613
make test roboelectric
JishnuGoyal Apr 25, 2022
a5b2fe4
convert to parameterised tests
JishnuGoyal Apr 26, 2022
052febc
add EOF newline
JishnuGoyal Apr 26, 2022
0063dd1
add exemption for parameterised test
JishnuGoyal Apr 28, 2022
bd406c9
add imports
JishnuGoyal Apr 28, 2022
de6ff95
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal May 8, 2022
f5990e1
remove trailing comma
JishnuGoyal May 8, 2022
67c4dd4
add BUILD.bazel [problem]
JishnuGoyal May 12, 2022
311839c
fix bazel issues
JishnuGoyal Jun 30, 2022
488d8f6
fix bazel lint
JishnuGoyal Jun 30, 2022
ddde20c
Merge branch 'develop' into profiles-cant-have-internationalised-names
JishnuGoyal Jul 12, 2022
4b3122d
BUILD.bazel working session
JishnuGoyal Jul 13, 2022
0f22585
working session
JishnuGoyal Jul 13, 2022
26da326
add profilenamevalidator to ProfileRenameFragmentPresenter.kt
JishnuGoyal Jul 13, 2022
b90bf80
lint
JishnuGoyal Jul 13, 2022
e8f5d6c
Merge pull request #4 from JishnuGoyal/profiles-cant-have-internation…
BenHenning Sep 8, 2022
bf0282b
Fix broken tests.
BenHenning Sep 8, 2022
e4f744d
Merge branch 'develop' into fix-tests-for-internationalized-profile-n…
BenHenning Sep 8, 2022
c6e08f4
Merge pull request #1 from BenHenning/fix-tests-for-internationalized…
JishnuGoyal Sep 8, 2022
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,7 @@ import org.oppia.android.util.data.DataProviders.Companion.transform
import org.oppia.android.util.data.DataProviders.Companion.transformAsync
import org.oppia.android.util.locale.OppiaLocale
import org.oppia.android.util.profile.DirectoryManagementUtil
import org.oppia.android.util.profile.NameValidator
import org.oppia.android.util.system.OppiaClock
import java.io.File
import java.io.FileOutputStream
Expand Down Expand Up @@ -68,7 +69,8 @@ class ProfileManagementController @Inject constructor(
private val directoryManagementUtil: DirectoryManagementUtil,
private val exceptionsController: ExceptionsController,
private val oppiaClock: OppiaClock,
private val machineLocale: OppiaLocale.MachineLocale
private val machineLocale: OppiaLocale.MachineLocale,
private val nameValidator: NameValidator
) {
private var currentProfileId: Int = -1
private val profileDataStore =
Expand Down Expand Up @@ -197,7 +199,7 @@ class ProfileManagementController @Inject constructor(
val deferred = profileDataStore.storeDataWithCustomChannelAsync(
updateInMemoryCache = true
) {
if (!onlyLetters(name)) {
if (!nameValidator.isNameValid(name)) {
return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME)
}
if (!isNameUnique(name, it)) {
Expand Down Expand Up @@ -309,7 +311,7 @@ class ProfileManagementController @Inject constructor(
val deferred = profileDataStore.storeDataWithCustomChannelAsync(
updateInMemoryCache = true
) {
if (!onlyLetters(newName)) {
if (!nameValidator.isNameValid(newName)) {
return@storeDataWithCustomChannelAsync Pair(it, ProfileActionStatus.INVALID_PROFILE_NAME)
}
if (!isNameUnique(newName, it)) {
Expand Down Expand Up @@ -721,10 +723,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
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ class ProfileManagementControllerTest {
Profile.newBuilder().setName("Veena").setPin("567").setAllowDownloadAccess(true).build()
)

private val allowedNames = listOf<String>("जिष्णु", "Ben-Henning", "Rajat.T", "جيشنو")

private val disallowedNames = listOf<String>("जिष्णु7", "Ben_Henning", "Rajat..T", "جيشنو^&&")

@Before
fun setUp() {
setUpTestApplicationComponent()
Expand Down Expand Up @@ -167,23 +171,43 @@ class ProfileManagementControllerTest {
}

@Test
fun testAddProfile_addProfileWithNumberInName_checkResultIsFailure() {
fun testAddProfile_addProfilesWithDisallowedNames_checkResultIsFailure() {
addTestProfiles()
testCoroutineDispatchers.runCurrent()

profileManagementController.addProfile(
name = "James034",
pin = "321",
avatarImagePath = null,
allowDownloadAccess = false,
colorRgb = -10710042,
isAdmin = true
).toLiveData().observeForever(mockUpdateResultObserver)
disallowedNames.forEach {
profileManagementController.addProfile(
name = it,
pin = "321",
avatarImagePath = null,
allowDownloadAccess = false,
colorRgb = -10710042,
isAdmin = true
).toLiveData().observeForever(mockUpdateResultObserver)
testCoroutineDispatchers.runCurrent()

verifyUpdateFailed()
assertThat(updateResultCaptor.value.getErrorOrNull()).hasMessageThat()
.contains("$it does not contain only letters")
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}

@Test
fun testAddProfiles_addProfilesWithAllowedNames_checkAllProfilesAreAdded() {
addAllowedNameProfiles()
testCoroutineDispatchers.runCurrent()

verifyUpdateFailed()
assertThat(updateResultCaptor.value.getErrorOrNull()).hasMessageThat()
.contains("James034 does not contain only letters")
val profileDatabase = readProfileDatabase()
verifyUpdateSucceeded()

val profiles = profileDatabase.profilesMap

profiles.forEach { index, profile ->
assertThat(profile.name).isEqualTo(allowedNames[index])
assertThat(File(getAbsoluteDirPath("$index")).isDirectory).isTrue()
}

assertThat(profiles.size).isEqualTo(allowedNames.size)
}

@Test
Expand Down Expand Up @@ -1003,6 +1027,19 @@ class ProfileManagementControllerTest {
}
}

private fun addAllowedNameProfiles() {
allowedNames.forEach {
profileManagementController.addProfile(
name = it,
pin = "314",
avatarImagePath = null,
allowDownloadAccess = false,
colorRgb = -10710042,
isAdmin = false
).toLiveData().observeForever(mockUpdateResultObserver)
}
}

private fun checkTestProfilesArePresent(resultList: List<Profile>) {
PROFILES_LIST.forEachIndexed { idx, profile ->
assertThat(resultList[idx].name).isEqualTo(profile.name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,11 @@ kt_android_library(
"//third_party:javax_inject_javax_inject",
],
)

kt_android_library(
name = "name_validator",
srcs = [
"NameValidator.kt",
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
],
visibility = ["//domain:__subpackages__","//utility:__subpackages__"],
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package org.oppia.android.util.profile

import javax.inject.Inject
import javax.inject.Singleton

/** Utility to validate profile names */
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
@Singleton
class NameValidator @Inject constructor() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
private val letterAndSymbolsRegex by lazy {
Regex("^.[${NameValidatorUtil.lettersAndSymbolsRegexString}]+\$")
}

private val noRepeatedAllowedSymbolsRegex by lazy { Regex("[.'-]{2}") }

/**
* Validates names for a profile
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
*
* @param name name of the profile
* @return if the profile name is allowed or not
*/
fun isNameValid(name: String): Boolean {
return (onlyLettersAndAllowedSymbols(name) && noRepeatedUseOfAllowedSymbols(name))
}

private fun onlyLettersAndAllowedSymbols(name: String): Boolean {
return name.matches(letterAndSymbolsRegex)
}

private fun noRepeatedUseOfAllowedSymbols(name: String): Boolean {
return !name.contains(noRepeatedAllowedSymbolsRegex)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.oppia.android.util.profile

/** Utility class for [NameValidator]. */
object NameValidatorUtil {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
/**
* Some languages may not be detected properly by [allLanguageLettersRegex].
* In that case, the range of the Unicode character set will have to be added separately
* for that language to be processed by Regex.
*
* To add a language,
* create a variable containing Unicode range and add to the end of [lettersAndSymbolsRegexString]
*/

private const val allLanguageLettersRegex = "\\p{L}"
private const val allowedSymbolsRegex = ".'\\-"

private const val hindiRegex = "\\u0900-\\u097F"

const val lettersAndSymbolsRegexString = "$allLanguageLettersRegex$hindiRegex$allowedSymbolsRegex"
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package org.oppia.android.util.profile

import com.google.common.truth.Truth.assertThat
import org.junit.Before
import org.junit.Test
import javax.inject.Inject

class NameValidatorTest {

@Inject
lateinit var nameValidator: NameValidator

private val allowedNames = listOf<String>("जिष्णु", "Ben-Henning", "Rajat.T", "جيشنو")

private val disallowedNames =
listOf<String>("जिष्णु7", "Ben_Henning", "Rajat..T", "جيشنو^&&", " ", ".", "Ben Henning")

@Before
fun setup() {
nameValidator = NameValidator()
}

@Test
fun testNameValidator_addDisallowedName_returnFalse() {
disallowedNames.forEach {
assertThat(nameValidator.isNameValid(it)).isEqualTo(false)
}
}

@Test
fun testNameValidator_addAllowedName_returnTrue() {
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
allowedNames.forEach {
assertThat(nameValidator.isNameValid(it)).isEqualTo(true)
BenHenning marked this conversation as resolved.
Show resolved Hide resolved
}
}
}