Skip to content
This repository has been archived by the owner. It is now read-only.

fix(settings): unblock rendering of settings on communication prefs screen #3088

Merged
merged 1 commit into from Sep 30, 2015

Conversation

@zaach
Copy link
Contributor

@zaach zaach commented Sep 21, 2015

Fixes #3061. Using afterVisible from subviews does not block rendering of the settings page.

@shane-tomlinson r?

@zaach zaach force-pushed the issue-3061-email-prefs-non-blocking branch from 806dff2 to a1226c4 Sep 21, 2015
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 21, 2015

@zaach - With this diff applied, I see some unexpected behavior:

diff --git a/app/scripts/lib/marketing-email-client.js b/app/scripts/lib/marketing-email-client.js
index b30f3f1..8b6c641 100644
--- a/app/scripts/lib/marketing-email-client.js
+++ b/app/scripts/lib/marketing-email-client.js
@@ -42,6 +42,8 @@ define([
       var self = this;
       return this._request('get', '/lookup-user', accessToken)
         .then(function (response) {
+          throw MarketingEmailErrors.toError('NETWORK_FAILURE');
+
           // TODO
           // I would prefer to place this into the MarketingEmailPrefs model
           // but doing so required passing around the preferencesUrl to lots of

No error message is displayed. Expected?

@rfk rfk added this to the FxA-0: quality milestone Sep 21, 2015
@zaach zaach force-pushed the issue-3061-email-prefs-non-blocking branch from a1226c4 to d9f8b24 Sep 23, 2015
@zaach zaach added the WIP label Sep 23, 2015
@zaach zaach force-pushed the issue-3061-email-prefs-non-blocking branch 3 times, most recently from 6b2d48c to 7cb060a Sep 28, 2015
@zaach zaach removed the WIP label Sep 28, 2015
@zaach zaach assigned shane-tomlinson and unassigned zaach Sep 28, 2015
@shane-tomlinson shane-tomlinson force-pushed the issue-3061-email-prefs-non-blocking branch from 7cb060a to 9e97ce2 Sep 29, 2015
return emailPrefs.fetch()
.fail(function (err) {
.then(function () {
return self.render();

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Sep 29, 2015
Member

For this PR this approach is fine, but it does allow for a flash of button content if the user reloads the page with the communications_preferences panel open:

subscribe-unsubscribe-flash

A solution to this is to render the page with a "loading" button (gray button with throbber) while the fetch is outstanding, then re-render the button (or the entire screen) with the appropriate text once the emailPrefs fetch has completed.

Since re-loading the page with the communication prefs screen is probably not a common occurrence, "meh" for now.

@shane-tomlinson shane-tomlinson force-pushed the issue-3061-email-prefs-non-blocking branch from 9e97ce2 to 2118e53 Sep 29, 2015
@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 29, 2015

If all the tests go green, r+, thanks @zaach!

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 29, 2015

@zaach - the communication preferences - do not opt in to the marketing email consistently fails for me.

× firefox on any platform - communication preferences - do not opt in to the marketing email (30.044s)
CancelError: Timeout reached on firefox on any platform - communication preferences - do not opt in to the marketing email
  at null._onTimeout  <node_modules/intern/lib/Test.js:162:20>
  at Timer.listOnTimeout [as ontimeout]  <timers.js:121:15>
  at Command.target.(anonymous function) [as click]  <node_modules/intern/node_modules/leadfoot/Command.js:674:11>
  at Test.registerSuite.do not opt in to the marketing email [as test]  <tests/functional/email_opt_in.js:215:12>
  at start.then.finally.self.timeElapsed  <node_modules/intern/lib/Test.js:211:24>
  at <node_modules/intern/node_modules/dojo/Promise.ts:393:15>
  at runCallbacks  <node_modules/intern/node_modules/dojo/Promise.ts:11:11>
  at <node_modules/intern/node_modules/dojo/Promise.ts:317:4>
  at Object.run [as _onImmediate]  <node_modules/intern/node_modules/dojo/Promise.ts:237:7>
  at processImmediate [as _immediateCallback]  <timers.js:363:15>
  at Command.then  <node_modules/intern/node_modules/leadfoot/Command.js:542:10>
  at Command  <node_modules/intern/node_modules/leadfoot/Command.js:566:15>
  at <node_modules/intern/lib/Test.js:236:37>
  at Promise.cancel  <node_modules/intern/node_modules/dojo/Promise.ts:343:37>
  at null._onTimeout  <node_modules/intern/lib/Test.js:164:21>
  at Timer.listOnTimeout [as ontimeout]  <timers.js:121:15>
@zaach
Copy link
Contributor Author

@zaach zaach commented Sep 29, 2015

@shane-tomlinson I can't reproduce– have you made sure to rebuild the SASS files?

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 30, 2015

@shane-tomlinson I can't reproduce– have you made sure to rebuild the SASS files?

I'll re-check to make sure, I'd guess this as the most likely cause of the issue.

@shane-tomlinson
Copy link
Member

@shane-tomlinson shane-tomlinson commented Sep 30, 2015

@zaach - I had two problems, the first is I needed to regenerate the SASS, the second is I had a fake-basket-server that intentionally introduced delay in responding.

shane-tomlinson pushed a commit that referenced this pull request Sep 30, 2015
…king

fix(settings): unblock rendering of settings on communication prefs screen

r=@shane-tomlinson

Thanks @zaach!
@shane-tomlinson shane-tomlinson merged commit 2a6afb7 into master Sep 30, 2015
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0009%) to 98.897%
Details
@shane-tomlinson shane-tomlinson deleted the issue-3061-email-prefs-non-blocking branch Sep 30, 2015
shane-tomlinson pushed a commit that referenced this pull request Jan 11, 2016
PR #3088 sped up the rendering of the settings page but introduced
a regression in the communication preferences tests. Communication
preferences are re-rendered after an XHR request to Basket is made
to check whether the user has opted in to email communcations. The
Selenium tests moved to quickly and clicked the "open panel" button
using an element that was sometimes no longer in the DOM (sometimes meaning
sometimes the response to basket took a while and the prefs opened,
other times basket finished early and Selenium's reference was stale).

This PR fixes this by adding a `basket-ready` class to the
`#communication-preferences` element when rendering is complete.
Selenium looks for this class name, and then continues.

fixes #3357
shane-tomlinson pushed a commit that referenced this pull request Jan 11, 2016
PR #3088 sped up the rendering of the settings page but introduced
a regression in the communication preferences tests. Communication
preferences are re-rendered after an XHR request to Basket is made
to check whether the user has opted in to email communcations. The
Selenium tests moved to quickly and clicked the "open panel" button
using an element that was sometimes no longer in the DOM (sometimes meaning
sometimes the response to basket took a while and the prefs opened,
other times basket finished early and Selenium's reference was stale).

This PR fixes this by adding a `basket-ready` class to the
`#communication-preferences` element when rendering is complete.
Selenium looks for this class name, and then continues.

fixes #3357
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants