For #10173: login duplicates and save #11208

eliserichards · 2020-06-03T20:59:00Z

For #10173
Related to mozilla/application-services#3330 - fix in mozilla/application-services#3331
Requires mozilla-mobile/android-components#7694

Requirements for this PR

Save button enabled/disabled based on errors
Pull list of duplicates and don't allow dupes to be saved
UX cleanup for login detail and edit screens
Move AC components.core.passwordsStorage calls into their own store: LoginsDataStore
- to be tested

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

After merge

Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

app/src/main/java/org/mozilla/fenix/settings/logins/EditLoginFragment.kt

ekager · 2020-06-11T02:50:11Z

Can you comment the filed out data review form here so we can get that review rolling too? see example: #11446 (comment)

eliserichards · 2020-06-11T17:49:13Z

Request for data collection review form

All questions are mandatory. You must receive review from a data steward peer on your responses to these questions before shipping new data collection.

What questions will you answer with this data?

What % of our users use CRUD actions (create, read, update, delete) on their saved logins?
How frequently do users manipulate their saved logins?

Why does Mozilla need to answer these questions? Are there benefits for users? Do we need this information to address product or business requirements?

It is crucial we know what types of user segments we have so we can prioritize future logins features. This data will help us better slice our user population & figure out retention metrics. It will also help us to understand the relationship between the Lockwise userbase (standalone password manager) and the Fenix browser users who use saved logins.

What alternative methods did you consider to answer these questions? Why were they not sufficient?

There are currently no alternatives.

Can current instrumentation answer these questions?

No, there are no currently metrics around these actions.

List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox data c ollection categories on the found on the Mozilla wiki.

All data is Category 2.

How long will this data be collected?

Until 09/01/2020

What populations will you measure?

All release, beta, and nightly users with telemetry enabled.

Please provide a general description of how you will analyze this data.

Glean / Amplitude

Where do you intend to share the results of your analysis?

Only on Glean, Amplitude and with mobile teams.

app/metrics.yaml

ekager

Okay, all the pieces look there but we need to refactor some of the architecture. Let's not create a new paradigm here if possible! The interactor -> controller model that is used elsewhere should really replace the LoginsDataStore. Look at bookmarks, history, or the other logins fragments for examples of how this should look. You should start with a View for the EditLoginFragment following the pattern of the other fragments, then we can update that view in consumeFrom. The view can call the interactor, and the interactor calls the controller.

Let's pull out the string to land separately so we can definitely make string freeze by tomorrow.

app/src/main/res/color/save_enabled_ic_color.xml

liuche

Data-review+ only

A few nits and renaming, inline

Data Review Form (to be filled by Data Stewards)

Is there or will there be documentation that describes the schema for the ultimate data set in a public, complete, and accurate way?

Yes, documented the 3 view/edit/delete password probes added

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes, controlled by Fenix data controls

If the request is for permanent data collection, is there someone who will monitor the data over time?

Probes have expiry

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Type 2, interaction with settings

Is the data collection request for default-on or default-off?

default on

Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No, user interaction with UI

Is the data collection covered by the existing Firefox privacy notice?

Yes

Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

No

Does the data collection use a third-party collection tool?

no

app/metrics.yaml

eliserichards · 2020-06-12T15:10:37Z

Pulled out the string into #11516 and tagged you for approval @ekager 🔥

app/src/main/java/org/mozilla/fenix/settings/logins/LoginsDataStore.kt

app/src/main/java/org/mozilla/fenix/settings/logins/EditLoginFragment.kt

Addressed issues :)

eliserichards · 2020-07-09T19:54:12Z

Waiting on mozilla/application-services#3331 to land

ekager

Looking much better! Wondering if we can do some of the cleanup I suggested and if you could also cherry-pick @Mugurell 's commits from #12507 into this PR? (Since they conflict with some of your changes)
cc @Mugurell if you also wanted to take a look at these logins changes and leave some feedback since you've been touching it more recently

