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

Add discussion around declined handling #1208

Closed
wants to merge 1 commit into from

Conversation

mhammond
Copy link
Member

I've made significant changes to the sync-manager document around declined handling, specifically to avoid the issue we had on Desktop with addresses and credit-cards. I'm sorry it's so long, but I'd really appreciate feedback on the approach I suggest and help finding the holes in it.

Most of the changes are in this section

will supply explicit "Enabled" or "Disabled" values - nothing will be specified
as "Unknown".
* Because this will be the first sync for the account, no other engines will
exist on the server, so the exact same states will be returned after the sync.
Copy link
Member Author

@mhammond mhammond May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear this is true: consider acct creation on a device which doesn't support "passwords" although it is offered. After the sync, passwords will not be in declined, but also will not exist as a collection, so will be returned as Unknown. From the above rules, passwords will be declined as there's nowhere to record the acceptance :(

Do we need to bite-the-bullet and add accepted as a sibling of declined and thus ignore the state of info/collections in all the above?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear this is true: consider acct creation on a device which doesn't support "passwords" although it is offered.

Hmm, I don't follow...why would a device that doesn't support passwords (like Fenix, at least for now) offer them in CWTS?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was thinking of when, say, CWTS is offered by the server as part of account creation. Ryan has a doc talking about how account creation isn't the right place and it's going to get "more wrong" as accounts are created for non-sync purposes - however, he still proposes offering CWTS exactly once - so I guess I can see a future where we decide to offer "all" collections even if the current app only supports a subset.

Not clear though - it's difficult to predict future UX asks, should we probably should allow for this capability?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we settle on adding accepted in the end?

Copy link
Contributor

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broadly I think the concrete changes here are good. I think the accepted list discussed today would be a positive change but I'm not sure if it's necessary (It does feel like the kind of thing we might regret adding, so we should probably just do it) though.

* A "Beta" device, which has just been upgraded, and this upgrade is the
very first time it understands anything about the new engine.

* A "Release" device, which doesn't understand anything about the new engine.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary for the example.


#### How do new engines become enabled?

As implied above, the state of an engine transitions from "Unknown" to "Allowed"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I guess the hypothetical "accepted" list you mentioned earlier would, if present, be a list of engines whose values would never be unknown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They would never be returned as unknown. For a first sync on a device where CWTS is not offered, they may be supplied as unknown though. Does that clarify?

Copy link
Contributor

@linabutler linabutler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this @mhammond! I left a few questions and comments, this is shaping up nicely! 🚀


#### How the sync manager resolves these

There's no canonical list of engines, so somehow the sync manager needs to work
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this lack of a canonical list is why iOS resorts to uploading a synthetic meta/global that mentions all Desktop engines, even ones it doesn't support.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That and https://bugzilla.mozilla.org/show_bug.cgi?id=1479929, right? I don't think there can ever be a canonical list (ie, there's always a future where one could be added)

that in practice, this might be better described as "Exists" - there's no
explicit list of allowed engines. I'm not sure that distinction is worthwhile
though?)
* Unknown - the engine state is not known by this application.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, before the very first sync, the app supplies a list of all engines it knows about, which are all Unknown. After the sync, the sync manager returns new states, which includes any engine state changes (from Unknown to Allowed and Declined). Additional engines that we don't support and that other devices do support (or are these only ever going to be Allowed and Declined), or new engines that we support but other devices don't, will remain Unknown. Is that right?

Edit: Wait, I think I'm confused now. Another device might opt-in to syncing an engine that we don't know about, so it might still be Allowed or Declined, right? So Unknown only ever shows up if we provide an engine that's not already in m/g? But then what about the case where there is no m/g? Without a canonical list of engines, how do we know if something is Unknown or known?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, before the very first sync, the app supplies a list of all engines it knows about, which are all Unknown

Assuming no CWTS, yes.

Additional engines that we don't support and that other devices do support

I saw these as being returned as Unknown too.

or new engines that we support but other devices don't, will remain Unknown. Is that right?

hrmph - that's a good question. Assuming no CWTS, that engine also coming back as Unknown seems fine too - the app can use that as an indication that some kind of CWTS should be offered for this engine, or maybe they could just wait until the equiv of about:sync is shown and treat that as affirmation.

IOW, a known engine coming back as unknown simply means the user has yet to choose how to handle this engine.

Edit: Wait, I think I'm confused now.

I'm not sure what you are saying there - but a new device showing up to find empty storage is an edge-case we don't handle well now and probably will not in the future either. So "durable storage" ftw? :(

must expect that the engines it gets back after the sync might include engines
that were not initially specified - it will get the *complete* set of known
engines and their state. The application is expected to persist these values and
supply them to subsequent `sync()` calls, although it need not provide UI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can move toward persisting this state ourselves in something like rkv, or even a flat file, instead of having the app hold and manage it. @thomcc, I think you were receptive to the idea in Whistler?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with that if we decide it's the right way forward. I've been coming at it from the POV that the app side of the manager already needs storage for things like it's preferences, whereas we don't need storage, so adding rkv is another dependency and additional complexity (eg, what happens if the app loses all storage but we don't, or vice-versa?)


If a device supplies "Unknown" for an engine, and there's no storage for the
account (eg, a device syncing after a node reassignment) it will not sync that
engine. This is because it doesn't know what the state for that engine should
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Will not sync that engine" means the engine won't be mentioned in m/g that the device uploads, and won't have a collection on the server, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

will supply explicit "Enabled" or "Disabled" values - nothing will be specified
as "Unknown".
* Because this will be the first sync for the account, no other engines will
exist on the server, so the exact same states will be returned after the sync.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear this is true: consider acct creation on a device which doesn't support "passwords" although it is offered.

Hmm, I don't follow...why would a device that doesn't support passwords (like Fenix, at least for now) offer them in CWTS?

* The application is expected to persist *all* of these values - including
"passwords", even though it doesn't understand that collection. This value
may be used to repopulate the declined list should this device be the first to
sync after storage is reset.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, OK, I think this answers my question above—engines that we don't support, but other devices do, might still be Allowed or Declined.


If there is no storage things get tricky. Consider the Fenix example above -
it will find no storage, and even though it doesn't sync passwords, it knows
passwords should be enabled. It will not put passwords in the declined list, but
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this because otherwise, an existing legacy client that comes along to sync would see passwords isn't in m/g, and disable the engine locally? If not, how does Fenix know that passwords should be enabled, even though it doesn't support them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does Fenix know that passwords should be enabled, even though it doesn't support them?

I was thinking that part of the state we persist is the entire "accepted" and "declined" values from m/g without regard for whether the app actually supports the collection.

`{passwords: Enabled}`, it will now get back `{passwords: Unknown}`. This will
only be a problem is Fenix decides to show "passwords" in its "choose what to
sync" - so Fenix should probably avoid offering unknown engines (which is fine,
because that would probably confuse the user anyway!) Regardless, in this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK, this answers my question from earlier about why Fenix would do this.

"choose what to sync" UI after connecting and after the first sync, but
it might also need to handle "incidental" showing of this UI before that
first sync completes - for example, simply *showing* the UI probably shouldn't
force changing the state of an engine from "Unknown". The UX challenge is to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

of an engine while there's no network, then exits the app, how do we ensure the
user's new state is updated next time the app starts? What if the user has since
made a different state request on a different device? Is the state as last-known
on this device considered canonical?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a three-way merge would be reasonable here, and easy to implement (famous last words 😉)—we're already keeping the last-known engine selections locally anyway; enabling or disabling an engine would flag it as overridden, and we'd merge the changes with other devices on the next sync. I think that would be less surprising than taking the complete set of engine selections, potentially reverting changes on other devices. Especially since engine selections are global, but users might not realize that, and assume they're per-device ("I unchecked history on my work laptop, then bookmarks on my home; why did unchecking bookmarks re-enable history?")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've no real objection to that, but ISTM that either scenario might surprise some users. I agree we should surprise the smallest number of users we can though (but if it seems 50:50, we might as well do the simpler of the things)

@rfk
Copy link
Contributor

rfk commented Jan 20, 2020

This PR has been open for a while now, @mhammond should we try to re-invigorate and close out this discussion?

@mhammond
Copy link
Member Author

This PR has been open for a while now, @mhammond should we try to re-invigorate and close out this discussion?

I think we should do that as part of actually implementing it, which we will hopefully do this quarter. (Trying to pre-define the behaviour didn't help it actually get implemented the first time around, and having "aspirational" docs checked in without a clear plan for implementation seems a bit like a foot-gun for everyone)

@mhammond mhammond closed this Feb 19, 2020
@mhammond mhammond deleted the declined-docs branch February 19, 2020 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants