848260 - [FTU] When downloading system update, canceling it and redownload it, the warning won't show up

Status: RESOLVED FIXED

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

Description

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

Using Testrun5.1 build in Unagi

STR:
 1. Connect to Wifi/Data/Roaming
 2. Update OTA
 3. Cancel it before download finished

Expected result:
 go to OTA notification and update it, it should still show warning dialog.

Real result:
 it shows no warning dialog.

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Forgive me, what does the 'warning dialog' say?

Comment 2

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

I forgot to describe it in details.
In the UX document, it say that it should should a warning before you started to download it.

https://www.dropbox.com/s/9ygfskg9dlfsb2q/Roaming%20UI_Feb%2004.pdf
MTR-29340 screen 2

Comment 3

[Michael Vines [:m1] [:evilmachines]]

"When users download software updates via data connection for the first time, they are warned of potential data charges using a prompt as shown here " -- I read the bug description as the warning does not appear the 2nd time that OTA download occurs, and if so then seems to be working as specified?

Comment 4

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

IT's still "1st time" since I cancelled it on the way downloading?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

Did the warning appear before you canceled it the first time?

Comment 6

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

Yes, the warning did come out before I cancel it.
But, it's not about the warning before cancelling it.

And, sorry for the confusion
Corrected my STR:

STR:
 1. Connect to Wifi/Data/Roaming
 2. Download OTA
 3. Cancel it before download finished
 4. Redownload OTA

It's about the warning when you try to download the OTA for the 1st time.

Comment 7

[Michael Vines [:m1] [:evilmachines]]

Thanks, so the warning appeared at #2 and this bug is about the warning not reappearing at #4 as well?

Comment 8

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

yes, that's correct description of what that is. And, sorry again that I must switch to do something else that I didn't complete my STR.

Comment 9

[Michael Vines [:m1] [:evilmachines]]

No worries.  I don't think this is blocking tef because the warning technically already did appear once so the user is aware of the potential charges even though they cancelled the first download.

Comment 10

[Arthur Chen [:arthurcc][inactive after 6/30]]

Arun, could you help comment on this? In the case of users have seen the warning dialog but not finished the download, should we display the warning dialog when they try to download again?

Comment 11

[Arun Balachandran Ganesan [:abc]]

Sorry for the delay, Arthur. I'm on this now.

Cheers,
Arun

Comment 12

[Arun Balachandran Ganesan [:abc]]

Arthur: 

Walter's comment is spot on. The warning should still appear as it's still the first time until an update is fully completed (by when you know that the user's intent to make a download over data connection is clear.)

Ping me if you need any clarifications.

Cheers,
Arun

Comment 13

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

this still didn't get fixed in v110 and v101.
Any news updates on this?

Comment 14

[Arthur Chen [:arthurcc][inactive after 6/30]]

Sorry for ignoring the bug. 

It seems related to bug 842230. In comment 5 of it Neo suggested that we should display the warning no matter users have downloaded an update or not. Arun, could you give a final conclusion of it? Thanks!

Comment 15

[Arun Balachandran Ganesan [:abc]]

Hi Arthur:

I completely agree with Neo – I think that has to be taken care of in addition to what Walter has said. 

The warning has to appear every time after a 

• SIM change
• System crash
• System reboot


We need to let the user know until he makes a successful first download, while eliminating anything that would mean that the user might have changed (people hand-down cellphones to relatives in our demographics – source: user research) or the SIM plan might have changed (people user multiple SIM cards which have different carriers and thus, different SIM plans – source: user research) 

Thanks!

Cheers,
Arun

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

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

Etienne, based on the UX designer's feedback, a warning should be displayed every time when users try to download via data connection until they successfully download an update. If the user reboots the phone, the warning should be displayed again in the same scenario. 

I used a flag stored in async storage before to record whether the warning is displayed or not. Since this flag is cleared every time users reboot the phone, I use a ordinary variable instead. To get the timing of an update gets downloaded, I refactored some code in updatable.js. Details please refer to the pull request, thanks!

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 741735 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/9400

The UpdateManager should hold code common to system and apps update. Moving code from the various Updatables into the UpdateManager with | if (updatable instanceof...| really defeats the purpose.

It looks like for this bug you want to know if the user had a first batch of successful updates (by batch I mean that the user saw "X updates available" in the prompt and successfully downloaded them). A good place to check this might be in |UpdateManager.removeFromUpdatesQueue| when the count goes back to 0.

Also please make sure to cover your change with tests (and not break the existing ones), this part of the code has a pretty good coverage, we want to keep it this way.

Thanks!

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Thanks for reviewing!

It would be simple to do the check in |UpdateManager.removeFromUpdatesQueue|. However, the function is also called in the case of download failures. Thus a function in UpdateManager is still needed for being called when download success.

Comment 19

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 741735 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/9400

Patch updated. I added a function in UpdateManager, and which will be called at the time when an updatable is downloaded. I'll add corresponding test cases if the change gets a r+.

Comment 20

[Etienne Segonzac (:etienne)]

Comment on attachment 741735 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/9400

Looks much better thanks!

Added some small naming comments on github, ping me when the tests are done for the final review! (or if you need any help with the tests).

Comment 21

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 741735 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/9400

Etienne, I've addressed your comments and added test cases. Please help review it, thanks!

Comment 22

[Etienne Segonzac (:etienne)]

Comment on attachment 741735 [details]
Link to https://github.com/mozilla-b2g/gaia/pull/9400

awesome, r=me with the last tiny test added (see github)

Thanks!

Comment 23

[Arthur Chen [:arthurcc][inactive after 6/30]]

I've added a test case but not sure if it is what you meant. Please check github, thanks!

Comment 25

[Etienne Segonzac (:etienne)]

(In reply to Arthur Chen [:arthurcc] from comment #23)
> I've added a test case but not sure if it is what you meant. Please check
> github, thanks!

Yep looking good!

Comment 26

[Arthur Chen [:arthurcc][inactive after 6/30]]

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