862785 - [system updates] no error message is shown when the update.xml url leads to a 404 error

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

STR:
* have an update URL that is wrong
* check for updates from Settings > Device Information

Expected:
* the system update fails, we should have a message stating that

Actual:
* no message is displayed but the line that should display the message is still there

Bug 816306 introduced that feature. I feel sad because I reviewed it.

So here is the actual code that fails :

  systemStatus.textContent = _(value, null, _('check-error'));

The 3rd argument is the fallback argument, but the problem is that the l10n library does not handle this fallback argument correctly if it's a simple string.

In Bug 816306 I suggested to use this line instead :

  systemStatus.textContent = _(value) || _('check-error');

However we must first check that the l10n library is returning a falsy value when the key is not found.

So there are basically 2 simple paths here :
* either fix the fallback argument
* or don't use the fallback argument and make the l10n library return a falsy value when the key is not found

Talking with kaze, we'd prefer the second path, but this is more risky if this bug is tef+ (which I'm asking). The first path is less risky because we could fix it in a "backward compatible" way if necessary (ie: if any other app use this argument too).

Comment 1

[Andrew Overholt [:overholt]]

Tracking but not blocking, given the fact that this won't change user behavior when they run into a 404 update. Please nominate for approval once this is resolved.

Comment 2

[Fabien Cazenave [:kaze]]

(In reply to Julien Wajsberg [:julienw] (PTO until Monday 13th May) from comment #0)
> Talking with kaze, we'd prefer the second path

I confirm. In retrospect, I realized this optional « fallbackString » parameter in webL10n.get() is a huge mistake, and I’d like to get rid of it.

Comment 3

[Anthony Ricaud (:rik)]

So l10n.js does return an empty string but when looking at the code I'm not sure that was intended. Anyway, that means we can just do |systemStatus.textContent = _(value) || _('check-error');|

Comment 4

[Anthony Ricaud (:rik)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9684

Pointer to Github pull-request

Comment 5

[Anthony Ricaud (:rik)]

Comment on attachment 748027 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9684

To test, you can put this in build/custom-prefs.js
pref("app.update.url", "http://update.boot2gecko.org/nightly/update_with_404_test.xml");

and then
|rm -rf profile|
|make reset-gaia|

Comment 6

[Julien Wajsberg [:julienw]]

I'd love a test for the l10n library that somehow checks that getting an unknown l10n key will yield a falsy value. I'd rather have |null| but |''| works for me too. I'm only afraid that, as you said, returning |''| was not really intended, and then might be "fixed" at one point.

Comment 7

[Julien Wajsberg [:julienw]]

also, webl10n's fallback capacity is badly implemented, so if we don't fix it here I'd like another bug to get rid of it.

Comment 8

[Anthony Ricaud (:rik)]

I've updated the pull request with a test! It was a lot of trial/error, but it works. That means we'll now be able to test the L10n lib.

Comment 9

[Julien Wajsberg [:julienw]]

\o/

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 748027 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/9684

Nice, thanks!

Comment 11

[Fabien Cazenave [:kaze]]

merged on master:
https://github.com/mozilla-b2g/gaia/commit/30683cf76e43c3f85681f568d489d4f8d45380ce

Comment 12

[Anthony Ricaud (:rik)]

I've opened bug 872527 for the followup to clean up l10n.js.

Comment 13

[Anthony Ricaud (:rik)]

And a followup commit because I broke the linter tests :( I thought I ran |make lint| before submitting that patch.

https://github.com/mozilla-b2g/gaia/commit/b81008e15aef5060930430b41816044197e8b47b

Comment 14

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

v1-train: c2852a0

Comment 15

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

v1-train: c845ccb