824930 - [FTU][Firefox Private Choices] The option to introduce an email is enabled even though there is not internet connection

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::First Time Experience, defect)

Description

[isabelrios]

Unagi Test Run 1
Gaia 16006ea
Geck cb6ee76

PROCEDURE
 1. Follow the set up process
 2. Do not enable data or connect to Wi-Fi
 3. On Firefox Security Choices try to enter your email

EXPECTED
 If there is no connection, the possibility to enter an e-mail is dimmed and a message explaining that network connection is required to use this feature is shown.

ACTUAL
 There is no difference on this screen when there is or isn't internet connection. The field to insert the email is always active

Comment 1

[David Scravaglieri [:scravag]]

We would take a patch for this.

Comment 2

[Dylan Oliver [:doliver]]

Can you comment on the user impact here? Does any error message result if they try to proceed with the email or does it fail silently and go to the next screen?

Comment 3

[Victoria Gerchinhoren [:vicky]]

My suggestion is to just don't have that screen if you don't have connection as it is useless to the user.

Comment 4

[Fernando Campo (:fcampo)]

(In reply to Dylan Oliver [:doliver] from comment #2)
> Can you comment on the user impact here? Does any error message result if
> they try to proceed with the email or does it fail silently and go to the
> next screen?
 If we try to press the Send button (now text is Done, but this will change), an error appears saying that connection is needed. If we press Next on navigation, the email is not send to store.

Comment 5

[amiller]

UCID: owd-13771
https://moztrap.mozilla.org/results/case/63631/

Comment 6

[Casey Yee]

I don't think skipping the screen because there is no connection is the right solution here.   

I recall Larissa mentioning that in a no connection scenario we would be storing the email for later submission.

Flagging Larissa for more info.

Comment 7

[Victoria Gerchinhoren [:vicky]]

Well, we can also disable the imput field and put a very short message underneath it saying that you need connection. But's a bit odd, the whole purpose of the screen is that you provide your email and cannot imagine users going 3 steps back to connect, right?

Comment 8

[Casey Yee]

My point is that the user shouldn't be prevented from entering a email address because of availability of connectivity.   I'm fairly certain that collecting emails is important enough that we should have some way of storing the email for delivery later when a connection is available.

Comment 9

[Larissa Co [:lco]]

Agree with Casey. We should store the email address locally until the user connects to the Internet, then we send it in the background. I thought this was the established plan?

Adding Vivien, who I believe was working on this screen (sorry if you're not the right person).

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Larissa Co from comment #9)
> Agree with Casey. We should store the email address locally until the user
> connects to the Internet, then we send it in the background. I thought this
> was the established plan?
> 
> Adding Vivien, who I believe was working on this screen (sorry if you're not
> the right person).

It is possible to store the email locally since the FTU is part of the communications app. This means the email could be sent to the server if the user has agreed to do so when the dialer or the contacts application is opened.

Comment 11

[Fernando Campo (:fcampo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #10)
> 
> It is possible to store the email locally since the FTU is part of the
> communications app. This means the email could be sent to the server if the
> user has agreed to do so when the dialer or the contacts application is
> opened.

Some concerns, just speaking my mind out loud:

Do we have any preferred way of storing it? localStorage is enough?
Do we want to modify both apps (dialer, contacts) apart from ftu for doing so? does it worth it?
Is the event 'online' working fine on FfoxOS? I was thinking on adding a listener for it to send the email, but unsure if it have WiFi into consideration.

Comment 12

[Josh Carpenter [:jcarpenter]]

It sounds like the behavior currently in place is what we want. In that case can we close this bug?

Comment 13

[Fernando Campo (:fcampo)]

(In reply to Josh Carpenter [:jcarpenter] from comment #12)
> It sounds like the behavior currently in place is what we want. In that case
> can we close this bug?
Sorry josh, but behaviour is not what we want, apparently. 
Currently we send it if there's connection, and show an error if there's not.
Seems on the comments that the desired behaviour would be store the email when there's no connection, and send it later. I started working on a patch, but would be better if we clear my questions above before submitting it.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fernando Campo (:fcampo) from comment #11)
> (In reply to Vivien Nicolas (:vingtetun) from comment #10)
> > 
> > It is possible to store the email locally since the FTU is part of the
> > communications app. This means the email could be sent to the server if the
> > user has agreed to do so when the dialer or the contacts application is
> > opened.
> 
> Some concerns, just speaking my mind out loud:
> 
> Do we have any preferred way of storing it? localStorage is enough?

I would say use the asyncStorage helper provided by the shared/js/async_storage.js file.

> Do we want to modify both apps (dialer, contacts) apart from ftu for doing
> so? does it worth it?

If the user spend some times feeling the email so it worth it :)

> Is the event 'online' working fine on FfoxOS? I was thinking on adding a
> listener for it to send the email, but unsure if it have WiFi into
> consideration.

It 'works' fine except that captive portal may trick it as far as i can tell but this should go away with the captive portal code living in the shira branch. So don't worry about that and use the online event - if there are any other bugs related to it we can fix it in the platform side. Don't forget to open the bug in that case :)

Comment 15

[Fernando Campo (:fcampo)]

Attachment: Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

I recommend to test with an user build, as onLine events are not working properly on the engineering builds.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

r-, the string change shouldn't be done. To quote my inline comment in the PR:

You already have the '.' after the </a> here, and I think that's where it should be. There's a different problem in that the post-link text isn't localizable, though. No idea how/if the l10n lib can do that.

Comment 17

[Fernando Campo (:fcampo)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

Thanks for the feedback :pike, I backed out the changes to strings, so I'm putting u out of the review as there's no need of it anymore :)
Do you know if there's an open bug about the post-link text or should we fill a new one? 


Also adding german to it for the changes on dialer

Comment 18

[Axel Hecht [:Pike]]

I don't know of an existing bug, but bug 820344 is related in concept. Mind filing a new bug, with stas and me on CC on the l12y keyword?

Comment 19

[Fernando Campo (:fcampo)]

Done, bug 851111

Comment 20

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks,

I'm not quite happy with the solution of doing this on the dialer at all. 

We are fighting for removing 80ms from the dialer startup and this (despite that we can remove it from the critical path) looks to me that we are adding a bit extra of complexity.

Could it be possible to change the strategy to an alarm to the FTU?

Comment 21

[Fernando Campo (:fcampo)]

For the alarms, I remember Jose Cantera had some issues with it, so I'm adding him to the loop for a better feedback.

Adding it to the Dialer was something him and Vivien agreed, so maybe they can explain the reasons better.

Certainly, adding a AsyncStorage check every time we start the dialer is an issue, even if we lazy load the js involved on it.

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Waiting on some decision to be made regarding the alarm issue before reviewing it ;-)

Comment 23

[Fernando Campo (:fcampo)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

Changing the reviewers as Vivien is on holidays. Etienne is taking a look, so he knows the dialer, German is probably not needed anymore ;).

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

Here is an approach we could take to avoid harming performances:

- first let's put the |NewsletterManager| in its own file, no reason to have it in dialer.js

- we add _nothing_ to the dialer startup path (not event the newly created newsletter_manager.js)

- we use an idleObserver to load the newsletter_manager.js after let's say 5 seconds of inactivity. (an example of idle api use can be found here: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/connectivity.js#L300-307)

- the newsletter_manager can lazy load async_storage.js

Also, some general notes:
- be sure to make lint, the PR had linting issues and thanks to Travis we won't miss it
- you can use |APP=communications/dialer make test-perf| to do on-device launch performance test and make sure we don't regress
- you should check |navigator.onLine| before adding the "online" event listener

Comment 25

[Fernando Campo (:fcampo)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

:etienne Made the suggested changes.

Perf tests remain more or less the same (differences of 2-6ms, higher and lower), so I don't think there was any regression.

Comment 26

[Etienne Segonzac (:etienne)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

r=me with the comments on github addressed.
Thanks for taking the time to do this without hurting performances!

Comment 27

[Andrew Overholt [:overholt]]

This blocks (or perhaps duplicates) bug 845964 which is tef+ so setting tef?.

Comment 28

[Fernando Campo (:fcampo)]

Merged https://github.com/mozilla-b2g/gaia/commit/d95bae7da6fb12a491423cd754429581b184ee64

Comment 29

[Fernando Campo (:fcampo)]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

User impact if declined: 
- Inconsistent UI with same look buttons doing different things (one sends email, other goes to next screen)
- Errors on connection status checks as relying on workarounds instead of using navigator.onLine
- User have no way of subscribe to newsletter when no connection

Testing completed: 
- performance test on dialer remains around same values

Risk to taking this patch (and alternatives if risky): 
- medium-low risk, as we modify behaviour of dialer also, but performance tests made show no increment on time with the new patch

String or UUID changes made by this patch:

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 724420 [details]
Link to PR https://github.com/mozilla-b2g/gaia/pull/8622

Approving for v1.1, while we gather consensus on whether or not this is tef+

Comment 31

[Daniel Coloma:dcoloma]

(In reply to Andrew Overholt [:overholt] from comment #27)
> This blocks (or perhaps duplicates) bug 845964 which is tef+ so setting tef?.
Blocking on this and marking bug 845964 as a duplicate

Comment 33

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: 676b3d41a1d8b8545400dde1aa78dfe0ea9c5d78

Comment 34

[Daniel Coloma:dcoloma]

John, I've seen that you have marked this as fixed in v1.0.1 but it is only uplifted to v1-train (not to v1.0.1). Can you please uplift it to v1.0.1 too?

Please note, that when doing so it would be great to uplift bug 854424 at the same time, as it fixes a regression introduced by this bug.

Comment 35

[mlevin]

Tested on the following v101 Unagi:

Build ID: 20130326070208
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe
Gaia: 5c3583a7a58f0c47da0e48b90273fa36cc508b0b

The message "You must be connected to the internet to subscribe to the newsletter. Please go back and connect to the internet" is received after tapping "Done" after typing in the e-mail address in the field.

Is this expected behavior?




Is this the expected behavior?

Comment 36

[John Ford [:jhford] CET/CEST Berlin Time]

(In reply to Daniel Coloma:dcoloma from comment #34)
> John, I've seen that you have marked this as fixed in v1.0.1 but it is only
> uplifted to v1-train (not to v1.0.1). Can you please uplift it to v1.0.1 too?
> 
> Please note, that when doing so it would be great to uplift bug 854424 at
> the same time, as it fixes a regression introduced by this bug.

This patch only applied cleanly to v1-train.  There is a merge conflict in "apps/communications/ftu/js/navigation.js" on v1.0.1.  The basic steps which could be used to resolve the conflict are:

  cd gaia
  git checkout v1.0.1
  git cherry-pick 676b3d41a1d8b8545400dde1aa78dfe0ea9c5d78
  <resolve conflict>
  git add apps/communications/ftu/js/navigation.js
  git commit

The conflict doesn't look like it'd be too difficult to resolve:

++<<<<<<< HEAD
 +        if (WifiManager.api.connection.status === 'connected' ||
 +            DataMobile.isDataAvailable) {
 +          fbOption.classList.remove('disabled');
++=======
+         if (window.navigator.onLine) {
+           fbState = 'enabled';
++>>>>>>> 676b3d4... Merge pull request #8622 from fcampo/emailConnection

Comment 37

[Fernando Campo (:fcampo)]

Done :)

PR in https://github.com/mozilla-b2g/gaia/pull/8848

Unsure if I need approval from someone in here, :jhford?

Comment 38

[Fernando Campo (:fcampo)]

After asking around, decided to do it myself, so merged on https://github.com/mozilla-b2g/gaia/commit/45780e85c66a0d73cbac94cf5ef307dd6a0e0158

Comment 39

[mdaniloff]

Verified fixed on Unagi. Setting up email with no internet and trying to log in  will display notification that an internet connection is required.

Unagi Build ID: 20130328070202
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c516d7e67150
Gaia: d40dcdd112f12e2a5a0d1de46451670918fd4369