Partition contacts store to allow grouping them by Fx Account

VERIFIED FIXED in Firefox 34

Status

Hello (Loop)
Client
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox34 verified, firefox35 verified)

Details

(Whiteboard: [loop-uplift])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
Use case: contacts are linked to a specific Firefox Account, so when a different user logs into FxA with another account on the same Fx profile, a different list of contacts should be shown.

To achieve this, we'll add a special field called `_partition` that indicates to which account a contact belongs to. We'll add an index to this field as to speed up lookups.
Flags: qe-verify+
Flags: firefox-backlog+
(Assignee)

Comment 1

4 years ago
Created attachment 8500458 [details] [diff] [review]
Patch v1: partition contacts per FxA
Attachment #8500458 - Flags: feedback?(adam)

Updated

4 years ago
Blocks: 1075203

Updated

4 years ago
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Marco, did you also put this bug on the Iteration Backlog? If not, could you please do that? Thanks!
Flags: needinfo?(mmucci)
Hi Mike.  Yes, Bug 1078309 is in the IT 35.3 Iteration Backlog.
Flags: needinfo?(mmucci)

Comment 4

4 years ago
Comment on attachment 8500458 [details] [diff] [review]
Patch v1: partition contacts per FxA

Review of attachment 8500458 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little perplexed about why we're using a new column in the table and filtering on that, rather than defining multiple databases. I would imagine that, as we add features, we're going to want to store more per-user data, and rolling a per-store solution is cumbersome and error-prone.

