Closed Bug 1008822 Opened 6 years ago Closed 6 years ago

Forwarding RSS post as inline does not use default account identity

Categories

(Thunderbird :: Message Compose Window, defect)

24 Branch
x86
Windows XP
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: jwbodnar, Assigned: alta88)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Selected a message in an RSS Feed:

1. Click the "Forward" button in the reading pane or the "Forward" button in the toolbar or specifically the "Forward Inline" toolbar button option.



Actual results:

From address of the message compose window defaults to first e-mail identity name in alphabetical order.


Expected results:

From address of the message compose window should default to default account (e-mail address).

Note, this IS what happens if "Forward As Attachment" is selected from the toolbar button.
Attached patch identity.patch (obsolete) — Splinter Review
identity fix, plus web page forward link fix (considers both tb and sm).
Assignee: nobody → alta88
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8421871 - Flags: review?(mkmelin+mozilla)
Duplicate of this bug: 970155
Comment on attachment 8421871 [details] [diff] [review]
identity.patch

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

Sorry for the delay

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +42,5 @@
>                                          aType, aFormat, aIdentity, aMsgWindow)
>  {
> +  // Do not use the header derived identity in ComposeMessage() for feeds; it may
> +  // not be the default. Compose correctly gets the default if identity is null.
> +  aIdentity = null;

I don't think this is right, if the function has a parameter it should be used. So, either fix the callers to pass null, or remove the param from the function signature.
(I didn't verify the bug exist, but I guess it does if allIdentities order doesn't start with the default account, if so we get the wrong identity in some other edge cases also.)
Attachment #8421871 - Flags: review?(mkmelin+mozilla) → review-
Attached patch identity.patch (obsolete) — Splinter Review
Attachment #8421871 - Attachment is obsolete: true
Attachment #8428670 - Flags: review?(mkmelin+mozilla)
Keywords: regression
Attachment #8428670 - Flags: review?(mkmelin+mozilla)
something basic has broken with getting the default identity.
Attached patch identity.patch (obsolete) — Splinter Review
ok, so it appears that for non identity accounts, setting the identity explicitly from the default account and its default identity is required, so that what the user set in the ui relates to what actually happens on compose.   

attempting to get an identity from headers for feeds is a regression from bug 714570, but it's likely all the guesswork at an identity in mailCommands.js didn't work right in a multi account/multi identity scenario anyway.
Attachment #8428670 - Attachment is obsolete: true
Attachment #8428753 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8428753 [details] [diff] [review]
identity.patch

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

::: mailnews/extensions/newsblog/content/newsblogOverlay.js
@@ +46,5 @@
> +    // If there is no server identity (currently true for feed accounts but
> +    // not necessarily true for feed messages in non feed account folders),
> +    // use the defaults.
> +    aIdentity = MailServices.accounts.defaultAccount.defaultIdentity;
> +  }

this block is not necessary, as null will imply the default account, default identity

@@ +53,5 @@
> +  let tabmail = document.getElementById("tabmail");
> +  let is3pane = tabmail && tabmail.selectedTab && tabmail.selectedTab.mode ?
> +                  tabmail.selectedTab.mode.type == "folder" : false;
> +  let showingwebpage = ("FeedMessageHandler" in window) && !is3pane &&
> +                       FeedMessageHandler.onOpenPref == FeedMessageHandler.kOpenWebPage;

why wouldn't I be showing webpage in the 3pane?
(In reply to Magnus Melin from comment #7)
> Comment on attachment 8428753 [details] [diff] [review]
> identity.patch
> 
> Review of attachment 8428753 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/extensions/newsblog/content/newsblogOverlay.js
> @@ +46,5 @@
> > +    // If there is no server identity (currently true for feed accounts but
> > +    // not necessarily true for feed messages in non feed account folders),
> > +    // use the defaults.
> > +    aIdentity = MailServices.accounts.defaultAccount.defaultIdentity;
> > +  }
> 
> this block is not necessary, as null will imply the default account, default
> identity
> 

true.

> @@ +53,5 @@
> > +  let tabmail = document.getElementById("tabmail");
> > +  let is3pane = tabmail && tabmail.selectedTab && tabmail.selectedTab.mode ?
> > +                  tabmail.selectedTab.mode.type == "folder" : false;
> > +  let showingwebpage = ("FeedMessageHandler" in window) && !is3pane &&
> > +                       FeedMessageHandler.onOpenPref == FeedMessageHandler.kOpenWebPage;
> 
> why wouldn't I be showing webpage in the 3pane?

if you are, it's governed correctly by gShowFeedSummary; the var really means showinginwebpage only for a tab or window, for which gShowFeedSummary isn't necessarily correct and onOpenPref is.
Attached patch identity.patchSplinter Review
Attachment #8428753 - Attachment is obsolete: true
Attachment #8428753 - Flags: review?(mkmelin+mozilla)
Attachment #8432272 - Flags: review?(mkmelin+mozilla)
(In reply to Magnus Melin from comment #3)
> (I didn't verify the bug exist, but I guess it does if allIdentities order doesn't start with the default account, 
> if so we get the wrong identity in some other edge cases also.)

From: is "From: <Seunguck Lee>", "From: MySQL Bugs",  like one, no To:/CC:/Delivered-To:, no X-Account-Key:, no X-Identity-Key,.... if RSS Feed => Mail sent to me by BCC:, with wrong mail address or Mailing list.
"First identity of first account in mail.accountmanager.accounts" was used for Forward of RSS Feed in Tb 24.5.0. This looks final fall back identity("default account doesn't have identity" case, "default account points non-existent account" case,  etc.).
(In reply to alta88 from comment #8)
> > why wouldn't I be showing webpage in the 3pane?
> 
> if you are, it's governed correctly by gShowFeedSummary; the var really
> means showinginwebpage only for a tab or window, for which gShowFeedSummary
> isn't necessarily correct and onOpenPref is.

We really should clean up that mess...
Attachment #8432272 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/935a121a00df
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Component: Untriaged → Message Compose Window
You need to log in before you can comment on or make changes to this bug.