app/metrics.yaml

ekager · 2020-07-10T23:12:17Z

app/src/main/java/org/mozilla/fenix/library/history/HistoryFragment.kt

-            getString(
-                R.string.history_delete_single_item_snackbar,
-                historyItems.first().url.toShortUrl(requireComponents.publicSuffixList)
+            String.format(


Why did this change?

lint/detekt error

app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt

ekager · 2020-07-10T23:14:06Z

app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt

+    private val navController: NavController,
+    private val loginsFragmentStore: LoginsFragmentStore
+) {
+    private val activity: HomeActivity = context as HomeActivity


nit - if we're only using the context to grab the activity can we just pass in the activity?

Also looking at the rest of the class it's unclear to me why we need to cast this context to an activity? Can we just use the passed in context?

About the Context..
I'm normally against passing it around and then expect all receivers to know how to build/extract their end dependencies from it. (Demeter's law).
To keep the code clean, have a better separation of responsibilities, loose coupling and ease testing I would propose to have end dependencies sent as parameters to the controller.
Here I see that anyway the context, cast to a specific activity!? is only used for obtaining a reference to components.core.passwordsStorage. That can easily be injected in the constructor without even worrying about it become bloated.

This is a flow taken from a number of other places in the app. I would not like to change it without cleaning up every other place as well. Maybe we can file an eng health ticket.

fenix/app/src/main/java/org/mozilla/fenix/settings/advanced/DefaultLocaleSettingsController.kt

Line 19 in f424656

private val context: Context,

fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkController.kt

Line 52 in f424656

private val activity: HomeActivity,

fenix/app/src/main/java/org/mozilla/fenix/components/toolbar/BrowserToolbarController.kt

Line 63 in f424656

private val activity: HomeActivity,

https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/tabtray/TabTrayController.kt#L34

It is a common pattern in the rest of the app (that we can file a followup ticket about), but in this case we really are only using the context to get the passwordsStorage, so instead of passing in the context, we can just pass in the passwordsStorage directly.

Changed this to use context instead of HomeActivity, and filed a followup: #12565

I think we can still improve this further by changing it pass passwordsStorage directly since AFAICT that's all we use it for

is it cool to pass a component around like that?

Ya! See https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/collections/CollectionCreationController.kt#L76

app/src/main/java/org/mozilla/fenix/settings/logins/interactor/SavedLoginsInteractor.kt

app/src/main/java/org/mozilla/fenix/settings/logins/view/EditLoginView.kt

ekager · 2020-07-13T15:23:40Z

app/src/main/java/org/mozilla/fenix/settings/logins/view/EditLoginView.kt

+        containerView.passwordText?.inputType =
+            InputType.TYPE_CLASS_TEXT or InputType.TYPE_TEXT_VARIATION_PASSWORD
+        containerView.revealPasswordButton?.setImageDrawable(
+            ResourcesCompat.getDrawable(context.resources,


elsewhere we use AppCompatResources.getDrawable

This was a lint or detekt error telling me to use ResourcesCompat. I can use AppCompatResources instead :)

app/src/main/java/org/mozilla/fenix/settings/logins/view/EditLoginView.kt

app/src/main/java/org/mozilla/fenix/settings/logins/view/LoginDetailView.kt

Mugurell

Left some drop-by comments mainly about the architecture.
If we are to refactor this I think we should also focus on improving the architecture and code quality.

Mugurell · 2020-07-14T04:59:16Z

app/src/main/java/org/mozilla/fenix/settings/logins/LoginsFragmentStore.kt

@@ -54,11 +54,11 @@ sealed class LoginsAction : Action {
    data class UpdateLoginsList(val list: List<SavedLogin>) : LoginsAction()
    data class UpdateCurrentLogin(val item: SavedLogin) : LoginsAction()
    data class SortLogins(val sortingStrategy: SortingStrategy) : LoginsAction()
+    data class ListOfDupes(val dupeList: List<SavedLogin>) : LoginsAction()


https://github.com/mozilla-mobile/fenix/blob/master/docs/architecture-overview.md#action
"An Action describes something that happened, and carries any data relevant to that change"
Maybe this Action can be changed to something like "DuplicatesListChanged"?

I prefer this name. If possible duplicates exist, this list is non-empty. The action does not depend on the list changing; it will be changing with every edit.

Mugurell · 2020-07-14T05:07:50Z

app/src/main/java/org/mozilla/fenix/settings/logins/controller/SavedLoginsStorageController.kt

+    private val navController: NavController,
+    private val loginsFragmentStore: LoginsFragmentStore
+) {
+    private val activity: HomeActivity = context as HomeActivity


About the Context..
I'm normally against passing it around and then expect all receivers to know how to build/extract their end dependencies from it. (Demeter's law).
To keep the code clean, have a better separation of responsibilities, loose coupling and ease testing I would propose to have end dependencies sent as parameters to the controller.
Here I see that anyway the context, cast to a specific activity!? is only used for obtaining a reference to components.core.passwordsStorage. That can easily be injected in the constructor without even worrying about it become bloated.

Mugurell · 2020-07-14T05:16:44Z

app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt

+
+        interactor = LoginDetailInteractor(
+            SavedLoginsStorageController(
+                context = requireContext(),


We should look into passing end dependencies to clients and so remove the responsability of them knowing how to and creating such dependencies.
In this case the context seems to only be used for getting components.core.passwordsStorage. We can pass that instead of the Context.

Wondering if this might warrant a larger cleanup in the codebase...

app/src/main/java/org/mozilla/fenix/settings/logins/fragment/LoginDetailFragment.kt

app/src/main/java/org/mozilla/fenix/settings/logins/view/LoginDetailView.kt

app/src/main/java/org/mozilla/fenix/settings/logins/interactor/SavedLoginsInteractor.kt

app/src/main/java/org/mozilla/fenix/settings/logins/view/EditLoginView.kt

app/src/main/java/org/mozilla/fenix/settings/logins/view/LoginDetailView.kt

Mugurell · 2020-07-14T07:31:31Z

One other important improvement to be made:
Since there are 38 files here it is pretty hard to properly review this, there is a high risk un bugs, and afterwards it will be pretty hard to follow why something changed.
I see 21 commits, there will possibly be more followed by a merge of them all at the end, with.. 38 files.
Would be great to have the code split into smaller commits, each addressing a specific need, each having a clear commit message detailing why the change. And then for every fixup needed, like something being raised in review do a rebase-fixup.
I for one very much trust in this guidelines - https://github.com/google/eng-practices/blob/master/review/developer/index.md

Addressed comments. Will rebase with Mugurell's PR once it's merged. Once the app services version (62.0.0) is updated in AC and we get that version into Fenix, I would like this to land and then do followups. I don't want to keep this open for any longer than it already has been.

app/metrics.yaml

app/src/main/java/org/mozilla/fenix/HomeActivity.kt

ekager · 2020-07-16T19:36:10Z

test-debug failing on WHEN finding login dupes, THEN update duplicates in the store

…er based on username. Create edit login controller. Add text watchers and check for duplicates. Edit controller test

Move duplicates logic into the controller

…n into the datastore.

Save enabled in datastore. Move login data to datastore Rebase with password error states Update metrics to be more specific for edit

…parate out some layout manipulation. Inflate view in edit fragment. Double layout showing up. Edit view Controller tests Controller tests passing Interactor tests Lint and detekt cleanup

ekager

r+ with ktlint fix up :)

…assword storage. Addressed comments Lint : Rebase - 1

eliserichards marked this pull request as ready for review June 9, 2020 15:15

eliserichards force-pushed the 10173-login-duplicates branch from 16dcf0b to 2e84b3c Compare June 9, 2020 18:22

ekager reviewed Jun 9, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/logins/EditLoginFragment.kt Outdated Show resolved Hide resolved

eliserichards mentioned this pull request Jun 10, 2020

For 10172: Set edit text listeners #11196

Merged

4 tasks

eliserichards force-pushed the 10173-login-duplicates branch 2 times, most recently from 843a584 to 60cca68 Compare June 10, 2020 22:25

This comment has been minimized.

Sign in to view

eliserichards added the needs:data-review PR is awaiting a data review label Jun 11, 2020

eliserichards requested a review from liuche June 11, 2020 17:49

ekager reviewed Jun 11, 2020

View reviewed changes

app/metrics.yaml Outdated Show resolved Hide resolved

ekager previously requested changes Jun 11, 2020

View reviewed changes

app/src/main/res/color/save_enabled_ic_color.xml Show resolved Hide resolved

liuche approved these changes Jun 12, 2020

View reviewed changes

app/metrics.yaml Outdated Show resolved Hide resolved

app/metrics.yaml Outdated Show resolved Hide resolved

liuche removed the needs:data-review PR is awaiting a data review label Jun 16, 2020

eliserichards force-pushed the 10173-login-duplicates branch from 60cca68 to 9834eb1 Compare June 17, 2020 18:19

eliserichards commented Jun 17, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/logins/LoginsDataStore.kt Outdated Show resolved Hide resolved

eliserichards commented Jun 17, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/logins/EditLoginFragment.kt Show resolved Hide resolved

eliserichards force-pushed the 10173-login-duplicates branch from 66c95e0 to 461428e Compare June 17, 2020 21:41

eliserichards added the pr:do-not-land label Jun 24, 2020

eliserichards force-pushed the 10173-login-duplicates branch 2 times, most recently from 05f794a to 0afbeec Compare July 9, 2020 19:03

eliserichards force-pushed the 10173-login-duplicates branch from b4c25cb to 13b45a8 Compare July 9, 2020 20:39

eliserichards added the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Jul 9, 2020

ekager previously requested changes Jul 13, 2020

View reviewed changes

Mugurell reviewed Jul 14, 2020

View reviewed changes

eliserichards mentioned this pull request Jul 14, 2020

Clean up use of Context and Activity within Controllers #12565

Closed

eliserichards force-pushed the 10173-login-duplicates branch from 997d70a to 453b7b3 Compare July 14, 2020 20:05

ekager reviewed Jul 15, 2020

View reviewed changes

app/metrics.yaml Show resolved Hide resolved

ekager reviewed Jul 15, 2020

View reviewed changes

app/src/main/java/org/mozilla/fenix/HomeActivity.kt Show resolved Hide resolved

eliserichards force-pushed the 10173-login-duplicates branch from 453b7b3 to b0ae597 Compare July 16, 2020 19:22

eliserichards requested a review from ekager July 16, 2020 19:37

eliserichards changed the title ~~[WIP] For #10173: login duplicates and save~~ For #10173: login duplicates and save Jul 16, 2020

eliserichards added 7 commits July 16, 2020 15:04

Extract controller into it's own class. Implement find dupes and filt…

2fe47a1

…er based on username. Create edit login controller. Add text watchers and check for duplicates. Edit controller test

Find duplicates and save to store

8930312

Retrieve duplicates from AC and check list on username text changed

b322589

Move duplicates logic into the controller

Add glean pings for delete and edit. Move logic for login manipulatio…

e2d944f

…n into the datastore.

Use correct threads in controller. Enable save button when applicable.

0358507

Save enabled in datastore. Move login data to datastore Rebase with password error states Update metrics to be more specific for edit

Create logins controller for AC calls

e415c7f

Interactor and controller methods for edit login. Add edit view to se…

d82011a

…parate out some layout manipulation. Inflate view in edit fragment. Double layout showing up. Edit view Controller tests Controller tests passing Interactor tests Lint and detekt cleanup

eliserichards force-pushed the 10173-login-duplicates branch 2 times, most recently from a35683a to 68895cb Compare July 16, 2020 20:29

ekager approved these changes Jul 16, 2020

View reviewed changes

Remove datastore and use storage controller for all logins calls to p…

9b78c12

…assword storage. Addressed comments Lint : Rebase - 1

eliserichards force-pushed the 10173-login-duplicates branch from 68895cb to 9b78c12 Compare July 16, 2020 21:35

eliserichards removed the pr:do-not-land label Jul 16, 2020

eliserichards merged commit 023a498 into mozilla-mobile:master Jul 16, 2020

eliserichards removed the 🙅 waiting Issues that are blocked or has dependencies that are not ready label Jul 16, 2020

liuche mentioned this pull request Jul 20, 2020

Releng Firefox Beta 79.1.0 #12761

Closed

9 tasks

liuche mentioned this pull request Oct 6, 2020

Issue #14142 - Telemetry renewal to 08-01-2021 #15713

Merged

3 tasks

eliserichards mentioned this pull request Jul 26, 2021

For #20516: Renew product telemetry probes expiring in August #20517

Merged

3 tasks

eliserichards mentioned this pull request Jan 28, 2022

For #22870: Renew/remove metrics set to expire in February #23453

Merged

6 tasks

MatthewTighe mentioned this pull request Jan 11, 2023

For #28190: renew telemetry expiring in 111 #28502

Merged

4 tasks

rocketsroger mentioned this pull request Oct 11, 2023

Bug 1858440 - Renew all telemetry to never expires mozilla-mobile/firefox-android#4039

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #10173: login duplicates and save #11208

For #10173: login duplicates and save #11208

eliserichards commented Jun 3, 2020 •

edited

This comment has been minimized.

ekager commented Jun 11, 2020 •

edited

eliserichards commented Jun 11, 2020

ekager left a comment

liuche left a comment •

edited

eliserichards commented Jun 12, 2020

eliserichards commented Jul 9, 2020 •

edited

ekager left a comment

ekager Jul 10, 2020

eliserichards Jul 14, 2020

ekager Jul 10, 2020

ekager Jul 10, 2020

Mugurell Jul 14, 2020

eliserichards Jul 14, 2020

eliserichards Jul 14, 2020

ekager Jul 14, 2020 •

edited

eliserichards Jul 14, 2020

ekager Jul 14, 2020

eliserichards Jul 14, 2020

ekager Jul 14, 2020

ekager Jul 13, 2020

eliserichards Jul 14, 2020

Mugurell left a comment

Mugurell Jul 14, 2020

eliserichards Jul 14, 2020

Mugurell Jul 14, 2020

Mugurell Jul 14, 2020

eliserichards Jul 14, 2020

Mugurell commented Jul 14, 2020

ekager commented Jul 16, 2020

ekager left a comment

For #10173: login duplicates and save #11208

For #10173: login duplicates and save #11208

Conversation

eliserichards commented Jun 3, 2020 • edited

Requirements for this PR

Pull Request checklist

After merge

To download an APK when reviewing a PR:

This comment has been minimized.

ekager commented Jun 11, 2020 • edited

eliserichards commented Jun 11, 2020

Request for data collection review form

ekager left a comment

Choose a reason for hiding this comment

liuche left a comment • edited

Choose a reason for hiding this comment

Data Review Form (to be filled by Data Stewards)

eliserichards commented Jun 12, 2020

eliserichards commented Jul 9, 2020 • edited

ekager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekager Jul 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugurell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mugurell commented Jul 14, 2020

ekager commented Jul 16, 2020

ekager left a comment

Choose a reason for hiding this comment

eliserichards commented Jun 3, 2020 •

edited

ekager commented Jun 11, 2020 •

edited

liuche left a comment •

edited

eliserichards commented Jul 9, 2020 •

edited

ekager Jul 14, 2020 •

edited