Skip to content
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

Expect a complete Login for update in Login API #80

Merged

Conversation

@lmorchard
Copy link
Contributor

commented Feb 13, 2019

Fixes #58

Testing and Review Notes

Nothing apparent should change. Integration tests should pass, manual item editing should work as expected.

@lmorchard lmorchard requested a review from mozilla-lockwise/desktop-engineering as a code owner Feb 13, 2019

@ghost ghost assigned lmorchard Feb 13, 2019

@ghost ghost added the in progress label Feb 13, 2019

delete updatedLogin.timesUsed;
}
const updatedLoginBag = LoginHelper.newPropertyBag(updatedLogin);
const updatedLoginInfo = LoginHelper.vanillaObjectToLogin(updatedLogin);

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 13, 2019

Author Contributor

I scratched my head at the description of this issue for a little bit. But, I think the crux of the issue is where modifyLogin() can accept either an nsIPropertyBag or an nsILoginMetaInfo instance.

Switched to the latter, here, on assumption of a full login object modified by the calling code as necessary, rather than a sparse set of updates. So I think that's me understanding the issue correctly.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 13, 2019

Member

I think it'd be better to stick with the PropertyBag as the second argument to modifyLogin.

Sorry this wasn't clearer from the bug description. The LoginInfo fields can be updated in this way, but not the LoginMetaInfo fields (timestamps and usage count). As I understand it, the only reason there's a separate interface for the MetaInfo fields is that they were needed after login sync was already done, so each login object actually implements both interfaces: the syncable part, and the non-syncable part. Even with the historical background in mind, it's a confusing and, uh, suboptimal situation.

We could just rely on the touch() method to alter the MetaInfo fields, but that makes update() an annoyingly misleading function name, and causes the grody dual-interface situation to complicate our lives.

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 15, 2019

Author Contributor

Switched back to a property bag. But, it looks like we might have to rely on touch() for using the timesUsedIncrement property.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

relying on touch() seems reasonable to me.

then(log, log);
browser.experiments.logins.get(mockLogin.guid)
.then(login =>
browser.experiments.logins.update({ ...login, username: "updated" }))

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 13, 2019

Author Contributor

Oh, and also I think this represents correctly the change in assumptions. I was already using the API this way in datastore.js, oddly enough, which made no functional difference really.

This comment has been minimized.

Copy link
@6a68

6a68 Feb 13, 2019

Member

This looks good, but I'm wondering if we should add clues about the special MetaInfo fields (including the magic timesUsedIncrement property). Would you mind also setting timesUsedIncrement: 1 here, and also bump one of the MetaInfo timestamp fields, then adding the corresponding asserts on the test side? Maybe this'll help future contributors detect the weirdness of the underlying APIs.

This comment has been minimized.

Copy link
@lmorchard

lmorchard Feb 15, 2019

Author Contributor

Banged my head on this for awhile and I think I've discovered this:

If we supply both timesUsed and timesUsedIncrement in the property bag at the same time, the explicit timesUsed value will likely clobber whatever the result of timesUsedIncrement might be.

That is, depending on whatever the order of enumerated properties ends up being. I haven't dug deep enough to figure out what that order is, though it seemed pretty consistently to clobber the increment.

Worse news is if I try to omit timesUsed in the update call, it comes in as null - which is treated as a 0 for the update.

On the bright side, I was able to update timePasswordChanged.

This comment has been minimized.

Copy link
@linuxwolf

linuxwolf Feb 19, 2019

Member

I think it's reasonable for update() to actively omit timesUsedIncrement -- either the user of this API is properly incrementing timesUsed by hand, or they can use touch().

thoughts? @lmorchard @6a68

@6a68
Copy link
Member

left a comment

Looking good! We've gotten well into the sketch of the LoginInfo and LoginMetaInfo APIs here, couple changes to hopefully smooth the road for future selves or future contributors.

delete updatedLogin.timesUsed;
}
const updatedLoginBag = LoginHelper.newPropertyBag(updatedLogin);
const updatedLoginInfo = LoginHelper.vanillaObjectToLogin(updatedLogin);

This comment has been minimized.

Copy link
@6a68

6a68 Feb 13, 2019

Member

I think it'd be better to stick with the PropertyBag as the second argument to modifyLogin.

Sorry this wasn't clearer from the bug description. The LoginInfo fields can be updated in this way, but not the LoginMetaInfo fields (timestamps and usage count). As I understand it, the only reason there's a separate interface for the MetaInfo fields is that they were needed after login sync was already done, so each login object actually implements both interfaces: the syncable part, and the non-syncable part. Even with the historical background in mind, it's a confusing and, uh, suboptimal situation.

We could just rely on the touch() method to alter the MetaInfo fields, but that makes update() an annoyingly misleading function name, and causes the grody dual-interface situation to complicate our lives.

then(log, log);
browser.experiments.logins.get(mockLogin.guid)
.then(login =>
browser.experiments.logins.update({ ...login, username: "updated" }))

This comment has been minimized.

Copy link
@6a68

6a68 Feb 13, 2019

Member

This looks good, but I'm wondering if we should add clues about the special MetaInfo fields (including the magic timesUsedIncrement property). Would you mind also setting timesUsedIncrement: 1 here, and also bump one of the MetaInfo timestamp fields, then adding the corresponding asserts on the test side? Maybe this'll help future contributors detect the weirdness of the underlying APIs.

@6a68
6a68 approved these changes Feb 21, 2019
Copy link
Member

left a comment

@lmorchard Definitely owe you a beer for this one; I was wrong to suggest sticking with the PropertyBag. Just using update() to modify the Login fields is the way to go, as it lets us avoid these auto-inserted nulls, we can still bump the usage metadata via touch(), and we don't have to pull object validation out of the schema and into our API code. Feel free to strip out the latest commit and just land the fd1e141 commit.

@lmorchard lmorchard force-pushed the lmorchard:29-update-complete-login branch from e6ffb9a to fd1e141 Feb 21, 2019

@lmorchard lmorchard force-pushed the lmorchard:29-update-complete-login branch from fd1e141 to 3b17efa Feb 21, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

Okie doke, I'll merge this and then we can see if anything else needs doing from here.

@lmorchard lmorchard merged commit dacf0cf into mozilla-lockwise:master Feb 21, 2019

4 checks passed

WIP Ready for review
Details
codecov/patch Coverage not affected when comparing 2cb7ab4...3b17efa
Details
codecov/project 95.9% remains the same compared to 2cb7ab4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Feb 21, 2019

@lmorchard

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2019

FWIW, I was about to file a follow-up issue since I thought timePasswordChanged wasn't updated automatically - but it looks like that does happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.