929311 - Should provide more suitable description while user want to download the updates in roaming area.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: screen-1.png

For currently design, user will be informed it will have additional charges when using data connection, please see screen-1. 
But the description could not let user know the device is in roaming status clearly, and I think user will not know the device is in roaming status even it has roaming sign in title bar.So it will be charged a lot of money in case user click download to download the files in roaming area.

I suggest the warning message should inform user that device is in roaming status while user click download in notification page.

Comment 1

[Hubert Lu[:hlu] <[e-mail]>]

Hi Casey,
   The currently SPEC in my hand is created by you, could you please kindly provide your opinion?

   [SPEC] https://www.dropbox.com/s/9ygfskg9dlfsb2q/Roaming%20UI_Feb%2004.pdf


BR,
Hubert

Comment 2

[GoodMike]

I think Hubert has the concern to clearly inform users on the possibility of being charge a huge amount of money if FOTA in data roaming status. And then suggest or encourage users to do that with Wi-Fi connection. Rephrasing the wordings in this way and still keep "do it later" or "download" options for them to choose.  (p.s. some device like HTC phone doesn't allow users to do FOTA in data roaming mode to prevent from having sever customer complaint.)

Comment 3

[Stephany Wilkes]

Flagging Jenny to confirm that the copy and message are OK in Hubert's spec. Hubert, I am very sorry that Casey did not get to this and apologize for the delay on our part!

Comment 4

[Jenny Lee]

Hello, the revised message is as follow:

(downloading update via data connection when roaming)
You are currently roaming. International data roaming might cause additional charges, do you want to download update via data connection? 

(downloading update via data connection but no roaming)
When using data connection, additional charges may apply. Do you want to download update via data connection? 

ni Matej to check the string. Hello Matej, could you please take a look at the sting and revise if needed? Thanks!

Comment 5

[Matej Novak [:matej]]

Hi all. I think we could really simplify this to the core of the messages. Here's what I'm thinking:

(downloading update via data connection when roaming)
You are currently roaming and additional charges may apply. Do you want to continue?

(downloading update via data connection but no roaming)
When using a data connection, additional charges may apply. Do you want to continue?

Comment 6

[Manuel Casas Barrado [:mancas]]

Attachment: New description while user want to download the updates in roaming area

Comment 7

[Matej Novak [:matej]]

(In reply to Manuel Casas Barrado [:mancas] from comment #6)
> Created attachment 8460823 [details] [review]
> New description while user want to download the updates in roaming area

I don't think I'm the right person to review this. At least I don't usually review things on GitHub.

Comment 8

[Stephany Wilkes]

UX does string reviews in patches. Jenny, please load the patch and check that your recommended strings look OK. If Matej is OK with the copy you've put in this bug, that should get us far enough. 

Matej, if you're flashing a Firefox OS device regularly and it might be helpful to you to view patches on device, let me know and someone on our team can demo how we do that.

Thanks all!

Comment 9

[Matej Novak [:matej]]

Could someone clear the review flag for me on this? I'm not sure how to do that. Thanks.

Comment 10

[Walter Chen[:ypwalter][:wachen]]

Comment on attachment 8460823 [details] [review]
New description while user want to download the updates in roaming area

removed r? tag. 
However, I am redirecting it to Firefox OS UX team again for reviewers.

Comment 11

[Jenny Lee]

Hi Manuel, I think it looks good, thanks!
Just one thing, for the roaming case, can you change the title to "Download updates via data roaming"?
Sorry that I didn't specify the strings for title in advance.

Comment 12

[Manuel Casas Barrado [:mancas]]

(In reply to Jenny Lee from comment #11)
> Hi Manuel, I think it looks good, thanks!
> Just one thing, for the roaming case, can you change the title to "Download
> updates via data roaming"?
> Sorry that I didn't specify the strings for title in advance.

Please take a look at the new commit. Thanks

Comment 13

[Jenny Lee]

Looks great! Thank you Manuel ;)

Comment 14

[Stephany Wilkes]

What is this review? flag for? Jenny already approved this. Please clarify the need for the review?. Thanks!

Comment 15

[Manuel Casas Barrado [:mancas]]

(In reply to Stephany Wilkes from comment #14)
> What is this review? flag for? Jenny already approved this. Please clarify
> the need for the review?. Thanks!

Ups, sorry, I've selected the wrong reviewer. I need Jenny give me r+ in order to merge

Comment 16

[Jenny Lee]

Comment on attachment 8460823 [details] [review]
New description while user want to download the updates in roaming area

Hi Manuel, I've already approved this. Did you make any change?

Comment 17

[Manuel Casas Barrado [:mancas]]

(In reply to Jenny Lee from comment #16)
> Comment on attachment 8460823 [details] [review]
> New description while user want to download the updates in roaming area
> 
> Hi Manuel, I've already approved this. Did you make any change?

Just fixed unit test and linter. Thanks!

Comment 18

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/c0a8cebe31c49a90c443226710aef36c1e62bdc2

Comment 19

[Francesco Lodolo [:flod]]

Can we please back-out and land a proper patch? It's not OK to change existing strings like you did (downloadUpdatesViaDataConnectionMessage)

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_best_practices#Changing_existing_strings

Comment 20

[Francesco Lodolo [:flod]]

Not sure if I'm confused by the history of this bug, but has this pull request ever been reviewed in terms of code? Because I see only strings and UX reviews, and no comments on the pull request.

Comment 21

[Stephany Wilkes]

Comment on attachment 8460823 [details] [review]
New description while user want to download the updates in roaming area

Jenny is a UX designer and as such cannot take the review? flag. The review? flag is only for developers; the ui-review? flag is only for the UX team. 

Flagging Tim on the review? flag (Tim, please reassign to someone else as necessary) and setting ui-review? to Jenny.

Comment 22

[Stephany Wilkes]

Also, Jenny, please clear your review+ flag if possible.

Comment 23

[Francesco Lodolo [:flod]]

Can I ping a sheriff to back this out in the meantime?

Comment 24

[Francesco Lodolo [:flod]]

(In reply to Francesco Lodolo [:flod] from comment #23)
> Can I ping a sheriff to back this out in the meantime?

(to clarify IRC discussion) The reason for requesting a back-out is explained in comment 19: this PR changes an existing string without assigning a NEW id. So, it's either a back-out or a follow-up to fix the string. Given the doubt in comment 20, I think back-out is the reasonable choice here.

Comment 25

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

master revert: 34df8238f7367ac99b6897a073b9cc2f82f5371a

Comment 26

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

Comment on attachment 8460823 [details] [review]
New description while user want to download the updates in roaming area

Please submit a new pull request and ask review from :etienne.

Comment 27

[Manuel Casas Barrado [:mancas]]

Attachment: Updated l10n ID

Comment 28

[Etienne Segonzac (:etienne)]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

Looking good.

Some comments on github, please ask me for review again once they're done since the tests will probably change a bit.

Comment 29

[Etienne Segonzac (:etienne)]

More general UX question: having the |ril.data.roaming_enabled| set to true doesn't actually mean we're roaming (AFAIK), but that we *would* roam if in another country.

Maybe we should tweak the message accordingly or better, use mozMobileConnection to figure out if we're actually roaming currently.

Comment 30

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

Please check the new commit. I've also added a second check with mozMobileConnections

Comment 31

[Etienne Segonzac (:etienne)]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

While we figure out the last issues on the tests, can you upload screenshots of the results and set the ui-review? flag on them? Thanks!

Comment 32

[Manuel Casas Barrado [:mancas]]

Attachment: roaming.png

Etienne, please take a look at comments in PR. Thanks!

Comment 33

[Jenny Lee]

Comment on attachment 8475012 [details]
roaming.png

Looks good, thanks!

Comment 34

[Etienne Segonzac (:etienne)]

(In reply to Manuel Casas Barrado [:mancas] from comment #32)
> Created attachment 8475012 [details]
> roaming.png
> 
> Etienne, please take a look at comments in PR. Thanks!

Sorry, my comment wasn't clear enough and testing code with promises is always a challenge like this long thread shows [1] (it's a very interesting read, I recommend it).

Our case here is rather tricky since it makes no sense for |requestDownloads| to return a promise (it's an event handler).

I think the best compromise and the solution that will give use the best code coverage is the following:
- use the proper mozSetting mock
- spy on |_getDataRoamingSetting| to access the returned promise
- use this promise in the test

Here's a gist to make things clearer [2], I just tested it and it works well.

Cheers!

[1] https://groups.google.com/forum/#!topic/mozilla.dev.gaia/yn7waOIfKFQ
[2] https://gist.github.com/etiennesegonzac/aa42670ebeaf33c39680

Comment 35

[Manuel Casas Barrado [:mancas]]

(In reply to Etienne Segonzac (:etienne) from comment #34)
> (In reply to Manuel Casas Barrado [:mancas] from comment #32)
> > Created attachment 8475012 [details]
> > roaming.png
> > 
> > Etienne, please take a look at comments in PR. Thanks!
> 
> Sorry, my comment wasn't clear enough and testing code with promises is
> always a challenge like this long thread shows [1] (it's a very interesting
> read, I recommend it).
> 
> Our case here is rather tricky since it makes no sense for
> |requestDownloads| to return a promise (it's an event handler).
> 
> I think the best compromise and the solution that will give use the best
> code coverage is the following:
> - use the proper mozSetting mock
> - spy on |_getDataRoamingSetting| to access the returned promise
> - use this promise in the test
> 
> Here's a gist to make things clearer [2], I just tested it and it works well.
> 
> Cheers!
> 
> [1] https://groups.google.com/forum/#!topic/mozilla.dev.gaia/yn7waOIfKFQ
> [2] https://gist.github.com/etiennesegonzac/aa42670ebeaf33c39680

Thanks a lot! Now I see things much more clearer. I've implemented the async tests and now everything is working correctly. Thanks again for this useful lesson!

Comment 36

[Etienne Segonzac (:etienne)]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

r=me with the 2 small comments on github addressed.
Thanks!

Comment 37

[Manuel Casas Barrado [:mancas]]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

Everything should be ok now. Thanks!

Comment 38

[Etienne Segonzac (:etienne)]

Comment on attachment 8465227 [details] [review]
Updated l10n ID

Thank you, great patch :)

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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