feat(login): let users opt-out of sync when signing in to Firefox #2534

@@ -44,7 +44,7 @@ import Session from './session';
 import Storage from './storage';
 import StorageMetrics from './storage-metrics';
 import SupplicantRelier from '../models/reliers/pairing/supplicant';
-import SyncRelier from '../models/reliers/sync';
+import BrowserRelier from '../models/reliers/browser';


Dig the name 👍🏽

vbudhram · 2019-09-26T20:18:23Z

packages/fxa-content-server/app/scripts/lib/app-start.js

+      } else if (
+        this._isServiceSync() ||
+        // context v3 is able to sign in to Firefox without enabling Sync
+        this._searchParam('context') === Constants.FX_DESKTOP_V3_CONTEXT


Lets make sure we update documentation where needed.

vbudhram · 2019-09-26T20:23:00Z

packages/fxa-content-server/app/scripts/models/auth_brokers/fx-sync.js

+      response.capabilities && response.capabilities.multiService;
+    this.relier.set('multiService', multiService);
+    if (multiService) {
+      this.relier.set('service', response.clientId);


This is a little unclear to me, what is the clientId in this case? Sync or one of the services?

I will add some docs for it., but to explain here:

we return the OAuth client id for Desktop Firefox in order to replace some uses of service=sync on the backend

vbudhram · 2019-09-26T20:25:57Z

packages/fxa-content-server/app/scripts/models/reliers/browser.js

@@ -20,6 +20,10 @@ const QUERY_PARAMETER_SCHEMA = {
  // context is not available when verifying.
  context: Vat.string().min(1),
  country: Vat.string().valid(...AllowedCountries),
+  // user is signing into the browser and declined to Sync
+  doNotSync: Vat.boolean(),


Is declinedSync a better name here?

Ah I see, the button is called Don't sync.... I think this var name should be fine.

vladikoff · 2019-09-26T21:38:14Z

@vladikoff Did a first pass at this, but will do another. Is there a custom build of FF that I can use to test this?

Thank you!
Yeap see the Testing this change in the first comment of this PR.

shane-tomlinson · 2019-09-27T14:32:37Z

When signing in, I am presented with the screen that asks whether I want to Sync. If I click the blue button to sync the device, CWTS displays and then I am redirected to the SMS form immediately:

If I click the browser's "back" button from there, then "this.onFormSubmitComplete is not a function" shows up in the console.

vbudhram · 2019-09-27T14:46:21Z

I'm also getting

Which I think makes sense since there is no return function here?

fxa/packages/fxa-content-server/app/scripts/models/auth_brokers/fx-sync-channel.js

Line 306 in f4b3caf

delete loginData.declinedSyncEngines;

shane-tomlinson · 2019-09-27T14:49:27Z

When signing up, clicking on "Don't sync this device" has no effect:

vbudhram · 2019-09-27T16:10:58Z

Hey @vladikoff, nice work! I was able to do some more testing after applying the return fix. Here some notes:

No sync specific emails, was the intention to do Let users opt-out of sync when signing in to Firefox #2396 (comment) in a follow up or are we nuking sync specific messaging?
I received a Sync Enabled...will begin syncing momentarily push notification, when I opted to not to enable sync. Would that be fixed as part of markh patch?
When doing the sign-in and enable syncing, the CWTS page flashes and logs me in

Sign-in confirmation links works as expected! The user will get the Confirm sign-in email, and clicking it will verify the account

vladikoff · 2019-09-27T18:13:38Z

Sorry the loginData issue has now been fixed, I accidentally refactored it out...

vladikoff · 2019-09-27T18:24:51Z

I received a Sync Enabled...will begin syncing momentarily push notification, when I opted to not to enable sync. Would that be fixed as part of markh patch?

Yeap different, patch, filed in
mozilla/application-services#1893

vladikoff · 2019-09-27T18:27:15Z

No sync specific emails

yea! ff now sends client id

vladikoff · 2019-09-27T20:46:37Z

@shane-tomlinson @vbudhram please try now!

vbudhram

Thanks @vladikoff, this LGTM 👍!

packages/fxa-content-server/app/scripts/models/auth_brokers/fx-sync-channel.js

Fixes #2396

shane-tomlinson

Only two small questions, otherwise r+!

shane-tomlinson · 2019-09-30T15:09:00Z

packages/fxa-content-server/app/scripts/views/would_you_like_to_sync.js

+
+const SCREEN_CLASS = 'screen-would-you-like-to-sync';
+
+const View = FormView.extend(


Is the account already created and email sent when this view is displayed? If so, does this view need to poll similar to CWTS in case the user verifies, or are we punting on that b/c of boomerang?

boomerang + I think in most cases when there's a choice of SYNC vs NOT-Sync, we want to wait for the user...

@ryanfeeley might confirm

shane-tomlinson · 2019-09-30T15:10:18Z

packages/fxa-content-server/app/tests/spec/views/choose_what_to_sync.js


-        assert.isTrue($('body').hasClass(View.SCREEN_CLASS));
+    it('does not poll', () => {


When does this happen?

same scenario as #2534 (comment) , I spoke to @ryanfeeley and for now with email verification we do not poll when there's a choice of not syncing

Requires mozilla/fxa#2534 in production

vladikoff added a commit to vladikoff/activity-stream that referenced this pull request Sep 17, 2019

Fix Bug 1577690 - make sync optional in the about:welcome signin flow

e9334cd

Requires mozilla/fxa#2534 in production

vladikoff mentioned this pull request Sep 17, 2019

Fix Bug 1582002 - make sync optional in the about:welcome signin flow mozilla/activity-stream#5314

Merged

vladikoff added a commit to vladikoff/activity-stream that referenced this pull request Sep 17, 2019

Fix Bug 1577690 - make sync optional in the about:welcome signin flow

dff33c2

Requires mozilla/fxa#2534 in production

vladikoff force-pushed the i2396-no-sync-ff branch from 5d051dc to c8b9477 Compare September 18, 2019 15:35

vladikoff added this to the Train 147: FxA milestone Sep 18, 2019

vladikoff force-pushed the i2396-no-sync-ff branch from c8b9477 to c0402e7 Compare September 18, 2019 18:02

vladikoff force-pushed the i2396-no-sync-ff branch 5 times, most recently from b8e7728 to 54cc493 Compare September 26, 2019 20:11

vladikoff requested review from shane-tomlinson and vbudhram September 26, 2019 20:13

vladikoff marked this pull request as ready for review September 26, 2019 20:13

vladikoff self-assigned this Sep 26, 2019

This was referenced Sep 26, 2019

Let users opt-out of sync when signin in to Firefox mozilla/application-services#1743

Closed

Browser support for opting out of sync during the Firefox signin flow mozilla/application-services#1690

Closed

vladikoff commented Sep 26, 2019

View reviewed changes

vbudhram reviewed Sep 26, 2019

View reviewed changes

vladikoff force-pushed the i2396-no-sync-ff branch from 54cc493 to f4b3caf Compare September 26, 2019 23:22

vladikoff force-pushed the i2396-no-sync-ff branch from f4b3caf to aeda104 Compare September 27, 2019 18:10

vladikoff force-pushed the i2396-no-sync-ff branch 2 times, most recently from 9ed7455 to 9527a87 Compare September 27, 2019 20:27

vladikoff requested a review from vbudhram September 27, 2019 21:07

vbudhram approved these changes Sep 30, 2019

View reviewed changes

packages/fxa-content-server/app/scripts/models/auth_brokers/fx-sync-channel.js Outdated Show resolved Hide resolved

feat(login): let users opt-out of sync when signing in to Firefox

c9f6a95

Fixes #2396

vladikoff force-pushed the i2396-no-sync-ff branch from 9527a87 to c9f6a95 Compare September 30, 2019 14:39

vladikoff merged commit 7a14b1f into master Sep 30, 2019

vladikoff deleted the i2396-no-sync-ff branch September 30, 2019 15:04

shane-tomlinson approved these changes Sep 30, 2019

View reviewed changes

vladikoff added a commit to vladikoff/activity-stream that referenced this pull request Oct 9, 2019

Fix Bug 1577690 - make sync optional in the about:welcome signin flow

f3c5903

Requires mozilla/fxa#2534 in production

vladikoff added a commit to vladikoff/activity-stream that referenced this pull request Oct 9, 2019

Fix Bug 1582002 - make sync optional in the about:welcome signin flow

27cce7a

Requires mozilla/fxa#2534 in production

vladikoff added a commit to vladikoff/activity-stream that referenced this pull request Oct 15, 2019

Fix Bug 1582002 - make sync optional in the about:welcome signin flow

fe65dca

Requires mozilla/fxa#2534 in production

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(login): let users opt-out of sync when signing in to Firefox #2534

feat(login): let users opt-out of sync when signing in to Firefox #2534

vladikoff commented Sep 17, 2019 •

edited

Loading

vladikoff commented Sep 26, 2019

vbudhram left a comment

vbudhram Sep 26, 2019

vbudhram Sep 26, 2019

vbudhram Sep 26, 2019

vladikoff Sep 26, 2019

vbudhram Sep 26, 2019

vbudhram Sep 26, 2019

vladikoff commented Sep 26, 2019

shane-tomlinson commented Sep 27, 2019 •

edited by vladikoff

Loading

vbudhram commented Sep 27, 2019 •

edited by vladikoff

Loading

shane-tomlinson commented Sep 27, 2019 •

edited by vladikoff

Loading

vbudhram commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vbudhram left a comment

shane-tomlinson left a comment

shane-tomlinson Sep 30, 2019

vladikoff Sep 30, 2019

shane-tomlinson Sep 30, 2019

vladikoff Sep 30, 2019


		const SCREEN_CLASS = 'screen-would-you-like-to-sync';

		const View = FormView.extend(


		assert.isTrue($('body').hasClass(View.SCREEN_CLASS));
		it('does not poll', () => {

feat(login): let users opt-out of sync when signing in to Firefox #2534

feat(login): let users opt-out of sync when signing in to Firefox #2534

Conversation

vladikoff commented Sep 17, 2019 • edited Loading

Testing this change

vladikoff commented Sep 26, 2019

vbudhram 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladikoff commented Sep 26, 2019

shane-tomlinson commented Sep 27, 2019 • edited by vladikoff Loading

vbudhram commented Sep 27, 2019 • edited by vladikoff Loading

shane-tomlinson commented Sep 27, 2019 • edited by vladikoff Loading

vbudhram commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vladikoff commented Sep 27, 2019

vbudhram left a comment

Choose a reason for hiding this comment

shane-tomlinson 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

vladikoff commented Sep 17, 2019 •

edited

Loading

shane-tomlinson commented Sep 27, 2019 •

edited by vladikoff

Loading

vbudhram commented Sep 27, 2019 •

edited by vladikoff

Loading

shane-tomlinson commented Sep 27, 2019 •

edited by vladikoff

Loading