Skip to content

Commit

Permalink
Ensure best autofill match is always primary button style (duckduckgo…
Browse files Browse the repository at this point in the history
…#3185)

<!--
Note: This checklist is a reminder of our shared engineering
expectations.
The items in Bold are required
If your PR involves UI changes:
1. Upload screenshots or screencasts that illustrate the changes before
/ after
2. Add them under the UI changes section (feel free to add more columns
if needed)
    3. Make sure these changes are tested in API 23 and API 26
If your PR does not involve UI changes, you can remove the **UI
changes** section
-->

Task/Issue URL:
https://app.asana.com/0/488551667048375/1204391947652312/f

### Description
When offering to autofill saved logins, we show a list of matching
logins. Previously, we'd only apply the primary button style to the
first URL which perfectly matched the current page.

With this PR, we instead apply the primary button style to the first URL
in the list, regardless of whether that is for a perfect or partial
match.


### Steps to test this PR

#### Multiple perfect matches, multiple partial matches
- [x] Visit `Settings->Logins`, and tap the `+` button to manually add
the following entries (add username+password to all):
    - [x] url = `fill.dev`
    - [x] url = `fill.dev`
    - [x] url = `foo.fill.dev`
    - [x] url = `foo.fill.dev`
    - [x] url = `bar.fill.dev`
    - [x] url = `bar.fill.dev`
- [x] Visit https://fill.dev/form/login-simple
- [x] Verify the prompt that shows has the very top button (which is a
perfect match) as a primary button style
- [x] Verify all other buttons are secondary button style

#### No perfect matches, multiple partial matches
- [x] Visit `Settings->Logins` and manually delete both `fill.dev`
perfect matches
- [x] Visit https://fill.dev/form/login-simple (or refresh the page if
still there)
- [x] Verify the prompt that shows has the very top button (which is a
partial match) as a primary button style
- [x] Verify all other buttons are secondary button style

### UI changes
| Before  | After |
| ------ | ----- |

![2023-05-22-before](https://github.com/duckduckgo/Android/assets/1336281/f14e3d72-8786-4057-a387-abd20bc4bc70)|![2023-05-22-after](https://github.com/duckduckgo/Android/assets/1336281/d9f26d90-3258-439e-959f-94209e13882e)
  • Loading branch information
CDRussell authored May 22, 2023
1 parent dc37242 commit fa17f72
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,15 @@ class RealAutofillSelectCredentialsListBuilder @Inject constructor(
processPerfectMatches(sortedGroups, flattenedList)
}

// if we had any perfect matches we'll show the primary button there. So only show it for partial matches where there are no perfect matches.
var addPrimaryButtonToPartialMatches = sortedGroups.perfectMatches.isEmpty()

sortedGroups.partialMatches.forEach { (key, value) ->
processPartialMatches(flattenedList, key, value)
val addedPrimaryButton = processPartialMatches(flattenedList, key, value, addPrimaryButtonToPartialMatches)
if (addedPrimaryButton) {
// only add the primary button once
addPrimaryButtonToPartialMatches = false
}
}

return flattenedList
Expand All @@ -72,15 +79,29 @@ class RealAutofillSelectCredentialsListBuilder @Inject constructor(
}
}

/**
* Process the partial matches for a given domain, adding them to the flattened list
* @param usePrimaryButtonType if true, the primary button will be used for the first item in the list
* @return true if the primary button was used, false otherwise
*/
private fun processPartialMatches(
flattenedList: MutableList<ListItem>,
key: String,
value: List<LoginCredentials>,
) {
usePrimaryButtonType: Boolean,
): Boolean {
flattenedList.add(GroupHeading(context.getString(R.string.useSavedLoginDialogFromDomainLabel, key)))

value.forEach {
flattenedList.add(CredentialSecondaryType(it))
var primaryButtonAdded = false
value.forEachIndexed { index, loginCredentials ->
if (index == 0 && usePrimaryButtonType) {
flattenedList.add(CredentialPrimaryType(loginCredentials))
primaryButtonAdded = true
} else {
flattenedList.add(CredentialSecondaryType(loginCredentials))
}
}

return primaryButtonAdded
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,15 +88,28 @@ class RealAutofillSelectCredentialsListBuilderTest {
}

@Test
fun whenNoPerfectMatchesAndOnePartialMatchThenPartialMatchAddedAsSecondaryButton() {
fun whenNoPerfectMatchesAndOnePartialMatchThenPartialMatchAddedAsPrimaryButton() {
val sortedGroup = Groups(
perfectMatches = emptyList(),
partialMatches = mapOf(
Pair("foo.example.com", listOf(creds())),
),
)
val list = testee.buildFlatList(sortedGroup)
list[1].assertIsSecondaryButton()
list[1].assertIsPrimaryButton()
}

@Test
fun whenNoPerfectMatchesAndTwoPartialMatchesInSameDomainThenFirstPartialMatchAddedAsPrimaryButton() {
val sortedGroup = Groups(
perfectMatches = emptyList(),
partialMatches = mapOf(
Pair("foo.example.com", listOf(creds(), creds())),
),
)
val list = testee.buildFlatList(sortedGroup)
list[1].assertIsPrimaryButton()
list[2].assertIsSecondaryButton()
}

@Test
Expand Down Expand Up @@ -124,6 +137,22 @@ class RealAutofillSelectCredentialsListBuilderTest {
list[3].assertIsSecondaryButton()
}

@Test
fun whenNoPerfectMatchesAndMultiplePartialMatchesAcrossSitesThenFirstPartialMatchAddedAsPrimaryButton() {
val sortedGroup = Groups(
perfectMatches = emptyList(),
partialMatches = mapOf(
Pair("foo.example.com", listOf(creds(), creds())),
Pair("bar.example.com", listOf(creds(), creds())),
),
)
val list = testee.buildFlatList(sortedGroup)
list[1].assertIsPrimaryButton()
list[2].assertIsSecondaryButton()
list[4].assertIsSecondaryButton()
list[5].assertIsSecondaryButton()
}

@Test
fun whenPerfectMatchesAndMultiplePartialMatchesThenListOutputAsExpected() {
val sortedGroup = Groups(
Expand All @@ -145,15 +174,21 @@ class RealAutofillSelectCredentialsListBuilderTest {
list[6].assertIsSecondaryButton()
}

private fun ListItem.assertIsVerticalSpacing() = assertTrue(this is VerticalSpacing)
private fun ListItem.assertIsPrimaryButton() = assertTrue(this is ListItem.CredentialPrimaryType)
private fun ListItem.assertIsSecondaryButton() = assertTrue(this is ListItem.CredentialSecondaryType)
private fun ListItem.assertIsVerticalSpacing() = assertTrue("Not vertical spacing; is ${this.javaClass.simpleName}", this is VerticalSpacing)
private fun ListItem.assertIsPrimaryButton() = assertTrue(
"Not primary button; is ${this.javaClass.simpleName}",
this is ListItem.CredentialPrimaryType,
)
private fun ListItem.assertIsSecondaryButton() = assertTrue(
"Not secondary button; is ${this.javaClass.simpleName}",
this is ListItem.CredentialSecondaryType,
)
private fun ListItem.assertIsDomainGroupHeading(name: String) {
assertTrue(this is GroupHeading)
assertTrue("Not a group heading; is ${this.javaClass.simpleName}", this is GroupHeading)
assertEquals(String.format("From %s", name), (this as GroupHeading).label)
}
private fun ListItem.assertIsFromThisWebsiteGroupHeading() {
assertTrue(this is GroupHeading)
assertTrue("Not a group heading; is ${this.javaClass.simpleName}", this is GroupHeading)
assertEquals("From This Website", (this as GroupHeading).label)
}

Expand Down

0 comments on commit fa17f72

Please sign in to comment.