Skip to content

Commit 22376bf

Browse files
david-livefrontdavid-livefront
authored
PM-19403: Add better error messages for username generation (#4911)
1 parent ab270cd commit 22376bf

File tree

7 files changed

+289
-128
lines changed

7 files changed

+289
-128
lines changed

‎app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreen.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,7 +1220,7 @@ private fun ForwardedEmailAliasTypeContent(
12201220
when (usernameTypeState.selectedServiceType) {
12211221
is ServiceType.AddyIo -> {
12221222
BitwardenPasswordField(
1223-
label = stringResource(id = R.string.api_access_token),
1223+
label = stringResource(id = R.string.api_access_token_required_parenthesis),
12241224
value = usernameTypeState.selectedServiceType.apiAccessToken,
12251225
onValueChange = forwardedEmailAliasHandlers.onAddyIoAccessTokenTextChange,
12261226
showPasswordTestTag = "ShowForwardedEmailApiSecretButton",
@@ -1290,7 +1290,7 @@ private fun ForwardedEmailAliasTypeContent(
12901290

12911291
is ServiceType.FirefoxRelay -> {
12921292
BitwardenPasswordField(
1293-
label = stringResource(id = R.string.api_access_token),
1293+
label = stringResource(id = R.string.api_access_token_required_parenthesis),
12941294
value = usernameTypeState.selectedServiceType.apiAccessToken,
12951295
onValueChange = forwardedEmailAliasHandlers.onFirefoxRelayAccessTokenTextChange,
12961296
showPasswordTestTag = "ShowForwardedEmailApiSecretButton",

‎app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorViewModel.kt‎

Lines changed: 67 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ import com.x8bit.bitwarden.ui.tools.feature.generator.GeneratorState.MainType.Us
4646
import com.x8bit.bitwarden.ui.tools.feature.generator.GeneratorState.MainType.Username.UsernameType.PlusAddressedEmail
4747
import com.x8bit.bitwarden.ui.tools.feature.generator.GeneratorState.MainType.Username.UsernameType.RandomWord
4848
import com.x8bit.bitwarden.ui.tools.feature.generator.model.GeneratorMode
49+
import com.x8bit.bitwarden.ui.tools.feature.generator.util.GeneratorRequestResult
4950
import com.x8bit.bitwarden.ui.tools.feature.generator.util.toServiceType
5051
import com.x8bit.bitwarden.ui.tools.feature.generator.util.toStrictestPolicy
5152
import com.x8bit.bitwarden.ui.tools.feature.generator.util.toUsernameGeneratorRequest
@@ -170,7 +171,7 @@ class GeneratorViewModel @Inject constructor(
170171
is GeneratorAction.PasswordHistoryClick -> handlePasswordHistoryClick()
171172
is GeneratorAction.CloseClick -> handleCloseClick()
172173
is GeneratorAction.SaveClick -> handleSaveClick()
173-
is GeneratorAction.RegenerateClick -> handleRegenerationClick()
174+
is GeneratorAction.RegenerateClick -> handleRegenerateClick()
174175
is GeneratorAction.CopyClick -> handleCopyClick()
175176
is GeneratorAction.MainTypeOptionSelect -> handleMainTypeOptionSelect(action)
176177
is GeneratorAction.MainType -> handleMainTypeAction(action)
@@ -711,10 +712,13 @@ class GeneratorViewModel @Inject constructor(
711712

712713
//region Generated Field Handlers
713714

714-
private fun handleRegenerationClick() {
715+
private fun handleRegenerateClick() {
715716
// Go through the update process with the current state to trigger a
716717
// regeneration of the generated text for the same state.
717-
updateGeneratorMainType(forceRegeneration = true) { state.selectedType }
718+
updateGeneratorMainType(
719+
allowErrorsWhenMissingValues = true,
720+
forceRegeneration = true,
721+
) { state.selectedType }
718722
}
719723

720724
private fun handleCopyClick() {
@@ -1511,7 +1515,9 @@ class GeneratorViewModel @Inject constructor(
15111515

15121516
//region Utility Functions
15131517

1518+
@Suppress("LongMethod")
15141519
private inline fun updateGeneratorMainType(
1520+
allowErrorsWhenMissingValues: Boolean = false,
15151521
forceRegeneration: Boolean = false,
15161522
crossinline block: (GeneratorState.MainType) -> GeneratorState.MainType?,
15171523
) {
@@ -1523,21 +1529,26 @@ class GeneratorViewModel @Inject constructor(
15231529
generateTextJob = viewModelScope.launch {
15241530
when (updatedMainType) {
15251531
is GeneratorState.MainType.Passphrase -> {
1526-
savePassphraseOptionsToDisk(updatedMainType)
1527-
generatePassphrase(updatedMainType)
1532+
savePassphraseOptionsToDisk(passphrase = updatedMainType)
1533+
generatePassphrase(passphrase = updatedMainType)
15281534
}
15291535

15301536
is GeneratorState.MainType.Password -> {
1531-
savePasswordOptionsToDisk(updatedMainType)
1532-
generatePassword(updatedMainType)
1537+
savePasswordOptionsToDisk(password = updatedMainType)
1538+
generatePassword(password = updatedMainType)
15331539
}
15341540

15351541
is GeneratorState.MainType.Username -> {
15361542
when (val selectedType = updatedMainType.selectedType) {
15371543
is ForwardedEmailAlias -> {
1538-
saveForwardedEmailAliasServiceTypeToDisk(selectedType)
1544+
saveForwardedEmailAliasServiceTypeToDisk(
1545+
forwardedEmailAlias = selectedType,
1546+
)
15391547
if (forceRegeneration) {
1540-
generateForwardedEmailAlias(selectedType)
1548+
generateForwardedEmailAlias(
1549+
allowErrorsWhenMissingValues = allowErrorsWhenMissingValues,
1550+
alias = selectedType,
1551+
)
15411552
} else {
15421553
mutableStateFlow.update {
15431554
it.copy(generatedText = NO_GENERATED_TEXT)
@@ -1546,9 +1557,12 @@ class GeneratorViewModel @Inject constructor(
15461557
}
15471558

15481559
is CatchAllEmail -> {
1549-
saveCatchAllEmailOptionsToDisk(selectedType)
1560+
saveCatchAllEmailOptionsToDisk(catchAllEmail = selectedType)
15501561
if (forceRegeneration) {
1551-
generateCatchAllEmail(selectedType)
1562+
generateCatchAllEmail(
1563+
catchAllEmail = selectedType,
1564+
allowErrorsWhenMissingValues = allowErrorsWhenMissingValues,
1565+
)
15521566
} else {
15531567
mutableStateFlow.update {
15541568
it.copy(generatedText = NO_GENERATED_TEXT)
@@ -1557,9 +1571,9 @@ class GeneratorViewModel @Inject constructor(
15571571
}
15581572

15591573
is PlusAddressedEmail -> {
1560-
savePlusAddressedEmailOptionsToDisk(selectedType)
1574+
savePlusAddressedEmailOptionsToDisk(plusAddressedEmail = selectedType)
15611575
if (forceRegeneration) {
1562-
generatePlusAddressedEmail(selectedType)
1576+
generatePlusAddressedEmail(plusAddressedEmail = selectedType)
15631577
} else {
15641578
mutableStateFlow.update {
15651579
it.copy(generatedText = NO_GENERATED_TEXT)
@@ -1568,16 +1582,19 @@ class GeneratorViewModel @Inject constructor(
15681582
}
15691583

15701584
is RandomWord -> {
1571-
saveRandomWordOptionsToDisk(selectedType)
1572-
generateRandomWordUsername(selectedType)
1585+
saveRandomWordOptionsToDisk(randomWord = selectedType)
1586+
generateRandomWordUsername(randomWord = selectedType)
15731587
}
15741588
}
15751589
}
15761590
}
15771591
}
15781592
}
15791593

1580-
private suspend fun generateForwardedEmailAlias(alias: ForwardedEmailAlias) {
1594+
private suspend fun generateForwardedEmailAlias(
1595+
alias: ForwardedEmailAlias,
1596+
allowErrorsWhenMissingValues: Boolean,
1597+
) {
15811598
val request = alias
15821599
.selectedServiceType
15831600
?.toUsernameGeneratorRequest(
@@ -1589,8 +1606,27 @@ class GeneratorViewModel @Inject constructor(
15891606
mutableStateFlow.update { it.copy(generatedText = NO_GENERATED_TEXT) }
15901607
return
15911608
}
1592-
val result = generatorRepository.generateForwardedServiceUsername(request)
1593-
sendAction(GeneratorAction.Internal.UpdateGeneratedForwardedServiceUsernameResult(result))
1609+
when (request) {
1610+
is GeneratorRequestResult.MissingField -> {
1611+
mutableStateFlow.update { it.copy(generatedText = NO_GENERATED_TEXT) }
1612+
if (allowErrorsWhenMissingValues) {
1613+
sendEvent(
1614+
event = GeneratorEvent.ShowSnackbar(
1615+
message = R.string.validation_field_required.asText(request.fieldName),
1616+
),
1617+
)
1618+
}
1619+
}
1620+
1621+
is GeneratorRequestResult.Success -> {
1622+
val result = generatorRepository.generateForwardedServiceUsername(request.result)
1623+
sendAction(
1624+
action = GeneratorAction.Internal.UpdateGeneratedForwardedServiceUsernameResult(
1625+
result = result,
1626+
),
1627+
)
1628+
}
1629+
}
15941630
}
15951631

15961632
private suspend fun generatePlusAddressedEmail(plusAddressedEmail: PlusAddressedEmail) {
@@ -1603,9 +1639,21 @@ class GeneratorViewModel @Inject constructor(
16031639
sendAction(GeneratorAction.Internal.UpdateGeneratedPlusAddressedUsernameResult(result))
16041640
}
16051641

1606-
private suspend fun generateCatchAllEmail(catchAllEmail: CatchAllEmail) {
1642+
private suspend fun generateCatchAllEmail(
1643+
catchAllEmail: CatchAllEmail,
1644+
allowErrorsWhenMissingValues: Boolean,
1645+
) {
16071646
val domainName = catchAllEmail.domainName.orNullIfBlank() ?: run {
16081647
mutableStateFlow.update { it.copy(generatedText = NO_GENERATED_TEXT) }
1648+
if (allowErrorsWhenMissingValues) {
1649+
sendEvent(
1650+
event = GeneratorEvent.ShowSnackbar(
1651+
message = R.string.validation_field_required.asText(
1652+
R.string.domain_name.asText(),
1653+
),
1654+
),
1655+
)
1656+
}
16091657
return
16101658
}
16111659
val result = generatorRepository.generateCatchAllEmail(

‎app/src/main/java/com/x8bit/bitwarden/ui/tools/feature/generator/util/ServiceTypeExtensions.kt‎

Lines changed: 73 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@ package com.x8bit.bitwarden.ui.tools.feature.generator.util
22

33
import com.bitwarden.generators.ForwarderServiceType
44
import com.bitwarden.generators.UsernameGeneratorRequest
5+
import com.x8bit.bitwarden.R
6+
import com.x8bit.bitwarden.ui.platform.base.util.Text
7+
import com.x8bit.bitwarden.ui.platform.base.util.asText
58
import com.x8bit.bitwarden.ui.platform.base.util.orNullIfBlank
69
import com.x8bit.bitwarden.ui.platform.base.util.prefixHttpsIfNecessary
710
import com.x8bit.bitwarden.ui.tools.feature.generator.GeneratorState.MainType.Username.UsernameType.ForwardedEmailAlias.ServiceType
@@ -14,23 +17,27 @@ fun ServiceType.toUsernameGeneratorRequest(
1417
website: String?,
1518
allowAddyIoSelfHostUrl: Boolean,
1619
allowSimpleLoginSelfHostUrl: Boolean,
17-
): UsernameGeneratorRequest.Forwarded? {
20+
): GeneratorRequestResult {
1821
return when (this) {
1922
is ServiceType.AddyIo -> {
20-
val accessToken = this.apiAccessToken.orNullIfBlank() ?: return null
21-
val domain = this.domainName.orNullIfBlank() ?: return null
23+
val accessToken = this.apiAccessToken.orNullIfBlank()
24+
?: return GeneratorRequestResult.MissingField(R.string.api_access_token.asText())
25+
val domain = this.domainName.orNullIfBlank()
26+
?: return GeneratorRequestResult.MissingField(R.string.domain_name.asText())
2227
val baseUrl = if (allowAddyIoSelfHostUrl && selfHostServerUrl.isNotBlank()) {
2328
selfHostServerUrl.prefixHttpsIfNecessary()
2429
} else {
2530
ServiceType.AddyIo.DEFAULT_ADDY_IO_URL
2631
}
27-
UsernameGeneratorRequest.Forwarded(
28-
service = ForwarderServiceType.AddyIo(
29-
apiToken = accessToken,
30-
domain = domain,
31-
baseUrl = baseUrl,
32+
GeneratorRequestResult.Success(
33+
UsernameGeneratorRequest.Forwarded(
34+
service = ForwarderServiceType.AddyIo(
35+
apiToken = accessToken,
36+
domain = domain,
37+
baseUrl = baseUrl,
38+
),
39+
website = website,
3240
),
33-
website = website,
3441
)
3542
}
3643

@@ -39,44 +46,57 @@ fun ServiceType.toUsernameGeneratorRequest(
3946
.apiKey
4047
.orNullIfBlank()
4148
?.let {
42-
UsernameGeneratorRequest.Forwarded(
43-
service = ForwarderServiceType.DuckDuckGo(token = it),
44-
website = website,
49+
GeneratorRequestResult.Success(
50+
UsernameGeneratorRequest.Forwarded(
51+
service = ForwarderServiceType.DuckDuckGo(token = it),
52+
website = website,
53+
),
4554
)
4655
}
56+
?: GeneratorRequestResult.MissingField(R.string.api_key.asText())
4757
}
4858

4959
is ServiceType.FirefoxRelay -> {
5060
this
5161
.apiAccessToken
5262
.orNullIfBlank()
5363
?.let {
54-
UsernameGeneratorRequest.Forwarded(
55-
service = ForwarderServiceType.Firefox(apiToken = it),
56-
website = website,
64+
GeneratorRequestResult.Success(
65+
UsernameGeneratorRequest.Forwarded(
66+
service = ForwarderServiceType.Firefox(apiToken = it),
67+
website = website,
68+
),
5769
)
5870
}
71+
?: GeneratorRequestResult.MissingField(R.string.api_access_token.asText())
5972
}
6073

6174
is ServiceType.FastMail -> {
6275
this
6376
.apiKey
6477
.orNullIfBlank()
6578
?.let {
66-
UsernameGeneratorRequest.Forwarded(
67-
service = ForwarderServiceType.Fastmail(apiToken = it),
68-
// A null `website` value here will cause an error.
69-
website = website.orEmpty(),
79+
GeneratorRequestResult.Success(
80+
UsernameGeneratorRequest.Forwarded(
81+
service = ForwarderServiceType.Fastmail(apiToken = it),
82+
// A null `website` value here will cause an error.
83+
website = website.orEmpty(),
84+
),
7085
)
7186
}
87+
?: GeneratorRequestResult.MissingField(R.string.api_key.asText())
7288
}
7389

7490
is ServiceType.ForwardEmail -> {
75-
val apiKey = this.apiKey.orNullIfBlank() ?: return null
76-
val domainName = this.domainName.orNullIfBlank() ?: return null
77-
UsernameGeneratorRequest.Forwarded(
78-
service = ForwarderServiceType.ForwardEmail(apiKey, domainName),
79-
website = website,
91+
val apiKey = this.apiKey.orNullIfBlank()
92+
?: return GeneratorRequestResult.MissingField(R.string.api_key.asText())
93+
val domainName = this.domainName.orNullIfBlank()
94+
?: return GeneratorRequestResult.MissingField(R.string.domain_name.asText())
95+
GeneratorRequestResult.Success(
96+
UsernameGeneratorRequest.Forwarded(
97+
service = ForwarderServiceType.ForwardEmail(apiKey, domainName),
98+
website = website,
99+
),
80100
)
81101
}
82102

@@ -90,11 +110,37 @@ fun ServiceType.toUsernameGeneratorRequest(
90110
.apiKey
91111
.orNullIfBlank()
92112
?.let {
93-
UsernameGeneratorRequest.Forwarded(
94-
service = ForwarderServiceType.SimpleLogin(apiKey = it, baseUrl = baseUrl),
95-
website = website,
113+
GeneratorRequestResult.Success(
114+
UsernameGeneratorRequest.Forwarded(
115+
service = ForwarderServiceType.SimpleLogin(
116+
apiKey = it,
117+
baseUrl = baseUrl,
118+
),
119+
website = website,
120+
),
96121
)
97122
}
123+
?: GeneratorRequestResult.MissingField(R.string.api_key.asText())
98124
}
99125
}
100126
}
127+
128+
/**
129+
* Wrapper that contains a [UsernameGeneratorRequest.Forwarded] on success of indicates what data
130+
* is still required to create a request.
131+
*/
132+
sealed class GeneratorRequestResult {
133+
/**
134+
* A request has been successfully created.
135+
*/
136+
data class Success(
137+
val result: UsernameGeneratorRequest.Forwarded,
138+
) : GeneratorRequestResult()
139+
140+
/**
141+
* The request failed to be generated do to a missing value.
142+
*/
143+
data class MissingField(
144+
val fieldName: Text,
145+
) : GeneratorRequestResult()
146+
}

‎app/src/main/res/values/strings.xml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -773,7 +773,9 @@ select Add TOTP to store the key safely</string>
773773
<string name="forwarded_email_alias">Forwarded email alias</string>
774774
<string name="random_word">Random word</string>
775775
<string name="email_required_parenthesis">Email (required)</string>
776+
<string name="domain_name">domain name</string>
776777
<string name="domain_name_required_parenthesis">Domain name (required)</string>
778+
<string name="api_key">API key</string>
777779
<string name="api_key_required_parenthesis">API key (required)</string>
778780
<string name="service">Service</string>
779781
<string name="addy_io">addy.io</string>
@@ -782,6 +784,7 @@ select Add TOTP to store the key safely</string>
782784
<string name="duck_duck_go">DuckDuckGo</string>
783785
<string name="fastmail">Fastmail</string>
784786
<string name="forward_email">ForwardEmail</string>
787+
<string name="api_access_token_required_parenthesis">API access token (required)</string>
785788
<string name="api_access_token">API access token</string>
786789
<string name="are_you_sure_you_want_to_overwrite_the_current_username">Are you sure you want to overwrite the current username?</string>
787790
<string name="generate_username">Generate username</string>

‎app/src/test/java/com/x8bit/bitwarden/ui/tools/feature/generator/GeneratorScreenTest.kt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1064,7 +1064,7 @@ class GeneratorScreenTest : BaseComposeTest() {
10641064
val newAccessToken = "accessToken"
10651065

10661066
composeTestRule
1067-
.onNodeWithText("API access token")
1067+
.onNodeWithText("API access token (required)")
10681068
.performScrollTo()
10691069
.performTextInput(newAccessToken)
10701070

@@ -1306,7 +1306,7 @@ class GeneratorScreenTest : BaseComposeTest() {
13061306
val newAccessToken = "accessToken"
13071307

13081308
composeTestRule
1309-
.onNodeWithText("API access token")
1309+
.onNodeWithText("API access token (required)")
13101310
.performScrollTo()
13111311
.performTextInput(newAccessToken)
13121312

0 commit comments

Comments
 (0)