-
Notifications
You must be signed in to change notification settings - Fork 932
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
Autofill: Implement mapping for appPackages for autofill provider service #5508
base: feature/cristian/autofill/autofill_provider_poc
Are you sure you want to change the base?
Autofill: Implement mapping for appPackages for autofill provider service #5508
Conversation
Seems like the lint failures and unit test failures were on your changes @cmonfortep so I wouldn’t touch them. |
6c71bcf
to
07a9784
Compare
...fill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppCredentialProvider.kt
Show resolved
Hide resolved
...autofill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppToDomainMapper.kt
Outdated
Show resolved
Hide resolved
...ll-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/PackageManagerExtension.kt
Outdated
Show resolved
Hide resolved
e70f429
to
4cf2a64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @karlenDimla for the work here.
Left some comments. We need to rebase so the diff gets updated
.../autofill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AssetLinksLoader.kt
Show resolved
Hide resolved
...ill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppFingerprintProvider.kt
Show resolved
Hide resolved
...fill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppCredentialProvider.kt
Show resolved
Hide resolved
...main/java/com/duckduckgo/autofill/impl/service/mapper/RemoteDomainTargetAppDataDownloader.kt
Show resolved
Hide resolved
...pl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/RemoteDomainTargetAppService.kt
Outdated
Show resolved
Hide resolved
07a9784
to
cfa85e8
Compare
Addressed all comments (pending final endpoint). if you don’t mind manually running this https://app.asana.com/0/1201462763415876/1209196674121696/f would be helpful. Thanks! @cmonfortep @CDRussell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review final pass, all good from my side 👍
Planning to test this today afternoon, and report back
...fill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppCredentialProvider.kt
Outdated
Show resolved
Hide resolved
...autofill-impl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/AppToDomainMapper.kt
Outdated
Show resolved
Hide resolved
...pl/src/main/java/com/duckduckgo/autofill/impl/service/mapper/RemoteDomainTargetAppService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tested and works really neat!
Merge-note: I assume CI is failing due to parent branch, where I'm currently working on. I can take care of rebasing this next time until CI is green.
@cmonfortep Awesome! Feel free to merge then when ready. Thanks |
4cf2a64
to
adc22eb
Compare
b10e715
to
7cb8a70
Compare
1814778
to
3a16022
Compare
7cb8a70
to
650b8d6
Compare
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
3a16022
to
8647864
Compare
5824f1f
to
5d63aa5
Compare
cb79246
to
12bf984
Compare
5d63aa5
to
810b9c2
Compare
Task/Issue URL: https://app.asana.com/0/1201462763415876/1209125943914364/f
Description
See attached task description
Steps to test this PR
https://app.asana.com/0/1201462763415876/1209196674121696/f