830708 - updatable.js references undefined 'updateReady' string

Status: VERIFIED FIXED

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

Description

[Axel Hecht [:Pike]]

In bug 799888, a lot of code was refactored, and it removed the updateReady string from system.en-US.properties, but kept using it.

I'm not sure if that's intentional (aka, having a dialog without title) or accidental.

Guess we should make it intentional either way, that is, either passing in a blank title to CustomDialog.show, or get the title back in and localized.

Comment 1

[Julien Wajsberg [:julienw] (PTO -> may 13)]

This would be a very safe patch, either adding a line in locale or passing an empty string when creating the CustomDialog.

So nominating, and asking for Josh's comment so that his wisdom will tell us what we should do ;)

Comment 2

[Jason Smith [:jsmith]]

Can you be a bit more specific where this 'updateReady' string is referenced?

Comment 3

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Jason, this is the title of the dialog shown when a system update is downloaded and we have to choose between "later" or "apply now".

Comment 4

[Josh Carpenter [:jcarpenter]]

Attachment: Screenshot - update available

Attached is a wireframe of how we originally intended to display the Updates available prompt. The title was "[N] updates available". This same title would also be used for the Notification title.

This is still the preferred behavior for v1. A blank title is not acceptable. How will the user know what the notification pertains to?

Comment 5

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Josh, this one is OK.

The one that is missing here is the title for the "apply system update" prompt.

Comment 6

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Attachment: patch v1

ok, more time coding, less time talking ;)

Comment 7

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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

Pike, I don't think this should go to v1.0.0, I'd rather see this in v1-train or even in master.

What are the implications for you ? Should we still tag this late-l10n ?

Comment 8

[Axel Hecht [:Pike]]

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

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

We haven't figured anything out yet when it comes down to v1-* and beyond.

I'd expect that you'd want master for sure, and then you'd need to ask for approval for each branch.

Comment 9

[Julien Wajsberg [:julienw] (PTO -> may 13)]

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

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 10

[Josh Carpenter [:jcarpenter]]

(In reply to Julien Wajsberg [:julienw] from comment #5)
> Josh, this one is OK.
> 
> The one that is missing here is the title for the "apply system update"
> prompt.

Ah, thank you for clarifying. Let's go with "System update ready".

Comment 11

[Julien Wajsberg [:julienw] (PTO -> may 13)]

https://github.com/mozilla-b2g/gaia/commit/8128b5b7562eaae4e9bcca28967c7d1463e71693

Comment 12

[Jason Smith [:jsmith]]

Why remove the approval? Can you clarify?

At this current state, this will only land on master (v2).

Comment 13

[Julien Wajsberg [:julienw] (PTO -> may 13)]

That's good enough for me. I'm tired of running behind nomination and approval stuff.

Comment 14

[Jason Smith [:jsmith]]

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

Actually, let's put the approval back on. Triage thought this was tracking+, so it might be worth taking the patch.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Axel- I know that you're saying we need to wrap up on taking late-l10n changes so that makes us inclined to deny this for uplift, can anyone state whether this bug is user-affecting enough that we should override that impulse for a v1-train uplift? or v1.0.0?

Comment 16

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Without this patch, the user doesn't know that the system update prompt concerns the system update.

The only text is : "Install update now?" with "later" or "install now" buttons.

With this patch, we have the title "System Update Ready".

So I think this is nice to have even in v1.0.0.

Comment 17

[Jason Smith [:jsmith]]

(In reply to Lukas Blakk [:lsblakk] from comment #15)
> Axel- I know that you're saying we need to wrap up on taking late-l10n
> changes so that makes us inclined to deny this for uplift, can anyone state
> whether this bug is user-affecting enough that we should override that
> impulse for a v1-train uplift? or v1.0.0?

Not worth uplifting for v1.0.0. Might be worth uplifting for v1 train though.

Comment 18

[Alex Keybl [:akeybl]]

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

Approving for v1-train (1.0.1).

Comment 19

[Alex Keybl [:akeybl]]

(in the future, please don't hesitate to land to master while waiting for approval to land on v1-train)

Comment 20

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Alex, this landed in master 5 days ago (comment 11) :)

That's why this bug is resolved/fixed BTW.

Comment 21

[Julien Wajsberg [:julienw] (PTO -> may 13)]

Alex, why did you put "affected" on status-b2g18-v1.0.0 ?

Comment 22

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

v1-train: 37365a30b4bd2da9031a7774f7790d21c7f514a5

Comment 23

[Delphine Lebédel [:delphine - Use needinfo]]

Verified fixed in 2013-02-06-11-05-09 pvt nightly b2g18 build

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: bugs fixed on b2g18 since 1/25 branch of v1.0 are fixed on v1.0.1