Last Comment Bug 714570 - Forward feed article not include the full text but the URL only
: Forward feed article not include the full text but the URL only
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: 9
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: alta88
:
Mentors:
: 496572 725795 (view as bug list)
Depends on:
Blocks: 721517
  Show dependency treegraph
 
Reported: 2012-01-01 18:37 PST by Lu Wei
Modified: 2012-03-05 17:02 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
the feed article I want to forward (2.01 KB, text/plain)
2012-01-01 18:37 PST, Lu Wei
no flags Details
patch. (6.28 KB, patch)
2012-01-12 13:09 PST, alta88
no flags Details | Diff | Splinter Review
tweak to handle empty fwd prefix pref. (6.35 KB, patch)
2012-01-14 12:19 PST, alta88
no flags Details | Diff | Splinter Review
better tweak. (6.53 KB, patch)
2012-01-15 11:58 PST, alta88
no flags Details | Diff | Splinter Review
updated patch (8.96 KB, patch)
2012-02-29 16:48 PST, alta88
mozilla: review+
Details | Diff | Splinter Review
update for checkin. (8.91 KB, patch)
2012-03-05 10:36 PST, alta88
alta88: review+
Details | Diff | Splinter Review

Description Lu Wei 2012-01-01 18:37:15 PST
Created attachment 585242 [details]
the feed article I want to forward

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

Forward a feed article that has full text content


Actual results:

The forward message only contains the article's URL


Expected results:

The forward message should contains the full article
Comment 1 Lu Wei 2012-01-01 18:42:25 PST
In TB support group, news: <iaudncM-Rb2gFnLTnZ2dnUVZ_sWdnZ2d@mozilla.org> 
"alta88[nntp]" pointed out: 

"you have discovered an interesting bug.  if the messageId (View->Source 
or Show All Headers to see it) contains the url of the article (must be 
a valid url) then Forward includes it only.

if the messageId is anything else (invalid url or a 'tag:' format 
value), the full Summary of the article will filled in upon Forward.

the value used for messageId is based on what the publisher provides. 
but, for this purpose, Tb would need to do a workaround.  you should 
file a bug."
Comment 2 Hashem Masoud 2012-01-01 23:52:47 PST
(In reply to LU Wei from comment #1)
In my opinion, this should be an enhancement or an addon. It is not a bug.
Comment 3 Lu Wei 2012-01-02 03:25:39 PST
(In reply to Hashem Masoud from comment #2)
> (In reply to LU Wei from comment #1)
> In my opinion, this should be an enhancement or an addon. It is not a bug.

Whether you think the "enhancement" should make the forwards just forward a URL or full text, the present behavior (sometimes forward a URL, sometimes forward full text, according to "alta88[nntp]") is a bug.
Comment 4 alta88 2012-01-02 15:19:56 PST
this issue exists due to a Very Old bug 258278.  the conditions of that bug no longer exist, ie a summary is now always stored with the message to allow a toggle between summary and web page, since bug 438429.

the poor implementation in bug 258278 allowed forwarding summaries with tag: messageIds to work, but those feeds with http url messageIds got the link.  a feed messageId can be any construct.

if bug 258278 were reverted, then a forward would always include the summary (even if viewing a web page).  i can look into this at some point; forwarding a summary should definitely be possible.
Comment 5 Lu Wei 2012-01-02 18:22:10 PST
IMO, the default forward behavior should be the same as view setting, i.e. when in summary view, "forward" will forward the summary; when in web page view, "forward" will forward the web page.
Comment 6 alta88 2012-01-12 13:09:28 PST
Created attachment 588165 [details] [diff] [review]
patch.


this patch sorts out how feed messages are determined for further compose handling.  it applies only to feed account messages (feeds moved/copied to other account types get normal handling, as inline messagepane web page viewing only happens in feed accounts).  if a user is viewing a summary, single/multiselection compose will load summary(ies); if viewing a web page, load link(s).

@comment 5: i too think there are nice aspects to loading the entire web page, but there are also issues, and don't think it would meet with approval by those who decide.  a full web page would be better done in an extension.  opening a link can be done in a content tab, which can allow javascript, and mitigates the usability problem somewhat.
Comment 7 Lu Wei 2012-01-13 00:09:02 PST
Put "web page view" situation aside (anyway, the feed message body contains only  "summary"), I hope the patch will apply to all account folders. Feed message does not differ from mail message in storage format, in user experience (at simple HTML view and plain text view, which I prefer), and filters are normally used to re-organize messages to different folders, whatever the account they are under.
Comment 8 alta88 2012-01-13 11:43:11 PST
as mentioned, a feed article moved to a non feed account is treated normally, ie as any other message in that account.

it is actually not strictly possible to identify a message as a 'feed' outside its account.  content-base in the header, of course, is a good indicator, but that header can be sent along by anyone.  see bug 522645.
Comment 9 alta88 2012-01-14 12:19:51 PST
Created attachment 588676 [details] [diff] [review]
tweak to handle empty fwd prefix pref.
Comment 10 alta88 2012-01-15 11:58:16 PST
Created attachment 588760 [details] [diff] [review]
better tweak.
Comment 11 Lu Wei 2012-01-15 18:27:58 PST
(In reply to alta88 from comment #8)
> as mentioned, a feed article moved to a non feed account is treated
> normally, ie as any other message in that account.
> 
Thanks, that's good.

> it is actually not strictly possible to identify a message as a 'feed'
> outside its account.  content-base in the header, of course, is a good
> indicator, but that header can be sent along by anyone.  see bug 522645.

The current situation is TB identifies this, because if a feed message is moved to a mail account, then forwarded, the summary body is also cut off and just URL will be forwarded.
Comment 12 Ludovic Hirlimann [:Usul] 2012-01-16 06:50:29 PST
(In reply to alta88 from comment #10)
> Created attachment 588760 [details] [diff] [review]
> better tweak.

Alta88 could we get some unit tests too ?
Comment 13 alta88 2012-01-17 13:52:43 PST
if someone could point me to where compose window open type tests are, i could take a look to incorporating this.
Comment 15 Hashem Masoud 2012-02-14 00:24:49 PST
*** Bug 725795 has been marked as a duplicate of this bug. ***
Comment 16 alta88 2012-02-15 19:28:18 PST
*** Bug 496572 has been marked as a duplicate of this bug. ***
Comment 17 alta88 2012-02-29 16:48:46 PST
Created attachment 601822 [details] [diff] [review]
updated patch


updated patch to unbitrot and better handle empty content-base header.
Comment 18 David :Bienvenu 2012-03-05 08:23:18 PST
Comment on attachment 601822 [details] [diff] [review]
updated patch

r=me, but doesn't createInstance throws when it fails? That would make this check not useful:

+    if (!params || !composeFields)
+      return;
Comment 19 alta88 2012-03-05 09:49:51 PST
it looks like it.  those checks were there since the beginning.  do you prefer them removed?
Comment 20 David :Bienvenu 2012-03-05 10:12:10 PST
(In reply to alta88 from comment #19)
> it looks like it.  those checks were there since the beginning.  do you
> prefer them removed?

yes, thx, I think it's better to remove them just so no one copies the bad pattern.
Comment 21 alta88 2012-03-05 10:36:28 PST
Created attachment 602960 [details] [diff] [review]
update for checkin.
Comment 22 Ryan VanderMeulen [:RyanVM] 2012-03-05 17:02:37 PST
http://hg.mozilla.org/comm-central/rev/b3925ea79358

For future patches, please use a checkin comment that summarizes the changes made rather than reiterating the bug title.

Note You need to log in before you can comment on or make changes to this bug.