1266358 - Hello is not properly disabled in ESR 45 builds

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Ada [:adalucinet]]

Attachment: Screenshot.png

[Affected versions]:
45.1.0ESR (Build ID: 20160420142932)

[Affected platforms]:
- Windows 7 64-bit
- Mac OS X 10.11.1
- Ubuntu 14.04 32-bit

[Steps to reproduce]:
1. Launch ESR.
2. Go to about:support page.
3. Click on Tools via Menu bar.

[Expected result]:
Hello is properly disabled.

[Actual result]:
2. Firefox Hello Beta version 1.1.14 is displayed in Extensions area.
3. 'Start a conversation' option is displayed.

[Regression range]:
- Not a regression - encountered the same behavior with the first 45ESR (45.0.1 build 1); imho, this started to happen as soon as bug 1241501 was pushed.
- Not reproducible with 38.8.0ESR

[Additional notes]:
1. When clicking on 'Start a conversation' via Menu bar/Tools, "ReferenceError: LoopUI is not defined" is displayed via Browser console.
2. Setting the severity for this issue as [major], because it might be hiding a bigger underlying issue.

Comment 1

[Mark Banner (:standard8)]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #0)
> [Expected result]:
> Hello is properly disabled.
> 
> [Actual result]:
> 2. Firefox Hello Beta version 1.1.14 is displayed in Extensions area.

This is expected, we've only disabled the functionality not the add-on itself.

> 3. 'Start a conversation' option is displayed.

The place to look is for the "Start a conversation" menu item, its in Firefox core. We should probably just remove it as that's easiest (it was added as an accessibility item).

Comment 2

[Ada [:adalucinet]]

(In reply to Mark Banner (:standard8) from comment #1)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #0)
> > [Expected result]:
> > Hello is properly disabled.
> > 
> > [Actual result]:
> > 2. Firefox Hello Beta version 1.1.14 is displayed in Extensions area.
> 
> This is expected, we've only disabled the functionality not the add-on
> itself.

I still don't feel like it's accurate for a user who gets to the about:support page, to see an enabled extension and no sign of it. Having the Enabled value displayed as 'false' in about:support would remove any confusion - so that it points to the correct state of the system add-on.

Comment 3

[Mark Banner (:standard8)]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #2)
> (In reply to Mark Banner (:standard8) from comment #1)
> > (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #0)
> > > [Actual result]:
> > > 2. Firefox Hello Beta version 1.1.14 is displayed in Extensions area.
> > 
> > This is expected, we've only disabled the functionality not the add-on
> > itself.
> 
> I still don't feel like it's accurate for a user who gets to the
> about:support page, to see an enabled extension and no sign of it. Having
> the Enabled value displayed as 'false' in about:support would remove any
> confusion - so that it points to the correct state of the system add-on.

Good point, I'm raising this with the Go Faster team and we might end up separating it out in the about:support page to a different section.

Comment 4

[Mike Kaply [:mkaply]]

> The place to look is for the "Start a conversation" menu item, its in Firefox core. We should probably just remove it as that's easiest (it was added as an accessibility item).

Couldn't it just be added via the add-on?

Comment 5

[Mark Banner (:standard8)]

(In reply to Mike Kaply [:mkaply] from comment #4)
> > The place to look is for the "Start a conversation" menu item, its in Firefox core. We should probably just remove it as that's easiest (it was added as an accessibility item).
> 
> Couldn't it just be added via the add-on?

It is in subsequent releases. 45 got messed up a bit as we were caught by backouts and other issues. This just got missed as a potential issue.

Comment 6

[Mark Banner (:standard8)]

Attachment: Swap how menus are managed [45 patch only]

Since 45 got the upgraded Loop add-on, and this was landed into ESR before it was released, that means we can simply change how the menus item is created to the post 45 way.

This then means we get for free the menu item not being created when the loop.enabled preference is false.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8749156 [details] [diff] [review]
Swap how menus are managed [45 patch only]

Just to be explicit, other branches aren't affects as they've already got this way of working. 45 was special initially.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8749156 [details] [diff] [review]
Swap how menus are managed [45 patch only]

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

LGTM

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8749156 [details] [diff] [review]
Swap how menus are managed [45 patch only]

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Useless menu item in default setup for ESR 45
User impact if declined: There's a menu item that does nothing.
Fix Landed on Version: This patch is based on the work of Loop post-45 release. The differences in 45 were originally due to use working around not having the full loop add-on for the initial 45 release.
Risk to taking this patch (and alternatives if risky): Low, disables the menuitem, would re-enable it if Loop is manually enabled.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

I agree this is a good idea to fix in ESR though it isn't a security or stability issue. This should make it into 45.2.0esr.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8749156 [details] [diff] [review]
Swap how menus are managed [45 patch only]

Approved for uplift to esr45, we don't want an entire menu item to be empty/nonresponsive and it shouldn't be showing on esr.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/6738c720774c

Comment 13

[Ada [:adalucinet]]

Verified fixed with 45.2.0ESR build 2 (Build ID: 20160601155443), across platforms [1] - 'Start a conversation' option is no longer displayed via Menu bar/Tools.

[1] Ubuntu 14.04 32-bit, Mac OS X 10.10.5 and Windows 10 64-bit