1039501 - [User Story] [FOTA] WiFi prioritized for downloading

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Marcelino Veiga Tuimil [:sonmarce]]

See description of User Story field

Comment 1

[Maria Angeles Oteo (:oteo)]

Hi Bruce, can you review the AC of this US? Thanks a lot

Comment 2

[Bruce Huang [:bhuang] (inactive)]

Looks good to me, we should probably also add the case where downloaded is started and the device moves to 3G.  I'm guessing we trigger something similar to AC3.

Comment 3

[Manuel Casas Barrado [:mancas]]

Attachment: Wifi prioritized

This patch is a first approach. We still waiting for UX

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

After looking at the patch I think it's non-trivial so it's better we ask for the dedicated peer for review, unless Etienne have hands over that part of system app to someone else ...

I also think we should get the UX review done first before code review, since any behavior change will definitely introduce some code change.

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

We're still waiting for the UX so this is just general feedback.

But can we keep the logic in the UpdateManager (as opposed to the SystemUpdatable)?

Comment 6

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Hi Jenny, I upload here the ux spec for the "WiFi prioritized for downloading" and "Not allow updates via 2G" bugs. Could you please take a look to it and correct me if I have missed something?

On the other hand, Matej, could you please check the wording please?

Thanks!

Comment 7

[Matej Novak [:matej]]

Here are my revisions and comments:

> Unable to start the download. Please, connect via Wi-Fi or 3G and try to download it again.

Small changes here:

Unable to start download. Please connect via Wi-Fi or 3G and try again.


> You are currently connected via 3G. Do you want to switch to Wi-Fi to start the download?

This one looks good.


> Required system update (%SizeUnit) available. Download now?

It seems odd to tell users that this is a required update, but then still give them the option. "Required" makes me think my phone won't work unless I do it. If that's not the case, can we remove that word?


> By using 3G, you might incur in additional costs. Do you want to continue?

Change to:

When using 3G, additional costs may apply. Do you want to continue?

Comment 8

[Pau Masiá [:Pau]]