::: browser/components/loop/MozLoopAPI.jsm
@@ +38,5 @@
>   * @param {nsIDOMWindow} targetWindow The content window to attach the API
>   */
>  const cloneErrorObject = function(error, targetWindow) {
>    let obj = new targetWindow.Error();
> +  dump("MozLoopAPI Error:\n");

Use structured logging rather than dump, please.
Attachment #8500458 - Flags: feedback?(adam)
(Assignee)

Comment 5

4 years ago
Created attachment 8500888 [details] [diff] [review]
Patch v2: use a different database for each Fx Account
Attachment #8500458 - Attachment is obsolete: true
Attachment #8500888 - Flags: review?(adam)
(Assignee)

Comment 6

4 years ago
Comment on attachment 8500888 [details] [diff] [review]
Patch v2: use a different database for each Fx Account

Forgot to add the UI-facing part.
Attachment #8500888 - Flags: review?(adam)
(Assignee)

Comment 7

4 years ago
Created attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account
Attachment #8500888 - Attachment is obsolete: true
Attachment #8500999 - Flags: review?(adam)
(Assignee)

Comment 8

4 years ago
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Review of attachment 8500999 [details] [diff] [review]:
-----------------------------------------------------------------

Matt, could you review the bits in MozLoopService.jsm for me? They touch the FxA bits to make the state switching a bit more obvious and centralized. This was needed to hook in the DB switching bits.
Attachment #8500999 - Flags: review?(MattN+bmo)

Comment 9

4 years ago
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Review of attachment 8500999 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks good. r+, assuming you fix the issues I call out below and have someone with good react knowledge look at contacts.jsx.

I have some concerns with the changes to contacts.jsx. In particular, I want to know how we know that direct state manipulation won't bite us when when have a queue of pending state changes. This is why I'd like someone with more react familiarity to take a look at those (although I do note that Bug 1074547 implies that this may be a pretty serious defect).

Dan: I'm tagging you to take a look at contacts.jsx. Ping me if you have any questions.

::: browser/components/loop/LoopStorage.jsm
@@ +118,5 @@
> + *
> + * @param {String} name New name of the database to switch to.
> + */
> +const switchDatabase = function(name) {
> +  let newName = kDatabasePrefix + name.replace("@", "_at_").replace(/[^\w]/g, "_");

Nit: use "\W" instead of "[^\w]"

@@ +126,5 @@
> +  }
> +
> +  gDatabaseName = newName;
> +  if (gDatabase) {
> +    gDatabase.close();

This makes the handling of the "gClosed" variable pretty confusing, since it no longer represents the closed state of the database. Please rename "gClosed" to something like "gShuttingDown"

@@ +221,5 @@
>      ensureDatabaseOpen(callback);
>    },
>  
>    /**
> +   * Switch to a database with a different name.

This is very confusing, since it's a very different format than what's returned by "get databaseName()". For example, this looks like it should be a no-op:

  LoopStorage.switchDatabase(LoopStorage.databaseName);

But it very much isn't.

It looks like "get databaseName()" is only used by testing. I would propose you import it directly from this file rather than adding an accessor. If you need to keep it as-is, please add very explicit documentation to the accessor that its value is not of the same format as is used by switchDatabase.

::: browser/components/loop/MozLoopService.jsm
@@ +1052,5 @@
> +    // Switched to a new profile.
> +    if ("profile" in data) {
> +      if ("error" in data) {
> +        // An error occurred while fetching the profile.
> +        this.setError("profile", error);

this.setError("profile", data.error);

::: browser/components/loop/content/js/contacts.jsx
@@ +232,3 @@
>        let contactsAPI = navigator.mozLoop.contacts;
> +      //XXXmikedeboer: I know we're hacking the state right here, but doing
> +      //               setState() right here would cause an infinite loop.

I don't know enough about react handling to understand the full implications here (aside from the react docs warning never to do what you're doing because "state" might be out-of-date/overwritten due to pending updates -- how do we know that this warning isn't applicable?).

I'm going to r? to dmose for a quick double-check of just this one issue.
Attachment #8500999 - Flags: review?(dmose)
Attachment #8500999 - Flags: review?(adam)
Attachment #8500999 - Flags: review+

Comment 10

4 years ago
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Review of attachment 8500999 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/contacts.jsx
@@ +284,5 @@
> +    shouldComponentUpdate: function(nextProps, nextState) {
> +      let currId = this.props.userProfile ? this.props.userProfile.uid : null;
> +      let nextId = nextProps.userProfile ? nextProps.userProfile.uid : null;
> +      if (currId != nextId) {
> +        this.refresh(err => {

You shouldn't rely on shouldComponentUpdate being called to check whether contacts need to be recreated - the method may be called multiple times or none at all, if nothing else triggered a state change in a parent component, or a parent component has determined it shouldn't update.

Rather, the same external event that triggered the profile change should also trigger the rebuild of the contacts list, like the removeAll notification case.

As per discussion in bug 1074547, this should probably amount to a deletion of all keys in the existing "contacts" object (instead of a setState), followed by a forceUpdate() of the React component. You can fix the removeAll case while you're at it.

Updated

4 years ago
No longer blocks: 1075203
(Assignee)

Comment 11

4 years ago
(In reply to :Paolo Amadini from comment #10)
> As per discussion in bug 1074547, this should probably amount to a deletion
> of all keys in the existing "contacts" object (instead of a setState),
> followed by a forceUpdate() of the React component. You can fix the
> removeAll case while you're at it.

Excellent suggestion! I'll do that.
(Assignee)

Comment 12

4 years ago
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Paolo already provided the feedback to fix the React stuff, so no additional review needed for this.
Attachment #8500999 - Flags: review?(dmose)
Whiteboard: [loop-uplift]
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Review of attachment 8500999 [details] [diff] [review]:
-----------------------------------------------------------------

The changes in MozLoopService.jsm aren't necessary and make things harder to understand IMO since we are never directly switching between profiles. LoopStorage.jsm can just add a notification observer for "loop-status-changed" to handle this.
Attachment #8500999 - Flags: review?(MattN+bmo) → review-
Comment on attachment 8500999 [details] [diff] [review]
Patch v2.1: use a different database for each Fx Account

Review of attachment 8500999 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/MozLoopService.jsm
@@ +1057,5 @@
> +        this.notifyStatusChanged();
> +      } else {
> +        gFxAOAuthProfile = data.profile;
> +        // Switch to use the database that belongs to this profile.
> +        LoopStorage.switchDatabase(gFxAOAuthProfile.email);

This is bad. You must use the "uid" since the email can change in the future.
(In reply to Matthew N. [:MattN] from comment #13)
> The changes in MozLoopService.jsm aren't necessary and make things harder to
> understand IMO since we are never directly switching between profiles.
> LoopStorage.jsm can just add a notification observer for
> "loop-status-changed" to handle this.

So LoopStorage.jsm doesn't currently have access to the profile information and I guess we don't want to import MozLoopService or MozLoopAPI so I think we should just build on top of bug 1047133 and put this LoopStorage.switchDatabase call in the userProfile setter. If you don't want to wait for bug 1047133, put the LoopStorage.switchDatabase call in notifyStatusChanged.

(In reply to Matthew N. [:MattN] from comment #14)
> This is bad. You must use the "uid" since the email can change in the future.

Sorry, it's late but I probably should have phrased this differently.
(Assignee)

Comment 16

4 years ago
(In reply to :Paolo Amadini from comment #10)
> As per discussion in bug 1074547, this should probably amount to a deletion
> of all keys in the existing "contacts" object (instead of a setState),
> followed by a forceUpdate() of the React component. You can fix the
> removeAll case while you're at it.

But that would result in moving toward actually manipulating the state directly again, just like in `handleContactRemove`, does it not?
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 17

4 years ago
Created attachment 8501678 [details] [diff] [review]
Patch v2.2: use a different database for each Fx Account

So I fixed two other things here; the `handleContactRemoveAll` method fix you mentioned and I now check for a uid change everywhere instead of comparing objects, which are _always_ different from each other and generally not advisable to compare against. This also made the tabs jump back to the 'call' tab each time you open the panel.
Attachment #8500999 - Attachment is obsolete: true
Attachment #8501678 - Flags: review?(paolo.mozmail)
Flags: needinfo?(paolo.mozmail)
(Assignee)

Comment 18

4 years ago
Carrying over r=abr. I made the changes Matt suggested.

Comment 19

4 years ago
Comment on attachment 8501678 [details] [diff] [review]
Patch v2.2: use a different database for each Fx Account

Review of attachment 8501678 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/LoopStorage.jsm
@@ +126,5 @@
> +
> +  gDatabaseName = name;
> +  if (gDatabase) {
> +    gDatabase.close();
> +    gDatabase = null;

Drive-by comment: "gDatabase = null" should probably be in a "finally" block, in case something goes wrong with the "close" method.

::: browser/components/loop/content/js/contacts.js
@@ +232,2 @@
>        let contactsAPI = navigator.mozLoop.contacts;
> +      

nit: whitespace

@@ +292,5 @@
> +      let currUid = this.state.userProfile ? this.state.userProfile.uid : null;
> +      let newUid = profile ? profile.uid : null;
> +      if (currUid != newUid) {
> +        // On profile change (login, logout), reload all contacts.
> +        this.state.userProfile = profile;

This should be an ordinary setState.

I would also say that, since we're not using any of the profile information to actually render the list, "_currentProfile" could be a private property on the component (set to the current profile when the component is mounted, set to null when the component is unmounted, updated here when the profile changes). In fact, it is only used here to check if the profile actually changed and trigger a total reload of the contacts.

@@ +319,5 @@
>      },
>  
>      handleContactRemoveAll: function() {
> +      // Do not allow any race conditions when removing all contacts.
> +      this.state.contacts = {};

This would be:

for (let guid of Object.getOwnPropertyNames(this.state.contacts)) {
  delete this.state.contacts[guid];
}

In other words, "this.state.contacts" never changes. In fact, this doesn't even need to be stored as part of state, as I previously mentioned in the other bug. We use "forceUpdate" exactly because we render from state that is stored externally.

I know there are better architectures, but I think these two small changes will get us to a safe place without too much uplift hassle.
Attachment #8501678 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 20

4 years ago
(In reply to :Paolo Amadini from comment #19)
> This would be:
> 
> for (let guid of Object.getOwnPropertyNames(this.state.contacts)) {
>   delete this.state.contacts[guid];
> }

so how is this different from this.state.contacts = {} ?

Let's talk about this.contacts from now on and indeed circumvent using this.state entirely. It's too confusing.
(Assignee)

Comment 21

4 years ago
Created attachment 8501798 [details] [diff] [review]
Patch v2.3: use a different database for each Fx Account
Attachment #8501678 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8501804 [details] [diff] [review]
Patch v2.4: use a different database for each Fx Account
Attachment #8501798 - Attachment is obsolete: true
Attachment #8501804 - Flags: review?(paolo.mozmail)

Updated

4 years ago
Attachment #8501804 - Flags: review?(paolo.mozmail) → review+
Green try, tree is open.  Landing for Mike per mreavy
https://hg.mozilla.org/integration/fx-team/rev/047060a5b1dc
Comment on attachment 8501804 [details] [diff] [review]
Patch v2.4: use a different database for each Fx Account

Review of attachment 8501804 [details] [diff] [review]:
-----------------------------------------------------------------

Backed out in https://hg.mozilla.org/integration/fx-team/rev/dcb04b36ad60

I was going to just land a patch that fixed the gFxAOAuthProfile rebasing but I'm not sure how you want to fix the default database name issue.

::: browser/components/loop/LoopStorage.jsm
@@ +229,5 @@
> +   * @param {String} name New name of the database to switch to. Defaults to
> +   *                      `kDefaultDatabaseName`
> +   */
> +  switchDatabase: function(name = kDefaultDatabaseName) {
> +    switchDatabase(name);

This causes browser_LoopContacts.js to fail at line 426 since it is missing the prefix on the default database name.

::: browser/components/loop/MozLoopService.jsm
@@ +329,5 @@
>    },
>  
>    notifyStatusChanged: function(aReason = null) {
>      log.debug("notifyStatusChanged with reason:", aReason);
> +    LoopStorage.switchDatabase(gFxAOAuthProfile ? gFxAOAuthProfile.uid : null);

gFxAOAuthProfile was removed in bug 1047133.
-    LoopStorage.switchDatabase(gFxAOAuthProfile ? gFxAOAuthProfile.uid : null);
+    let profile = MozLoopService.userProfile;
+    LoopStorage.switchDatabase(profile ? profile.uid : null);
Attachment #8501804 - Flags: review-
Flags: in-testsuite+
(Assignee)

Comment 27

4 years ago
Created attachment 8502427 [details] [diff] [review]
Patch v2.5: use a different database for each Fx Account

Patch ready for landing. Carrying over r=abr,paolo.
Attachment #8501804 - Attachment is obsolete: true
Attachment #8502427 - Flags: review+
Comment on attachment 8502427 [details] [diff] [review]
Patch v2.5: use a different database for each Fx Account

Approval Request Comment
Part of the staged Loop aurora second uplift set
Attachment #8502427 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/125453eb266c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Hi Pauly, To verify this you'll need 2 FxA accounts.  Log into one FxA account, create or import contacts, log out and then log into the second account.  You should not see the contacts from the first FxA account when you log into the second FxA account.
Flags: needinfo?(paul.silaghi)
Verified fixed 35.0a1 (2014-10-10) Win 7, Ubuntu 13.04, OS X 10.9.5
Status: RESOLVED → VERIFIED
status-firefox35: --- → verified
Flags: needinfo?(paul.silaghi)

Updated

4 years ago
status-firefox34: --- → fixed
Flags: needinfo?(paul.silaghi)
Verified fixed FF 34b1 Win 7
status-firefox34: fixed → verified
Flags: needinfo?(paul.silaghi)
Comment on attachment 8502427 [details] [diff] [review]
Patch v2.5: use a different database for each Fx Account

Post landed approval (already landed)
Attachment #8502427 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.