Closed
Bug 1078309
Opened 10 years ago
Closed 10 years ago
Partition contacts store to allow grouping them by Fx Account
Categories
(Hello (Loop) :: Client, defect)
Hello (Loop)
Client
Tracking
(firefox34 verified, firefox35 verified)
People
(Reporter: mikedeboer, Assigned: mikedeboer)
References
Details
(Whiteboard: [loop-uplift])
Attachments
(1 file, 6 obsolete files)
23.94 KB,
patch
|
mikedeboer
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8500458 -
Flags: feedback?(adam)
Updated•10 years ago
|
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Marco, did you also put this bug on the Iteration Backlog? If not, could you please do that? Thanks!
Flags: needinfo?(mmucci)
Comment 3•10 years ago
|
||
Hi Mike. Yes, Bug 1078309 is in the IT 35.3 Iteration Backlog.
Flags: needinfo?(mmucci)
Comment 4•10 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•10 years ago
|
||
Attachment #8500458 -
Attachment is obsolete: true
Attachment #8500888 -
Flags: review?(adam)
Assignee | ||
Comment 6•10 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•10 years ago
|
||
Attachment #8500888 -
Attachment is obsolete: true
Attachment #8500999 -
Flags: review?(adam)
Assignee | ||
Comment 8•10 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•10 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•10 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.
Assignee | ||
Comment 11•10 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•10 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)
Updated•10 years ago
|
Whiteboard: [loop-uplift]
Comment 13•10 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]: ----------------------------------------------------------------- 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 14•10 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/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.
Comment 15•10 years ago
|
||
(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•10 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•10 years ago
|
||
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•10 years ago
|
||
Carrying over r=abr. I made the changes Matt suggested.
Comment 19•10 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•10 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•10 years ago
|
||
Attachment #8501678 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8501798 -
Attachment is obsolete: true
Attachment #8501804 -
Flags: review?(paolo.mozmail)
Updated•10 years ago
|
Attachment #8501804 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8bf8f6cfea8c
Assignee | ||
Comment 24•10 years ago
|
||
Try push with bustage fix: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2bc3a602ed46
Comment 25•10 years ago
|
||
Green try, tree is open. Landing for Mike per mreavy https://hg.mozilla.org/integration/fx-team/rev/047060a5b1dc
Comment 26•10 years ago
|
||
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-
Updated•10 years ago
|
Flags: in-testsuite+
Assignee | ||
Comment 27•10 years ago
|
||
Patch ready for landing. Carrying over r=abr,paolo.
Attachment #8501804 -
Attachment is obsolete: true
Attachment #8502427 -
Flags: review+
Assignee | ||
Comment 28•10 years ago
|
||
Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/125453eb266c
Comment 29•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 31•10 years ago
|
||
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)
Comment 32•10 years ago
|
||
Verified fixed 35.0a1 (2014-10-10) Win 7, Ubuntu 13.04, OS X 10.9.5
Comment 33•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb36ba6e23e7 https://hg.mozilla.org/releases/mozilla-aurora/rev/6dfc2524085b
Updated•10 years ago
|
status-firefox34:
--- → fixed
Flags: needinfo?(paul.silaghi)
Comment 35•10 years ago
|
||
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.
Description
•