Allow empty name during import and use FeedDiscovery to try to figure the name out if possible (#1266)

While testing stringer locally, I tried to import my feeds from The Old Reader. The OPML generated by The Old Reader doesn't contain the feed names, so the import failed.
I made a change to allow nils and the import went through, but then the feeds had no name and it was hard to manage the feeds.

So I replicated the logic from `Feed::Create` to try to get a proper title for the Feed.

This will make the import slower in the case there's nameless entries, but the result seems to be better. I'm falling back to the url if the call to the feed failed to avoid retrying on dead feeds.

Author: renatolond

app/commands/feed/import_from_opml.rb

@@ -30,9 +30,18 @@ def find_or_create_group(group_name, user)
     def create_feed(parsed_feed, group, user)
       feed = user.feeds.where(**parsed_feed.slice(:name, :url))
                  .first_or_initialize
+      find_feed_name(feed, parsed_feed)
       feed.last_fetched = 1.day.ago if feed.new_record?
       feed.group_id = group.id if group
       feed.save
     end
+
+    def find_feed_name(feed, parsed_feed)
+      return if feed.name?
+
+      result = FeedDiscovery.call(parsed_feed[:url])
+      title = result.title if result
+      feed.name = ContentSanitizer.call(title.presence || parsed_feed[:url])
+    end
   end
 end

app/utils/opml_parser.rb

@@ -40,7 +40,7 @@ def extract_name(attributes)
 
     def feed_to_hash(feed)
       {
-        name: extract_name(feed.attributes).value,
+        name: extract_name(feed.attributes)&.value,
         url: feed.attributes["xmlUrl"].value
       }
     end

spec/commands/feed/import_from_opml_spec.rb

@@ -13,6 +13,8 @@
 
   autoblog = { name: "Autoblog", url: "http://feeds.autoblog.com/weblogsinc/autoblog/" }
   city_guide = { name: "City Guide News", url: "http://www.probki.net/news/RSS_news_feed.asp" }
+  macrumors = { name: "MacRumors: Mac News and Rumors - Front Page", url: "http://feeds.macrumors.com/MacRumors-Front" }
+  dead_feed = { name: "http://deadfeed.example.com/feed.rss", url: "http://deadfeed.example.com/feed.rss" }
 
   def read_subscriptions
     File.open(
@@ -28,6 +30,37 @@ def find_feed(feed_details)
     Feed.where(feed_details)
   end
 
+  before do
+    stub_request(:get, "http://feeds.macrumors.com/MacRumors-Front").to_return(
+      status: 200,
+      body: File.read("spec/sample_data/feeds/feed01_valid_feed/feed.xml")
+    )
+    stub_request(
+      :get,
+      "http://deadfeed.example.com/feed.rss"
+    ).to_return(status: 404)
+  end
+
+  context "adds title for existing feed" do
+    it "changes existing feed if name is nil" do
+      create_feed({ name: nil, url: macrumors[:url] })
+
+      described_class.call(read_subscriptions, user: default_user)
+
+      expect(find_feed(macrumors)).to exist
+    end
+
+    it "keeps existing feed name if name is something else" do
+      feed1 = create_feed({ name: "MacRumors", url: macrumors[:url] })
+
+      described_class.call(read_subscriptions, user: default_user)
+
+      expect(find_feed(macrumors)).not_to exist
+      feed1.reload
+      expect(feed1.name).to eq("MacRumors")
+    end
+  end
+
   context "adding group_id for existing feeds" do
     it "retains exising feeds" do
       feed1 = create_feed(tmw_football)
@@ -69,6 +102,8 @@ def find_feed(feed_details)
 
       expect(find_feed(tmw_football)).to exist
       expect(find_feed(giant_robots)).to exist
+      expect(find_feed(macrumors)).to exist
+      expect(find_feed(dead_feed)).to exist
     end
 
     it "sets group" do

spec/support/files/subscriptions.xml

@@ -25,5 +25,7 @@
         </outline>
         <outline title="Empty Group" text="Empty Group">
         </outline>
+        <outline type="rss" xmlUrl="http://feeds.macrumors.com/MacRumors-Front"/>
+        <outline type="rss" xmlUrl="http://deadfeed.example.com/feed.rss"/>
     </body>
 </opml>