362502 - TB-3a1 shouldn't create a rss folder when that rss link is Invalid

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Nir]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061201 Minefield/3.0a1
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20061201 Minefield/3.0a1

Using TB-3a1 (20061201) under Fedora Core 5.

Trying to subscribe to this rss link 'http://www.ddinews.gov.in/rssCMS/rss.aspx'.
So I have clicked on 'News & Blog' account > Subscribe > Add > Paste 'http://www.ddinews.gov.in/rssCMS/rss.aspx' url

when I clicked on 'Ok' , TB shows 'http://www.ddinews.gov.in/rssCMS/rss.aspx is not a Valid Rss feed' but meanwhile It has already created an entry for that rss link under 'News & Blogs' account.



Reproducible: Always

Steps to Reproduce:
1.Right click on 'News & Blogs' account > Subscribe > Add 
2.Paste 'http://www.ddinews.gov.in/rssCMS/rss.aspx' url
3.click on 'Ok'

Actual Results:  
TB shows an 'Invalid rss feed' alert but also creates an entry related to that feed under 'News & Blogs'

Expected Results:  
TB shouldn't create any entry under 'News & Blogs' account when rss link is invalid

TB should check rss link first before creating an entry under 'News & Blogs' account

Comment 1

[Boying Lu]

Attachment: Don't create folder when feed is invalid

Comment 2

[Scott MacGregor]

Comment on attachment 268326 [details] [diff] [review]
Don't create folder when feed is invalid

adding a name to the review field. :)

Comment 3

[Phil Ringnalda (:philor)]

Sorry for the slow review - I should have time for it this weekend.

Comment 4

[Phil Ringnalda (:philor)]

Comment on attachment 268326 [details] [diff] [review]
Don't create folder when feed is invalid

>+var InvalidFeed = false;

Please make that a property of the Feed object, rather than a global.

>   storeNextItem: function()
>   {
>+    if (InvalidFeed)
>+    {
>+      InvalidFeed = false;
>+      return;

That's both too late to check, and too early to return: since you already know it's invalid before parse() calls storeNextItem(), there's no need to call it, but quitting without hitting cleanupParsingState() means this.request doesn't get nulled out.

Comment 5

[Boying Lu]

Attachment: modified according to Phil Ringnalda's comments

Comment 6

[Phil Ringnalda (:philor)]

Comment on attachment 270697 [details] [diff] [review]
modified according to Phil Ringnalda's comments

Sorry, I misled you by talking about hitting cleanupParsingState() - I didn't mean that you should call it (since most of it is useless when you know you haven't parsed out any items), just that you should do the same this.request = null; as it. Once more, and then we can see what mscott says I missed.

Comment 7

[Boying Lu]

Attachment: new patch

Comment 8

[Phil Ringnalda (:philor)]

Attachment: Unrotted

Wups, forgot that I was going to attach a version with a little less bitrot.

Comment 9

[Scott MacGregor]

Comment on attachment 271000 [details] [diff] [review]
Unrotted

this seems reasonable to me.

Comment 10

[Boying Lu]

(In reply to comment #9)
> (From update of attachment 271000 [details] [diff] [review])
> this seems reasonable to me. 
> 

Scott, can you check in the patch?  Thanks

Comment 11

[Phil Ringnalda (:philor)]

See http://developer.mozilla.org/devnews/index.php/2007/07/10/getting-a-patch-checked-in/ - even though it's less reliable for Tbird, it may prove more reliable than asking Scott on a bug he's not cc'ed on :)

Comment 12

[Phil Ringnalda (:philor)]

mail/extensions/newsblog/content/Feed.js 1.50