837193 - At update, after a downloaderror, we should check the downloadAvailable property

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

The Update Manager assumes that if there was a download available, and there is an error, then a download is still available. So we're not removing the notification and we're not even checking this property again.

We should.

Also, I've seen that the update manager merely checks for "installState === 'installed'", however an installed app can also have a state 'updating'. The actual test should be "installState !== 'pending'" instead. It would be nice that we fix this in the process.

I'm nominating TEF because we saw in Bug 836909 that after some errors we don't have a downloadavailable any more, and so we should remove the app from the notification. So this is visible by the user.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Not clear that this should block, can QA confirm what workarounds exist here and what the risk is to the user regarding being offered downloads?  If this is just a notification sticking around and doesn't prevent later offerings of downloads we won't block here.

Comment 2

[Julien Wajsberg [:julienw]]

Bug 836909 landed a gecko check so now there won't be any problem if the user tries to download it again.

This is just a notification staying there until the phone is rebooted, and later updates will still be displayed.

So I agree with not blocking on this. However, if this is fixed by a patch, it would be nice to have this uplifted.

Comment 3

[Jason Smith [:jsmith]]

In that case, I would suggest tracking this.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: patch v1

see also https://github.com/mozilla-b2g/gaia/pull/9026 (note: the PR contains also the commit for Bug 858118).

If downloadAvailable is false after an error, it means the error is
unrecoverable and we don't want to bug the user until the next update check.
---
 apps/system/js/updatable.js             |    6 +++++-
 apps/system/test/unit/mock_app.js       |    1 -
 apps/system/test/unit/updatable_test.js |   12 ++++++++++++
 3 files changed, 17 insertions(+), 2 deletions(-)

Comment 5

[Etienne Segonzac (:etienne)]

Comment on attachment 734126 [details] [diff] [review]
patch v1

Review of attachment 734126 [details] [diff] [review]:
-----------------------------------------------------------------

If possible let's land the PR re-enabling the system tests in travis before sending this one :)

Comment 6

[Julien Wajsberg [:julienw]]

master: 76516f9fe75640875a2a71607a83a59287fdd004

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 734126 [details] [diff] [review]
patch v1

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: When a unrecoverable errors happen, the update notification should be removed so that the user is not bugged anymore until the next update cycle. Without this patch the update notification would stay forever.
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low, small change and this code is heavily unit tested.
String or UUID changes made by this patch: none

Comment 8

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

Uplifted commit 76516f9fe75640875a2a71607a83a59287fdd004 as:
v1-train: 3c402b7b14dfbd71498847bf5126fadc608f0eae