Initial minimal logins API connection to datastore #62

lmorchard · 2019-01-16T18:25:51Z

Connected to #21

This doesn't actually fix #21, but it's kind of a start as it connects up the add-on to the real logins info data.

Testing and Review Notes

The add-on should now display real login entries
Editing the login entries from the add-on should affect changes in the in-product login manager.
Editing entries in the in-product login manager should affect changes in the add-on.

lmorchard · 2019-01-16T18:28:43Z

Oof, actually: Adding a WIP to the title, as I appear to have broken something just before submitting 😞

Connected to mozilla-lockwise#21

lmorchard · 2019-01-16T18:44:45Z

src/list/reducers.js

@@ -37,7 +37,13 @@ export function cacheReducer(state = {items: [], currentItem: null}, action) {
  case actions.ADD_ITEM_COMPLETED:
    return {
      ...state,
-      items: [...state.items, makeItemSummary(action.item)],
+      items: [
+        // HACK: add dispatched via Logins API events may be a duplicate


Small hacky wrinkle here: Since I'm broadcasting datastore changes based on Logins API events without any source, the UI source that made the changes will get its own change dispatched back to it. This will look like a duplicate. So, I papered over that by making the "add" action idempotent here.

lmorchard · 2019-01-16T18:45:57Z

Okay, I think I worked out the issues. At least, insofar as I'm going to take off the "WIP" title and go get some lunch.

lmorchard · 2019-01-16T18:47:56Z

Oh, but one other thing I may need to investigate for this PR or in a follow-up: Nothing here calls touch() on the Logins API to register that a login has been used. Wanted to do a little more digging to make sure I don't make fetches for display end up counting as a use. (i.e. don't stick a touch() call in datastore, make it an explicit message to send on copying username / password?)

jaredhirsch

Looking good! Seems to be working for me. Called out a couple smallish things.

jaredhirsch · 2019-01-17T18:22:59Z

src/background/datastore.js

      timeCreated: Date.now(),
      timePasswordChanged: Date.now(),
    };
+    await browser.experiments.logins.add(added);


This and other logins-modifying API methods do throw if something goes wrong, so I'd wrap them in try/catch blocks and just log the errors for now. I think we can handle the errors in the relevant issues (looks like #18, #30, #34 might all be relevant).

Hmm, I can just silently log them, though datastore.js does throw errors for other validation problems and such. Kind of figured it was useful to let these throw too, though it doesn't look like we do anything useful with them yet. (e.g. report an error over a message port, etc)

datastore.js does throw errors for other validation problems and such

Ah, ok. Probably fine as is, then.

src/background/datastore.js

test/unit/background/datastore-test.js

linuxwolf · 2019-01-17T20:59:10Z

Nothing here calls touch() on the Logins API to register that a login has been used

That was on the list of things to get to from before; I'll make sure it's accounted for in our issues.

…API to apply changes

lmorchard · 2019-01-17T21:54:01Z

src/background/message-ports.js

@@ -44,19 +44,16 @@ export default function initializeMessagePorts() {
    case "add_item":
      return openDataStore().then(async (ds) => {
        const item = await ds.add(message.item);
-        broadcast({type: "added_item", item}, sender);


Realized that since I've got the datastore now broadcasting these on the Logins API event handlers, all these messages got doubled up.

This makes a lot more sense.

I hope it's clear the code was in an "evolutionary transitional" phase when you picked it up (-:

lmorchard · 2019-01-17T21:55:17Z

test/unit/background/message-ports-test.js

@@ -80,7 +83,6 @@ describe("background > message ports", () => {
    itemId = result.item.id;

    expect(result.item).to.deep.include(item);
-    expect(selfListener).to.have.callCount(0);


Since the datastore broadcasts messages to relay events from the Logins API, there's no sender or self available at that time to refrain from sending the event back to "self"

lmorchard · 2019-01-17T21:56:12Z

test/unit/mocks/browser.js

+    logins: {
+      async getAll() { return []; },
+      async add(login) {
+        browser.experiments.logins.onAdded.getListener()({ login });


I was hoping I could be less verbose here by just using this rather than browser.experiments.logins, but that apparently ends up being the wrong reference

lmorchard · 2019-01-17T21:57:52Z

Addressed comments except for wrapping things in try/catch to log errors. We might want to let those throw, so we can catch those higher up along with datastore validation errors.

jaredhirsch · 2019-01-18T16:38:56Z

Nice work 👍 Took this branch for a test drive, and all the logins add / update / delete happy paths are seamlessly mirrored across the logins management UI in Firefox, and the Lockbox management UI.

I dunno if @linuxwolf wants to take a pass before merging, but LGTM

linuxwolf

r+

linuxwolf · 2019-01-18T17:48:37Z

src/background/datastore.js

+  logins.onRemoved.addListener(({ login }) => dataStore.removeInfo(login));
+}
+
+export default openDataStore;


I like this pattern better

linuxwolf · 2019-01-18T17:52:18Z

src/background/message-ports.js

@@ -44,19 +44,16 @@ export default function initializeMessagePorts() {
    case "add_item":
      return openDataStore().then(async (ds) => {
        const item = await ds.add(message.item);
-        broadcast({type: "added_item", item}, sender);


This makes a lot more sense.

I hope it's clear the code was in an "evolutionary transitional" phase when you picked it up (-:

linuxwolf · 2019-01-18T17:55:47Z

test/unit/background/datastore-test.js

+const LOGINS_METHODS = ["getAll", "add", "update", "remove"];
+const LOGINS_EVENTS = ["Added", "Updated", "Removed"].map(name => `on${name}`);
+
+describe("background > datastore", () => {


linuxwolf · 2019-01-18T18:32:31Z

test/unit/mocks/browser.js

+    logins: {
+      async getAll() { return []; },
+      async add(login) {
+        browser.experiments.logins.onAdded.getListener()({ login });


lmorchard requested a review from a team as a code owner January 16, 2019 18:25

ghost assigned lmorchard Jan 16, 2019

ghost added the in progress label Jan 16, 2019

lmorchard changed the title ~~Initial minimal logins API connection to datastore~~ WIP: Initial minimal logins API connection to datastore Jan 16, 2019

Initial minimal logins API connection to datastore

5f749f2

Connected to mozilla-lockwise#21

lmorchard force-pushed the 21-logins-api-hookup branch from e444e5f to 5f749f2 Compare January 16, 2019 18:43

lmorchard commented Jan 16, 2019

View reviewed changes

lmorchard changed the title ~~WIP: Initial minimal logins API connection to datastore~~ Initial minimal logins API connection to datastore Jan 16, 2019

jaredhirsch suggested changes Jan 17, 2019

View reviewed changes

Revise event/message flow in datastore to rely on updates from Login …

3a839cb

…API to apply changes

lmorchard commented Jan 17, 2019

View reviewed changes

jaredhirsch approved these changes Jan 18, 2019

View reviewed changes

linuxwolf approved these changes Jan 18, 2019

View reviewed changes

lmorchard merged commit f4cd29b into mozilla-lockwise:master Jan 18, 2019

ghost removed the in progress label Jan 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial minimal logins API connection to datastore #62

Initial minimal logins API connection to datastore #62

lmorchard commented Jan 16, 2019

lmorchard commented Jan 16, 2019

lmorchard Jan 16, 2019

lmorchard commented Jan 16, 2019

lmorchard commented Jan 16, 2019 •

edited

jaredhirsch left a comment

jaredhirsch Jan 17, 2019

lmorchard Jan 17, 2019

jaredhirsch Jan 18, 2019

linuxwolf commented Jan 17, 2019

lmorchard Jan 17, 2019

linuxwolf Jan 18, 2019

lmorchard Jan 17, 2019

lmorchard Jan 17, 2019 •

edited

linuxwolf Jan 18, 2019

lmorchard commented Jan 17, 2019

jaredhirsch commented Jan 18, 2019

linuxwolf left a comment

linuxwolf Jan 18, 2019

linuxwolf Jan 18, 2019

linuxwolf Jan 18, 2019

linuxwolf Jan 18, 2019

Initial minimal logins API connection to datastore #62

Initial minimal logins API connection to datastore #62

Conversation

lmorchard commented Jan 16, 2019

Testing and Review Notes

lmorchard commented Jan 16, 2019

Choose a reason for hiding this comment

lmorchard commented Jan 16, 2019

lmorchard commented Jan 16, 2019 • edited

jaredhirsch 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

linuxwolf commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard Jan 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmorchard commented Jan 17, 2019

jaredhirsch commented Jan 18, 2019

linuxwolf 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

lmorchard commented Jan 16, 2019 •

edited

lmorchard Jan 17, 2019 •

edited