Closed Bug 814414 Opened 12 years ago Closed 11 years ago

Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false

Categories

(Thunderbird :: Toolbars and Tabs, defect)

defect
Not set
minor

Tracking

(thunderbird18 wontfix, thunderbird19 fixed, thunderbird20 fixed, thunderbird-esr1717+ fixed)

RESOLVED FIXED
Thunderbird 20.0
Tracking Status
thunderbird18 --- wontfix
thunderbird19 --- fixed
thunderbird20 --- fixed
thunderbird-esr17 17+ fixed

People

(Reporter: sergei.ivn+bugzilla, Assigned: Paenglab)

References

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

mail.chat.enabled to false


Actual results:

Appmenu entries "Go -> Chat" and "Tools -> Chat status" still here


Expected results:

No menu entries for chats.
See Also: → TB-AppMenu
Summary: Menu entries for chat is in AppMenu with mail.chat.enabled set to disable → Menu entries for chat is in AppMenu when mail.chat.enabled set to disable
Summary: Menu entries for chat is in AppMenu when mail.chat.enabled set to disable → Menu entries for chat is in AppMenu when mail.chat.enabled set to false
confirming per BWinton's Bug 650170 Comment 99.
Blocks: TB-AppMenu
Status: UNCONFIRMED → NEW
Component: Untriaged → Toolbars and Tabs
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Version: 17 → Trunk
Severity: normal → minor
Summary: Menu entries for chat is in AppMenu when mail.chat.enabled set to false → Menu entries for chat still shown in AppMenu when mail.chat.enabled set to false
Attached patch patch (obsolete) — Splinter Review
This patch removes the menuitems.

I tried also to combine two menuitems like you suggested in bug 650170 comment 100, but this didn't work:

  setStatusMenupopupCommand: function(aEvent) {
    let target = aEvent.originalTarget;
    if (target.getAttribute("id") == "imStatusShowAccounts" || "appmenu_imStatusShowAccounts") {
      openIMAccountMgr();
      return;
    }

Instead of the status change only the IMAccountMgr opens.

Florian, what do you think, is this patch safe for 17ESR?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #684734 - Flags: review?(florian)
(In reply to Richard Marti [:Paenglab] from comment #2)

Shouldn't appmenu_afterChatSeparator also be hidden?


>   setStatusMenupopupCommand: function(aEvent) {
>     let target = aEvent.originalTarget;
>     if (target.getAttribute("id") == "imStatusShowAccounts" ||
> "appmenu_imStatusShowAccounts") {
>       openIMAccountMgr();
>       return;
>     }

I suggested:
     if (target.getAttribute("id") == "imStatusShowAccounts") ||
         target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {
Attached patch patch v2Splinter Review
(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to Richard Marti [:Paenglab] from comment #2)
> 
> Shouldn't appmenu_afterChatSeparator also be hidden?

fixed

> I suggested:
>      if (target.getAttribute("id") == "imStatusShowAccounts") ||
>          target.getAttribute("id") == "appmenu_imStatusShowAccounts")) {

This works after removing of two braces.
Attachment #684734 - Attachment is obsolete: true
Attachment #684734 - Flags: review?(florian)
Attachment #684792 - Flags: review?(florian)
Florian, please could you review this? Maybe this would be something for ESR17 (especially for enterprises).
Comment on attachment 684792 [details] [diff] [review]
patch v2

I have some doubts on appmenu_afterChatSeparator, but it shouldn't matter much, and I assume you have tested the patch and it seemed fine to you when looking at that menu with chat disabled.

Sorry for the delay here :-/.

This indeed seems safe for esr17.

[Approval Request Comment]
Regression caused by (bug #): bug 650170
User impact if declined: Some chat menu items will be visible when chat is pref'ed off, which may happen in enterprise contexts.
Testing completed (on c-c, etc.): none by me. I hope Richard has tested the patch.
Risk to taking this patch (and alternatives if risky): Extremely low (even if there was a typo in some ids, there's a null check in the code that would ensure the typo doesn't break anything...).
Attachment #684792 - Flags: review?(florian)
Attachment #684792 - Flags: review+
Attachment #684792 - Flags: approval-comm-esr17?
Keywords: checkin-needed
Comment on attachment 684792 [details] [diff] [review]
patch v2

[Triage Comment]
Please land an aurora as well. We shouldn't need to land on beta as we're not releasing that.
Attachment #684792 - Flags: approval-comm-esr17?
Attachment #684792 - Flags: approval-comm-esr17+
Attachment #684792 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: