307629 - When importing an OPML file that only contains feeds that are already subscribed then error message wrongly says the file is not valid.

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect, P1)

Description

[Pontus Freyhult]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; sv-SE; rv:1.8b4) Gecko/20050909 Firefox/1.4
Build Identifier: Tb 1.4

When importing a OPML file that only contains feed we already subscribe to, the
error message "Either FILE is not a valid OPML file or there was an error
importing the file."

It should either be silent or simply say that we were already subscribed to all
feeds in the OPML file.


Reproducible: Always

Steps to Reproduce:
1. From manage subscriptions, export OPML file
2. Import file just exported


Actual Results:  
User receives a confusing error message.

Expected Results:  
Software should either be silent (as normally after a successful import) or
indicate that there were no feeds to which we weren't already subscribed.

Comment 1

[Peter Tandler]

I vote for this as well!

Comment 2

[Joshua Cranmer [:jcranmer]]

Confirmed on Linux 2.0.0.17pre. Steps to reproduce work perfectly; can also confirm that it's not an export OPML issue.

Comment 4

[Wayne Mery (:wsmwk)]

robert, will this be improved by bug 450543?

Comment 5

[Robert Sayre]

No, the toolkit parser only handles RSS/Atom.

Comment 7

[:aceman]

Should import work additively? Only add feeds not already existing. Or should it replace (delete all and add imported) existing feeds?

Comment 8

[Nicolas Borboën]

No replacement (that would mean that you have to re-read your feed). If the feed belong to another folder, duplicate the feed.

Comment 9

[fengyuan]

Can we just tweak the error message? The current one is misleading.
("Either My Thunderbird Feeds.opml is not a valid OPML file or there was an error importing the file")

Can we just change the error message into something like "Can not add duplicate feed"?

Comment 10

[:aceman]

I could change the text, but that would need to find out if that message is used only on this occasion. If it is a general fallback message for all "unknown" reasons, then we can't change it straight away.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to :aceman from comment #10)
> I could change the text, but that would need to find out if that message is
> used only on this occasion.

The better option is to change the logic to behave as Nicolas suggests in comment 8:

1. if the feed is an exact duplicate (same feed, same folder), ignore it;
2. if the feed is an inexact duplicate (same feed, different folder), duplicate the feed in the other folder.

Even if the error message were better, it would still require the user to then figure out what to do about it.  And the options (i.e. manually editing the OPML file, which is intended for machine consumption, to remove the duplicate feeds) are not great.

Comment 12

[:aceman]

Ok, I have looked into the source and it seems I can make out where the problem is. I can separate the error message when the file is really broken and when all the feeds are already subscribed.

Can anybody attach a sample opml file here? I try to add some feeds but they seem not to work and do not export (the opml file is empty). So I need a working file to import and import again. Thanks.

But I can't change the test whether the feed already exists and in which folder. I am not familiar enough with the code.

Comment 13

[Nicolas Borboën]

Attachment: OPML export of some of my feeds

Here is my "MyThunderbirdFeeds.opml" export as request by :aceman.

Comment 14

[Nicolas Borboën]

Attachment: OPML export of some of my feeds with a directory (sub outline)

A different version with a sub outline wich should be interpreted as a directory tree when importing. The tree is the last outline named "del.icio.us".

Comment 15

[Nicolas Borboën]

Hey guys, one last thing concerning directory structure when importing: this bug is related to https://bugzilla.mozilla.org/show_bug.cgi?id=307724 which annoys me since 2005. Exporting the feeds structure with directories (or sub outline) works well. But when importing it, TB just don't care about it.

Comment 16

[:aceman]

Great, so the folder matching from comment 8 and comment 11 can be done in that bug 307724.

Let's just fix the error message here. I have a preliminary patch ready for this so that the message is more verbose and tells what is going on. I'll check if it works on the attached samples. I will then upload it in several hours.

Nicolas, please attach your sample with subfolders also into bug 307724 so that some developer taking that bug has something to work on. I am not yet up to that task, maybe later.

Comment 17

[:aceman]

Attachment: more verbose status report

This is my proposal. It may be overkill and too verbose, but at least it does not lie to the user and provides status about the import. I split the "invalid file" message into 3 different reasons of failure, that can happen in the code.
(Maybe 4, but I stuck it into 3 strings for now.) Yes, there are localization problems, which I'll fix later. Please now tell me about conceptual problems you see in this proposal. Thanks.

Comment 18

[:aceman]

I found a possible bug while testing the attached files. If deleted feeds are still in Trash, they are considered valid and subscribed so will not be imported from the file. Is that a problem and is that reported somewhere?

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 579775 [details] [diff] [review]
more verbose status report

I agree with your intuition: the user experience of importing with this patch is too verbose. ;-)  However, the implementation looks reasonable given the UX targeted, so f=myk.

Regarding the UX, I would indeed separate the handling of "not a valid OPML file" from "valid OPML file with some feeds that were not imported", as this patch does; and I would continue to alert users to a complete failure to import the OPML file, as this patch continues to do; but I wouldn't further differentiate between "bad records" and "internal errors," as that is a distinction that will not benefit most users.

Nor would I give users such detailed information about the import.  Rather, I would take the optimistic view that the result of the import will be what the user expects (as indeed it will be for the common case), i.e. that our best effort to import the feed--discarding duplicates, bad records, and records we fail to import due to an internal error--will be successful.  And so I would only let the user know that we successfully imported the OPML file, even if there were bad records, internal errors, and duplicates.

Then, for the edge cases where that successful import does not result in what the user expects, I would provide useful information to the user, but in another location, f.e. via entries in the Activity Manager, so the user can look there to diagnose and report problems that we can then address.

Note: OPML doesn't require that records include a URL, so "bad records" aren't really bad.  They're either sub-outline containers or records that represent something other than a feed.  Thus it isn't so much that we fail to import them because they are bad as that we succeed at ignoring them because they do not represent feeds.

(We do fail to import feeds inside sub-outlines; but that's a separate issue.)

Also, the `if (!feed)` check is extraneous (both in the original code and with this patch), as the implementation of storeFeed() never returns anything other than a `Feed` instance, which will always evaluate to `true` in a conditional expression.  So there will never be an "internal error" (unless something throws, but then the function will propagate an exception rather than returning a value that evaluates to `false`).

Comment 20

[:aceman]

Attachment: v2, less verbose status report

Thanks for thorough feedback. I think I addressed most of your comments. I am not able to send anything to the Activity manager. So I at least show there were some skipped entries (only if needed). But in the common case of all feeds correctly imported it just shows one message with the count of them.

The patch is not ready for r+, as I will add strings for Seamonkey if you like this.

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to :aceman from comment #18)
> I found a possible bug while testing the attached files. If deleted feeds
> are still in Trash, they are considered valid and subscribed so will not be
> imported from the file. Is that a problem and is that reported somewhere?

Alta88 didn't you fix that recently ?

Comment 22

[:aceman]

It may be a manifestation of problem in bug 307724, that the folder structure in the file, but also in TB is ignored and the feeds are compared as if being in flat lists. However, Trash should have been exempted from the list to compare.

Comment 23

[alta88]

(In reply to Ludovic Hirlimann [:Usul] from comment #21)
> (In reply to :aceman from comment #18)
> > I found a possible bug while testing the attached files. If deleted feeds
> > are still in Trash, they are considered valid and subscribed so will not be
> > imported from the file. Is that a problem and is that reported somewhere?
> 
> Alta88 didn't you fix that recently ?

various pieces are fixed in Bug 705504 and Bug 711173 (pending).

exportOpml cannot currently be guaranteed to be complete/accurate at all given the feed db/folder feedUrl sync issues.  the flattening of the folder structure may be related to that, ie the feeds db never reflected a folder move.  what you see in Subscribe is not what is in the db if you've moved folders in folder pane.

root bug is https://bugzilla.mozilla.org/attachment.cgi?id=580820

it looks like the opml code is doing a recursive subfolder processing ok based on a 3 second look.

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 581045 [details] [diff] [review]
v2, less verbose status report

I don't see why this code can't talk to the Activity Manager; it should be able to do so just like other JS code does.  Nevertheless, this code currently uses a model dialog to communicate its state, so I suppose it's fine to make the iterative improvement of improving that dialog before jumping whole hog into modernizing it to use the new ways Thunderbird communicates with its users.

I quibble a bit with the usage of "unique", which won't be obvious to some people; a better (if more verbose) message would be something like "Imported %S feeds to which you aren't already subscribed."  But overall this looks good and is a worthy improvement. f=myk!

Comment 25

[:aceman]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> I don't see why this code can't talk to the Activity Manager; it should be
> able to do so just like other JS code does.
The code probably could, but I can't with my knowledge :)

> Nevertheless, this code
> currently uses a model dialog to communicate its state, so I suppose it's
> fine to make the iterative improvement of improving that dialog before
> jumping whole hog into modernizing it to use the new ways Thunderbird
> communicates with its users.
Yes, please let me just fix the message text in this bug.

Comment 26

[:aceman]

Attachment: v3 patch

Fix per comment 24 and seamonkey strings added.

Comment 27

[Ian Neal]

Comment on attachment 582598 [details] [diff] [review]
v3 patch

Some comments from just doing code inspection, no testing.

>+++ b/mailnews/extensions/newsblog/content/feed-subscriptions.js
>@@ -950,81 +950,93 @@ var gFeedSubscriptionsWindow = {
>       opmlDom = parser.parseFromStream(stream, null, stream.available(), 'application/xml');
>     }catch(e){
>       promptService.alert(window, null, this.mBundle.getString("subscribe-errorOpeningFile"));
>       return;
>     }finally{
>       stream.close();
>     }
> 
>+    let statusReport;
>+    let feedsAdded = 0;
I'm not too keen at using let at this level, prefer var.

>+      let outlines = opmlDom.getElementsByTagName("body")[0].getElementsByTagName("outline");
>+      // try to import records if there are any
>+      if (outlines.length)
Is this check necessary, doesn't the for statement below do it for you?

>+      {
>+        for (let index = 0; index < outlines.length; index++)
>+        {
>+          if (this.importOutline(outlines[index]) == 1)
>+            feedsAdded++;
>+        }
>+      }

>   importOutline: function(aOutline)
>   {

>+      let feed = this.storeFeed(feedProperties);
Is feed always going to exist?

>+    else
>+    {
>+      return 0; // feed is already subscribed
This doesn't need to be in an else (stops strict JS complaint too)

I am happy from the SM side as long as they match the TB ones and people are happy from the TB side.
r=me for the strings on that proviso
r=me on the code with issues answered, though there may well be more from Myk ;)

Comment 28

[:aceman]

I think he already saw the code and didn't have any problems. But he wasn't asked to do full review so far :) But I'll do your fixes.

(In reply to Ian Neal from comment #27)
> >+      let feed = this.storeFeed(feedProperties);
> Is feed always going to exist?

There was a check originally, but then see comment 19.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 582598 [details] [diff] [review]
v3 patch

(In reply to Ian Neal from comment #27)
> >+    let statusReport;
> >+    let feedsAdded = 0;
> I'm not too keen at using let at this level, prefer var.

I generally treat `let` as the new `var` and prefer it at all levels for uniformity and simplicity, but I'm not set on that, nor am I particularly familiar with Thunderbird coding conventions, so I'm fine with making these declarations use `var` if that's what they typically use in this codebase.


> >+      let outlines = opmlDom.getElementsByTagName("body")[0].getElementsByTagName("outline");
> >+      // try to import records if there are any
> >+      if (outlines.length)
> Is this check necessary, doesn't the for statement below do it for you?

Yup, good catch, it isn't necessary and should be removed.


> >+      let feed = this.storeFeed(feedProperties);
> Is feed always going to exist?

Yes, per the last paragraph in comment 19! :-)


> >+    else
> >+    {
> >+      return 0; // feed is already subscribed
> This doesn't need to be in an else (stops strict JS complaint too)

There isn't actually a strict warning here, since the JS engine is smart enough to see that the function always returns a value, even if all return statements are within conditionals (unlike getFeedsInFolder(), which sometimes returns no value, although its consumers don't take that into account; we should fix that in another bug).

Nevertheless, the point is valid: for the sake of simplicity, when a conditional returns, don't follow it with an else block, as noted in the Mozilla Coding Style Guide <https://developer.mozilla.org/En/Developer_Guide/Coding_Style>.


> r=me on the code with issues answered, though there may well be more from
> Myk ;)

The only other issue I see is that the syntax of the English status reports will be funky when one feed was imported or found:

> "Imported 1 new feeds."
  should be ->
> "Imported 1 new feed."

> "Imported 1 new feeds to which you aren't already subscribed (out of 7 total entries found)."
  should be ->
> "Imported 1 new feed to which you aren't already subscribed (out of 7 total entries found)."

> "Imported 0 new feeds to which you aren't already subscribed (out of 1 total entries found)."
  should be ->
> "Imported 0 new feeds to which you aren't already subscribed (out of 1 total entry found)."

However, alternatives that avoid the funkiness tend to be clunky, to wit:

    Number of new feeds imported: 1.

Coming up with such a construction for subscribe-OPMLImportTotalFeeds is challenging.

So I think the current strings are the best compromise (until l20n)!


Otherwise, I see no issues, and the patch behaves as expected in my testing. r=myk with the identified changes!

Comment 30

[:aceman]

I still have to update the patch!

Comment 31

[:aceman]

Attachment: v4 patch, with nits addressed

Some languages would even need 3 plural versions.

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 585485 [details] [diff] [review]
v4 patch, with nits addressed

Looks great, r=myk!

Comment 33

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/8dd62e9e6253

Comment 34

[Pavel Franc - Mozilla.cz]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #29)
> The only other issue I see is that the syntax of the English status reports
> will be funky when one feed was imported or found:
> 
> > "Imported 1 new feeds."
>   should be ->
> > "Imported 1 new feed."

And for other languages with multiple plurals it is simply wrong.
Please make localizer live easy and use
https://developer.mozilla.org/en/Localization_and_Plurals

Comment 35

[:aceman]

From reading that page I still have no idea how I should implement the strings correctly.

Comment 36

[:aceman]

On second read I think I now understand the method. If you file a bug for it, I can try to implement it.