[PM-31614] feat: Added new UI for the Email verification on sends#6488
[PM-31614] feat: Added new UI for the Email verification on sends#6488
Conversation
github-actions
bot
added
app:password-manager
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6488 +/- ##
==========================================
+ Coverage 86.31% 86.34% +0.02%
==========================================
Files 790 790
Lines 56525 56620 +95
Branches 8175 8210 +35
==========================================
+ Hits 48791 48888 +97
+ Misses 4887 4874 -13
- Partials 2847 2858 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @aj-rosado's task in 4m 49s —— View job Code Review: PM-31614 feat: Added new UI for the Email verification on sends
Reviewed commit: dae2231 SummaryThis review covers the latest push which includes formatting fixes and filtering of empty email entries. The PR adds a new UI for email verification on sends, behind the Key changes reviewed:
FindingsNo new issues found in this latest push. The previous review findings have been addressed:
Outstanding Review ThreadsMost existing threads are marked as outdated (the code they pointed at has been updated). The following items from previous reviews may still warrant attention:
Reviewed with ❤️ by Claude |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| var selectedOption: SendAuthType by rememberSaveable { | ||
| mutableStateOf( | ||
| value = when { | ||
| hasPassword -> SendAuthType.PASSWORD | ||
| emails.isNotEmpty() -> SendAuthType.EMAIL | ||
| else -> SendAuthType.NONE | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎨 SUGGESTED: The selectedOption is managed locally with rememberSaveable and computed from hasPassword and emails props. This works correctly in most scenarios, but creates a potential state desync if the ViewModel's authType changes from an external action (e.g., if "Remove password" functionality were to reset the auth type).
Consider passing the current authType from the ViewModel as a parameter to ensure single source of truth:
fun AddEditSendAuthTypeChooser(
selectedAuthType: SendAuthType, // Add this parameter
onAuthTypeSelect: (SendAuthType) -> Unit,
// ... other params
)This is a minor suggestion - the current implementation works correctly for the current use cases since the auth type changes only occur through user interaction with this component.
There was a problem hiding this comment.
I agree with Claude here, Maybe we can just pass in the SendAuthType
|
Feature label is missing. |
|
⛏️ Typo in the PR description
|
| authType = SendAuthType.PASSWORD, | ||
| ), | ||
| selectedType = DEFAULT_TEXT_TYPE, | ||
| ), |
There was a problem hiding this comment.
🎨 SUGGESTED: Missing test coverage for authType branches
The toViewState extension function has conditional logic for determining authType based on hasPassword and emails.isNotEmpty() (lines 39-43 in SendViewExtensions.kt):
authType = when {
hasPassword -> SendAuthType.PASSWORD
emails.isNotEmpty() -> SendAuthType.EMAIL
else -> SendAuthType.NONE
}The current tests only cover the PASSWORD case (via createMockSendView which has hasPassword = true). Consider adding tests for:
- EMAIL case: A SendView with
hasPassword = falseand non-emptyemailslist - NONE case: A SendView with
hasPassword = falseand emptyemailslist
This would ensure all branches are covered and the authType mapping logic is correctly tested.
| onAddNewEmailClick: () -> Unit, | ||
| onRemoveEmailClick: (Int) -> Unit, | ||
| password: String, | ||
| emails: List<String>, |
There was a problem hiding this comment.
Can we make this an immutable list
|
|
||
| @Composable | ||
| private fun specificPeopleEmailContent( | ||
| emails: List<String>, |
There was a problem hiding this comment.
Immutable list here too
| .passwordInput | ||
| .takeIf { | ||
| common.authType == SendAuthType.PASSWORD | ||
| }.orNullIfBlank(), |
There was a problem hiding this comment.
Can you use chain format here:
.passwordInput
.takeIf { common.authType == SendAuthType.PASSWORD }
.orNullIfBlank(),| } | ||
|
|
||
| @Composable | ||
| private fun specificPeopleEmailContent( |
There was a problem hiding this comment.
This should start with a capital S
Can we also add the ColumnScope. extension. I know the IDE will say it's not needed but this composable does not make sense outside of a ColumnScope and we should enforce that it is always applied there.
| label = stringResource(id = BitwardenString.add_email), | ||
| onClick = { | ||
| onAddNewEmailClick() | ||
| }, |
There was a problem hiding this comment.
Can we simplify this:
onClick = onAddNewEmailClick,
|
One of your screenshots show that part of the card has the incorrect style |
| val expirationDate: ZonedDateTime?, | ||
| val sendUrl: String?, | ||
| val hasPassword: Boolean, | ||
| val authEmails: List<String>, |
There was a problem hiding this comment.
Can we make this an ImmutableList
| * The user selected an authentication type. | ||
| */ | ||
| data class AuthTypeSelect(val authType: SendAuthType) : | ||
| AddEditSendAction() |
There was a problem hiding this comment.
Can we format this a bit nicer:
data class AuthTypeSelect(val authType: SendAuthType) : AddEditSendAction()
aj-rosado
changed the title
| */ | ||
| enum class SendAuthType( | ||
| val text: Text, | ||
| val supportingTextRes: Int?, |
There was a problem hiding this comment.
Can we make this a Text as well?
| // Initialize with one empty email field if list is empty | ||
| commonContent.copy( | ||
| authEmails = commonContent.authEmails.ifEmpty { listOf("") } | ||
| .toImmutableList(), |
There was a problem hiding this comment.
Can this be simplified:
authEmails = commonContent.authEmails.ifEmpty { persistentListOf("") },| onPasswordChange: (String) -> Unit, | ||
| onEmailValueChange: (String, Int) -> Unit, | ||
| onAddNewEmailClick: () -> Unit, | ||
| onRemoveEmailClick: (Int) -> Unit, |
There was a problem hiding this comment.
Instead of the index, can we [ass back the actual email to remove?
| val onPasswordCopyClick: (String) -> Unit, | ||
| val onAuthTypeSelect: (SendAuthType) -> Unit, | ||
| val onAuthPasswordChange: (String) -> Unit, | ||
| val onEmailValueChange: (String, Int) -> Unit, |
There was a problem hiding this comment.
Using the index is unsafe and can lead to errors.
Can we wrap the emails in a data class and create ids to track them. That will make this all much safer
There was a problem hiding this comment.
I have made a refactor on this. Added a new class like you suggested but also decided to remove the SendAuthType and aggregate all the data related to the Send Authentication on a class.
Will add a tech-debt task to also include the passwordInput there.
| @Parcelize | ||
| data object None : SendAuth() { | ||
| @IgnoredOnParcel | ||
| override val text: Text = BitwardenString.anyone_with_the_link.asText() |
There was a problem hiding this comment.
You don't need these annotations if you make them getters
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| val email1 = AuthEmail(id = "id1", value = "test@example.com") | ||
| val initialCommonState = DEFAULT_COMMON_STATE.copy( | ||
| passwordInput = "oldpassword", | ||
| sendAuth = SendAuth.Password, |
Check warning
Code scanning / Checkmarx One
Privacy Violation Medium test
| onPasswordChange: (String) -> Unit, | ||
| onEmailValueChange: (String, String) -> Unit, | ||
| onAddNewEmailClick: () -> Unit, | ||
| onRemoveEmailClick: (String) -> Unit, |
There was a problem hiding this comment.
Can we updates these to just emit the AuthEmail
onEmailValueChange: (AuthEmail) -> Unit,
onRemoveEmailClick: (AuthEmail) -> Unit,| contentDescription = stringResource(id = BitwardenString.delete), | ||
| contentColor = BitwardenTheme.colorScheme.status.error, | ||
| onClick = { | ||
| onRemoveEmailClick(authEmail.id) |
There was a problem hiding this comment.
Just emit the whole AuthEmail object
| hasPassword -> SendAuth.Password | ||
| emails.isNotEmpty() -> SendAuth.Email( | ||
| emails = this.emails.map { AuthEmail(value = it) }.toImmutableList(), | ||
| ) |
There was a problem hiding this comment.
Any multi-line case like this should be wrapped in curly braces
| if (authEmail.id == action.id) { | ||
| authEmail.copy(value = action.email) | ||
| if (authEmail.id == action.authEmail.id) { | ||
| authEmail.copy(value = action.authEmail.value) |
There was a problem hiding this comment.
Why copy it?
Can we just replace it?
val updatedEmails = currentAuth.emails.map { authEmail ->
if (authEmail.id == action.authEmail.id) {
action.authEmail
} else {
authEmail
}
| }, | ||
| emails = emptyList(), | ||
| authType = AuthType.NONE, | ||
| emails = (common.sendAuth as? SendAuth.Email)?.emails?.map { it.value }.orEmpty(), |
There was a problem hiding this comment.
emails list sent to the server.
Since handleAuthEmailsRemove always keeps at least one empty AuthEmail when the list would become empty (ViewModel line 624), saving with "Specific people" selected but no emails entered sends [""] to the server. This is also inconsistent with how empty passwords are handled -- newPassword uses .orNullIfBlank() to filter out empty values (line 28), but emails get no such filtering.
Consider filtering out blank entries before sending:
| emails = (common.sendAuth as? SendAuth.Email)?.emails?.map { it.value }.orEmpty(), | |
| emails = (common.sendAuth as? SendAuth.Email) | |
| ?.emails | |
| ?.map { it.value } | |
| ?.filter { it.isNotBlank() } | |
| .orEmpty(), |
| sendAuth = currentAuth.copy( | ||
| emails = currentAuth.emails.plus( | ||
| AuthEmail(value = ""), | ||
| ).toImmutableList(), |
There was a problem hiding this comment.
Formatting:
emails = currentAuth
.emails
.plus(AuthEmail(value = ""))
.toImmutableList(),| ?.emails | ||
| ?.map { it.value } | ||
| ?.filter { it.isNotBlank() } | ||
| .orEmpty(), |
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31614
📔 Objective
Initial PR to add the layout changes for the Email Verification option on Sends.
The changes are behind a feature flag and should not have any impact when it is turned off.
This does not add the changes to the server and misses validations that will be added on subsequent PRs.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes