[PM-31614] feat: Added new UI for the Email verification on sends #6488
Conversation
github-actions
bot
added
app:password-manager
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6488 +/- ##
==========================================
+ Coverage 86.32% 86.33% +0.01%
==========================================
Files 783 784 +1
Lines 56439 56598 +159
Branches 8162 8184 +22
==========================================
+ Hits 48719 48865 +146
- Misses 4880 4887 +7
- Partials 2840 2846 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @aj-rosado's task in 3m 2s —— View job Code Review SummaryPR: PM-31614 feat: Added new UI for the Email verification on sends Reviewed Commits: Status: ApprovedAll previous review comments have been addressed. The PR is ready for merge. Summary of ChangesThis PR adds a new authentication type chooser UI for Sends, allowing users to select between:
Key additions:
Addressed Items (Latest Commits)
Code Quality Assessment
Non-blocking NoteThe Reviewed with Claude Code |
![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
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