(In reply to Matej Novak [:matej] from comment #7)

> > Required system update (%SizeUnit) available. Download now?
 
> It seems odd to tell users that this is a required update, but then still
> give them the option. "Required" makes me think my phone won't work unless I
> do it. If that's not the case, can we remove that word?

Jenny, I took it from your ux spec. What do you think about that?

Comment 9

[Jenny Lee]

Attachment: data connection warning.png

Hi all, just want to clarify before moving on to review the spec, as far as I know, we are already showing warning message whenever user is trying to download update via data connection (see attached), so maybe what we need here is the flexibility to set "WiFi prioritized" on or off? When set to on, simply follow the current behavior, when set to off, don't show warning message.

Comment 10

[Pau Masiá [:Pau]]

Now I know there's a message that covers that, I'm ok with it. I'll make the changes on the spec to match that, then.

On the other hand, what about the wording regarding the "Download confirm" string Matej said in comment#7?

Comment 11

[Jenny Lee]

(In reply to Pau Masiá [:Pau] from comment #10)
> Now I know there's a message that covers that, I'm ok with it. I'll make the
> changes on the spec to match that, then.
> 
Does that mean you will also follow the current behavior: show warning dialog (as attached in comment 9) whenever user is trying to download update via data connection (3G, 2G etc)? As far as I know, user can still download update via 2G, so the condition for dialog on p.6 should be "when the device is not able to access network". 
Also, the follow up action that allows user to switch to Wi-Fi seems unnecessary to me. On p.7, when user taps on "yes (switch to Wi-Fi)", what happens next? It is not possible to ensure the device can automatically connect to Wi-Fi successfully every time. Ex. user can be at a new environment that he has to set up a connection from scratch manually. I'd say we can simply warn user about data connection is currently in use, and stop there (like our current behavior). Hope that makes sense :)       

> On the other hand, what about the wording regarding the "Download confirm"
> string Matej said in comment#7?

Yes I'm ok with removing "Required". Thanks!

Comment 12

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Comment 13

[Pau Masiá [:Pau]]

(In reply to Jenny Lee from comment #11)
> (In reply to Pau Masiá [:Pau] from comment #10)
> > Now I know there's a message that covers that, I'm ok with it. I'll make the
> > changes on the spec to match that, then.
> > 
> Does that mean you will also follow the current behavior: show warning
> dialog (as attached in comment 9) whenever user is trying to download update
> via data connection (3G, 2G etc)? As far as I know, user can still download
> update via 2G, so the condition for dialog on p.6 should be "when the device
> is not able to access network". 
> Also, the follow up action that allows user to switch to Wi-Fi seems
> unnecessary to me. On p.7, when user taps on "yes (switch to Wi-Fi)", what
> happens next? It is not possible to ensure the device can automatically
> connect to Wi-Fi successfully every time. Ex. user can be at a new
> environment that he has to set up a connection from scratch manually. I'd
> say we can simply warn user about data connection is currently in use, and
> stop there (like our current behavior). Hope that makes sense :)       
> 
> > On the other hand, what about the wording regarding the "Download confirm"
> > string Matej said in comment#7?
> 
> Yes I'm ok with removing "Required". Thanks!

Hi Jenny, I think there has been a misunderstanding regarding this issue. What I said was that I would apply the changes to the spec for sake of consistency grouping similar scenarios to make it easier.

That is why I have updated the spec. Take a look at it, please :)

Thanks!

Comment 14

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Sorry, this is the good one :)

Comment 15

[Jenny Lee]

(In reply to Pau Masiá [:Pau] from comment #14)
> Created attachment 8481198 [details]
> UX Spec. OTA. 2G/Wi-Fi
> 
> Sorry, this is the good one :)

Hi Pau,
p.6 - As mentioned in comment 11, user can still download update via 2G, so I'm not sure what is the purpose here. Do you have special requirement that forbids user from downloading via 2G?
p.7, p.8 - are IMO basically the same thing, can consider combining them and change the title. One thing I want to clarify though, in current implementation, the data connection warning dialog will appear *whenever* user tries to download via data connection, no use of parameter to decide whether or not to show the dialog. So, is the parameter something you want to add to current design?
p.9 - Looks good!

Thanks!

Comment 16

[Maria Angeles Oteo (:oteo)]

(In reply to Jenny Lee from comment #15)

> Hi Pau,
> p.6 - As mentioned in comment 11, user can still download update via 2G, so
> I'm not sure what is the purpose here. Do you have special requirement that
> forbids user from downloading via 2G?

Hi Jenny, the requirements for that are included in the Acceptance Criteria of the User Story in bug 1039509,  [User Story] [FOTA] Not allow updates via 2G. This parameter for allowing or not the 2G download is configured by the OEM in build time. Although it's already implemented, I asked Pau to include this corresponding interaction design in the spec so we can have a complete picture of both functionalities.


> p.7, p.8 - are IMO basically the same thing, can consider combining them and
> change the title. One thing I want to clarify though, in current
> implementation, the data connection warning dialog will appear *whenever*
> user tries to download via data connection, no use of parameter to decide
> whether or not to show the dialog. So, is the parameter something you want
> to add to current design?

As I told before the WIFI and 2G parameters are parameters configured in build time so we are including them in these implementations but they can not be accessed and changed directly by the users.
Is it more clear now?

Thanks a lot, Jenny!

Comment 17

[Jenny Lee]

(In reply to Maria Angeles Oteo (:oteo) from comment #16)
> (In reply to Jenny Lee from comment #15)
> 
> > Hi Pau,
> > p.6 - As mentioned in comment 11, user can still download update via 2G, so
> > I'm not sure what is the purpose here. Do you have special requirement that
> > forbids user from downloading via 2G?
> 
> Hi Jenny, the requirements for that are included in the Acceptance Criteria
> of the User Story in bug 1039509,  [User Story] [FOTA] Not allow updates via
> 2G. This parameter for allowing or not the 2G download is configured by the
> OEM in build time. Although it's already implemented, I asked Pau to include
> this corresponding interaction design in the spec so we can have a complete
> picture of both functionalities.
> 
> 
ok! So we have two sets of parameter, one for Wi-Fi prioritize and one for allowing 2G download? In that case there should be 4 outcomes: 
1. Wi-Fi prioritize ON + 2G update ok (covered in spec)
2. Wi-Fi prioritize OFF + 2G update ok (covered in spec)
3. Wi-Fi prioritize ON + 2G update Not ok and device is connected to 2G (needs more specification, first prompt user the Wi-Fi prioritize dialog, if user choose to download, prompt user the 2G not allowed dialog)
4. Wi-Fi prioritize OFF + 2G update Not ok and device is connected to 2G (covered in spec)

Thanks Maria!

Comment 18

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Here I attach the new version of the spec already updated according to the comments above.

Thanks.

Comment 19

[Jenny Lee]

Hi Pau, just one thing, your index is missing two pages. Thanks!

Comment 20

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Little mistake fixed ;)! Thanks.

Comment 21

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

Hey etienne! Take a look at the PR please.

Thanks!

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

Comments on github, sorry to be picky but the logic here is pretty complex so we need to take great care of the readability.

Comment 23

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

I've taken care of your comments in github

Thanks!

Comment 24

[Etienne Segonzac (:etienne)]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

the code change is a-okay! Thanks!
(a few comments on github but nothing blocking).

Please attach screenshots of the new dialogs so they can be ui-reviewed and we might also want to get QA to give this a spin before landing since it's a rather critical part of the system.

Comment 25

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

Hey etienne! I've made some changes to retrieve the value from the update2g setting as you said in github, so take a quick look at the pr.

Thanks!

Comment 26

[Manuel Casas Barrado [:mancas]]

Attachment: wifi_prioritized.png

Comment 27

[Manuel Casas Barrado [:mancas]]

Attachment: roaming.png

Comment 28

[Manuel Casas Barrado [:mancas]]

Attachment: forbidden.png

Comment 29

[Pau Masiá [:Pau]]

Attachment: UX Spec. OTA. 2G/Wi-Fi

Spec updated. Page 9. Thanks!

Comment 30

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #25)
> Comment on attachment 8475029 [details] [review]
> Wifi prioritized
> 
> Hey etienne! I've made some changes to retrieve the value from the update2g
> setting as you said in github, so take a quick look at the pr.
> 
> Thanks!

Made a small comment, but it's already much more readable than before your patch, thank you for that ! :)

Johan, before you get overloaded with a *lot* of new stuffs, could you help us make sure everything looks good QA-wise here? ;)

Comment 31

[Manuel Casas Barrado [:mancas]]

Attachment: wifi_prioritized_using_3g.png

Comment 32

[Manuel Casas Barrado [:mancas]]

Sorry for this but the spec has changed so I've had to make some changes in the code. Please take a look when possible.

Thanks!

Comment 33

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #32)
> Sorry for this but the spec has changed so I've had to make some changes in
> the code. Please take a look when possible.
> 

Looks good, please use the review flag next time I'll get to it quicker :)

Comment 34

[Jenny Lee]

Comment on attachment 8493020 [details]
wifi_prioritized_using_3g.png

Missing a period "." at the end, otherwise looks good, tks

Comment 35

[Jenny Lee]

Comment on attachment 8492897 [details]
forbidden.png

Missing a period "." at the end, otherwise looks good, tks

Comment 36

[Jenny Lee]

Comment on attachment 8492875 [details]
wifi_prioritized.png

Missing a period "." at the end, otherwise looks good, tks

Comment 37

[Pau Masiá [:Pau]]

Comment on attachment 8493020 [details]
wifi_prioritized_using_3g.png

As Jenny says, there's a period "." missing at the end. Otherwise, ui+

Comment 38

[Pau Masiá [:Pau]]

Comment on attachment 8492897 [details]
forbidden.png

Remember the missing period "." at the end.

Comment 39

[Manuel Casas Barrado [:mancas]]

Attachment: forbidden.png

Comment 40

[Manuel Casas Barrado [:mancas]]

Attachment: wifi_prioritized.png

Comment 41

[Manuel Casas Barrado [:mancas]]

Attachment: wifi_prioritized_using_3g.png

Comment 42

[Pau Masiá [:Pau]]

Comment on attachment 8495102 [details]
forbidden.png

The button shape should be blue.

Comment 43

[Manuel Casas Barrado [:mancas]]

Attachment: forbidden.png

Comment 44

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8475029 [details] [review]
Wifi prioritized

Hey Etienne!

This is embarrassing. One of the buttons of the UI should be blue so I've made some changes in the CustomDialog to make it work as expected. Please take a look at the PR since it's a shared script.

Comment 45

[Manuel Casas Barrado [:mancas]]

(In reply to Manuel Casas Barrado [:mancas] from comment #44)
> Comment on attachment 8475029 [details] [review]
> Wifi prioritized
> 
> Hey Etienne!
> 
> This is embarrassing. One of the buttons of the UI should be blue so I've
> made some changes in the CustomDialog to make it work as expected. Please
> take a look at the PR since it's a shared script.

Here are the lines you need to check:
https://github.com/mozilla-b2g/gaia/pull/23025/files#diff-100b2adc2865565a4857427242f33fb9R158

Thanks!

Comment 46

[Johan Lorenzo [:jlorenzo] - On PTO - Back on July 28th]

(In reply to Etienne Segonzac (:etienne) from comment #30)
I'm not sure to have time to focus on new features now. Let's say if I haven't looked at it by Tuesday, I'll transfer the ni to Marcia (the current QA contact for SystemsFE).

Comment 47

[Maria Angeles Oteo (:oteo)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #46)
> (In reply to Etienne Segonzac (:etienne) from comment #30)
> I'm not sure to have time to focus on new features now. Let's say if I
> haven't looked at it by Tuesday, I'll transfer the ni to Marcia (the current
> QA contact for SystemsFE).

Johan, Loli can do the testing of the feature (as this was requested by Telefonica team), we were going to do it anyway as it raised in the certification process.

Comment 48

[Johan Lorenzo [:jlorenzo] - On PTO - Back on July 28th]

(In reply to Maria Angeles Oteo (:oteo) from comment #47)
> Johan, Loli can do the testing of the feature (as this was requested by
> Telefonica team), we were going to do it anyway as it raised in the
> certification process.

Let's ask Loli then.

Comment 49

[Loli (:lolimartinezcr)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #48)
> (In reply to Maria Angeles Oteo (:oteo) from comment #47)
> > Johan, Loli can do the testing of the feature (as this was requested by
> > Telefonica team), we were going to do it anyway as it raised in the
> > certification process.
> 
> Let's ask Loli then.

Yes I test it!

Comment 50

[Manuel Casas Barrado [:mancas]]

Attachment: forbidden.png

Comment 51

[Manuel Casas Barrado [:mancas]]

Attachment: Wifi prioritized

Etienne, the PR has been closed due to tree stability issues as you should know. Please take a quick look at this new PR in order to give it r+.

Thanks!

Comment 52

[Etienne Segonzac (:etienne)]

Comment on attachment 8498708 [details] [review]
Wifi prioritized

for cases like this one where the patch doesn't change but you're just creating a new attachment you can "carry the r+" and mark the new attachment as r+ yourself (mentioning that you're carrying it over)

Comment 53

[(PTO June 19-July 7) Ryan VanderMeulen]

Master: https://github.com/mozilla-b2g/gaia/commit/e58fe34a63a85f51da0d7e35d3b6af290ba0662b

Comment 54

[Loli (:lolimartinezcr)]

I have tested all scenarios with conection 3G, 2G and wifi with 'update.2g.enabled' with true or false and with 'app.update.wifi-prioritized' with true or false values.

I *can't* tested roaming scenarios, because I haven't SIM card with data connection and coverage.

Comment 55

[Juanjo Iglesias]

I have tested it with Flame master (Gecko 437abd2 Gaia a5b7208). 

I have tested with a spanish SiM card (no roaming) and with a german SIM card (roaming scenario), in both scenarios all the cases (3G and 2G connection, wifi or without any data connection) have worked properly.

Comment 56

[Loli (:lolimartinezcr)]

(In reply to Juanjo Iglesias from comment #55)
> I have tested it with Flame master (Gecko 437abd2 Gaia a5b7208). 
> 
> I have tested with a spanish SiM card (no roaming) and with a german SIM
> card (roaming scenario), in both scenarios all the cases (3G and 2G
> connection, wifi or without any data connection) have worked properly.

For this reason, I change status to Verified.