From f780e5fbc9fffb24d27368f48e0a54de858f61fc Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Mon, 14 Jan 2019 14:25:49 -0500 Subject: [PATCH] Refactor RssReader (#1537) * Update RssReader::get_all_articles * Move RssReader to services * Create RssReader::Assembler * Improve RssReader's spec --- app/{labor => services}/rss_reader.rb | 125 +---------------- app/services/rss_reader/assembler.rb | 126 ++++++++++++++++++ ...nly_articles_from_an_feed_url.approved.txt | 1 + .../gets_articles_for_user.approved.txt | 1 + .../parses_correctly.approved.txt | 24 ++++ spec/{labor => services}/rss_reader_spec.rb | 16 ++- 6 files changed, 168 insertions(+), 125 deletions(-) rename app/{labor => services}/rss_reader.rb (58%) create mode 100644 app/services/rss_reader/assembler.rb create mode 100644 spec/fixtures/approvals/rssreader/get_all_articles/fetch_only_articles_from_an_feed_url.approved.txt create mode 100644 spec/fixtures/approvals/rssreader/get_all_articles/gets_articles_for_user.approved.txt create mode 100644 spec/fixtures/approvals/rssreader/get_all_articles/parses_correctly.approved.txt rename spec/{labor => services}/rss_reader_spec.rb (76%) diff --git a/app/labor/rss_reader.rb b/app/services/rss_reader.rb similarity index 58% rename from app/labor/rss_reader.rb rename to app/services/rss_reader.rb index c1cd1ef99..1ce817719 100644 --- a/app/labor/rss_reader.rb +++ b/app/services/rss_reader.rb @@ -1,5 +1,3 @@ -require "rss" -require "open-uri" require "nokogiri" require "httparty" require "securerandom" @@ -14,11 +12,8 @@ class RssReader end def get_all_articles - User.where.not(feed_url: nil).each do |u| - feed_url = u.feed_url.strip - next if feed_url == "" - - create_articles_for_user(u) + User.where.not(feed_url: [nil, ""]).find_each do |user| + create_articles_for_user(user) end end @@ -38,7 +33,7 @@ class RssReader def create_articles_for_user(user) with_span("create_articles_for_user", user_id: user.id, username: user.username) do |metadata| - feed = fetch_rss(user.feed_url) + feed = fetch_rss(user.feed_url.strip) metadata[:feed_length] = feed.entries.length if feed&.entries feed.entries.reverse_each do |item| make_from_rss_item(item, user, feed) @@ -90,7 +85,8 @@ class RssReader published_at: item.published, published_from_feed: true, show_comments: true, - body_markdown: assemble_body_markdown(item, user, feed, feed_source_url), + # body_markdown: assemble_body_markdown(item, user, feed, feed_source_url), + body_markdown: RssReader::Assembler.call(item, user, feed, feed_source_url), organization_id: user.organization_id.present? ? user.organization_id : nil } article = with_timer("save_article", metadata) do @@ -107,117 +103,6 @@ class RssReader end end - def assemble_body_markdown(item, user, feed, feed_source_url) - with_span("assemble_body_markdown", item_title: item.title) do - body = <<~HEREDOC - --- - title: #{item.title.strip} - published: false - tags: #{get_tags(item[:categories])} - canonical_url: #{user.feed_mark_canonical ? feed_source_url : ''} - --- - - #{finalize_reverse_markdown(item, feed)} - - HEREDOC - body.strip - end - end - - def get_tags(categories) - categories.first(4).map { |tag| tag[0..19] }.join(",") if categories - end - - def get_content(item) - item.content || item.summary || item.description - end - - def finalize_reverse_markdown(item, feed) - cleaned_item_content = HtmlCleaner.new.clean_html(get_content(item)) - cleaned_item_content = thorough_parsing(cleaned_item_content, feed.url) - ReverseMarkdown.convert(cleaned_item_content, github_flavored: true). - gsub("```\n\n```", "").gsub(/ |\u00A0/, " ") - end - - def thorough_parsing(content, feed_url) - with_span("thorough_parsing", content_length: content.length) do - html_doc = Nokogiri::HTML(content) - find_and_replace_possible_links!(html_doc) - if feed_url.include?("medium.com") - parse_and_translate_gist_iframe!(html_doc) - parse_and_translate_youtube_iframe!(html_doc) - parse_and_translate_tweet!(html_doc) - else - clean_relative_path!(html_doc, feed_url) - end - html_doc.to_html - end - end - - def parse_and_translate_gist_iframe!(html_doc) - html_doc.css("iframe").each do |iframe| - a_tag = iframe.css("a") - next if a_tag.empty? - - possible_link = a_tag[0].inner_html - if /medium\.com\/media\/.+\/href/.match?(possible_link) - real_link = HTTParty.head(possible_link).request.last_uri.to_s - return unless real_link.include?("gist.github.com") - - iframe.name = "p" - iframe.keys.each { |attr| iframe.remove_attribute(attr) } - iframe.inner_html = "{% gist #{real_link} %}" - end - end - html_doc - end - - def parse_and_translate_tweet!(html_doc) - html_doc.search("style").remove - html_doc.search("script").remove - html_doc.css("blockquote").each do |bq| - bq_with_p = bq.css("p") - next if bq_with_p.empty? - - second_content = bq_with_p.css("p")[1].css("a")[0].attributes["href"].value - if bq_with_p.length == 2 && second_content.include?("twitter.com") - bq.name = "p" - tweet_id = second_content.scan(/\/status\/(\d{10,})/).flatten.first - bq.inner_html = "{% tweet #{tweet_id} %}" - end - end - end - - def parse_and_translate_youtube_iframe!(html_doc) - html_doc.css("iframe").each do |iframe| - if /youtube\.com/.match?(iframe.attributes["src"].value) - iframe.name = "p" - youtube_id = iframe.attributes["src"].value.scan(/embed%2F(.{4,12})%3F/).flatten.first - iframe.keys.each { |attr| iframe.remove_attribute(attr) } - iframe.inner_html = "{% youtube #{youtube_id} %}" - end - end - end - - def clean_relative_path!(html_doc, url) - html_doc.css("img").each do |img_tag| - path = img_tag.attributes["src"].value - img_tag.attributes["src"].value = URI.join(url, path).to_s if path.start_with? "/" - end - end - - def find_and_replace_possible_links!(html_doc) - html_doc.css("a").each do |a_tag| - link = a_tag.attributes["href"]&.value - next unless link - - found_article = Article.find_by(feed_source_url: link)&.decorate - if found_article - a_tag.attributes["href"].value = found_article.url - end - end - end - def get_host_without_www(url) url = "http://#{url}" if URI.parse(url).scheme.nil? host = URI.parse(url).host.downcase diff --git a/app/services/rss_reader/assembler.rb b/app/services/rss_reader/assembler.rb new file mode 100644 index 000000000..95c4a7c51 --- /dev/null +++ b/app/services/rss_reader/assembler.rb @@ -0,0 +1,126 @@ +class RssReader + class Assembler + def self.call(item, user, feed, feed_source_url) + new(item, user, feed, feed_source_url).assemble + end + + def initialize(item, user, feed, feed_source_url) + @item = item + @title = item[:title].strip + @categories = item[:categories] || [] + @user = user + @feed = feed + @feed_source_url = feed_source_url + end + + def assemble + body = <<~HEREDOC + --- + title: #{@title} + published: false + tags: #{get_tags} + canonical_url: #{@user.feed_mark_canonical ? @feed_source_url : ''} + --- + + #{assemble_body_markdown} + HEREDOC + + body.strip + end + + private + + def get_tags + @categories.first(4).map { |tag| tag[0..19] }.join(",") + end + + def assemble_body_markdown + cleaned_content = HtmlCleaner.new.clean_html(get_content) + cleaned_content = thorough_parsing(cleaned_content, @feed.url) + ReverseMarkdown. + convert(cleaned_content, github_flavored: true). + gsub("```\n\n```", "").gsub(/ |\u00A0/, " ") + end + + def get_content + @item.content || @item.summary || @item.description + end + + def thorough_parsing(content, feed_url) + html_doc = Nokogiri::HTML(content) + find_and_replace_possible_links!(html_doc) + if feed_url.include?("medium.com") + parse_and_translate_gist_iframe!(html_doc) + parse_and_translate_youtube_iframe!(html_doc) + parse_and_translate_tweet!(html_doc) + else + clean_relative_path!(html_doc, feed_url) + end + html_doc.to_html + end + + def parse_and_translate_gist_iframe!(html_doc) + html_doc.css("iframe").each do |iframe| + a_tag = iframe.css("a") + next if a_tag.empty? + + possible_link = a_tag[0].inner_html + if /medium\.com\/media\/.+\/href/.match?(possible_link) + real_link = HTTParty.head(possible_link).request.last_uri.to_s + return unless real_link.include?("gist.github.com") + + iframe.name = "p" + iframe.keys.each { |attr| iframe.remove_attribute(attr) } + iframe.inner_html = "{% gist #{real_link} %}" + end + end + html_doc + end + + def parse_and_translate_tweet!(html_doc) + html_doc.search("style").remove + html_doc.search("script").remove + html_doc.css("blockquote").each do |bq| + bq_with_p = bq.css("p") + next if bq_with_p.empty? + + second_content = bq_with_p.css("p")[1].css("a")[0].attributes["href"].value + if bq_with_p.length == 2 && second_content.include?("twitter.com") + bq.name = "p" + tweet_id = second_content.scan(/\/status\/(\d{10,})/).flatten.first + bq.inner_html = "{% tweet #{tweet_id} %}" + end + end + end + + def parse_and_translate_youtube_iframe!(html_doc) + html_doc.css("iframe").each do |iframe| + if /youtube\.com/.match?(iframe.attributes["src"].value) + iframe.name = "p" + youtube_id = iframe.attributes["src"].value.scan(/embed%2F(.{4,12})%3F/).flatten.first + iframe.keys.each { |attr| iframe.remove_attribute(attr) } + iframe.inner_html = "{% youtube #{youtube_id} %}" + end + end + end + + def clean_relative_path!(html_doc, url) + html_doc.css("img").each do |img_tag| + path = img_tag.attributes["src"].value + img_tag.attributes["src"].value = URI.join(url, path).to_s if path.start_with? "/" + end + end + + def find_and_replace_possible_links!(html_doc) + html_doc.css("a").each do |a_tag| + link = a_tag.attributes["href"]&.value + next unless link + + found_article = Article.find_by(feed_source_url: link)&.decorate + if found_article + a_tag.attributes["href"].value = found_article.url + end + end + end + end +end diff --git a/spec/fixtures/approvals/rssreader/get_all_articles/fetch_only_articles_from_an_feed_url.approved.txt b/spec/fixtures/approvals/rssreader/get_all_articles/fetch_only_articles_from_an_feed_url.approved.txt new file mode 100644 index 000000000..3cacc0b93 --- /dev/null +++ b/spec/fixtures/approvals/rssreader/get_all_articles/fetch_only_articles_from_an_feed_url.approved.txt @@ -0,0 +1 @@ +12 \ No newline at end of file diff --git a/spec/fixtures/approvals/rssreader/get_all_articles/gets_articles_for_user.approved.txt b/spec/fixtures/approvals/rssreader/get_all_articles/gets_articles_for_user.approved.txt new file mode 100644 index 000000000..56a6051ca --- /dev/null +++ b/spec/fixtures/approvals/rssreader/get_all_articles/gets_articles_for_user.approved.txt @@ -0,0 +1 @@ +1 \ No newline at end of file diff --git a/spec/fixtures/approvals/rssreader/get_all_articles/parses_correctly.approved.txt b/spec/fixtures/approvals/rssreader/get_all_articles/parses_correctly.approved.txt new file mode 100644 index 000000000..bc25d2603 --- /dev/null +++ b/spec/fixtures/approvals/rssreader/get_all_articles/parses_correctly.approved.txt @@ -0,0 +1,24 @@ +--- +title: Testing RSS Feed +published: false +tags: test +canonical_url: +--- + +youtube link here + +{% youtube QOCaacO8wus %} + +tweet here + +{% tweet 948256083352735744 %} + +Github gist here + + + +code block + +``` +testsetsetsetsetsetset +``` \ No newline at end of file diff --git a/spec/labor/rss_reader_spec.rb b/spec/services/rss_reader_spec.rb similarity index 76% rename from spec/labor/rss_reader_spec.rb rename to spec/services/rss_reader_spec.rb index b5e779c76..101258584 100644 --- a/spec/labor/rss_reader_spec.rb +++ b/spec/services/rss_reader_spec.rb @@ -21,21 +21,27 @@ RSpec.describe RssReader, vcr: vcr_option do it "fetch only articles from an feed_url" do described_class.get_all_articles - # the 16 here depends on the fixture + # the result within the approval file depends on the feed # not fetching comments is baked into this - expect(Article.all.length).to be > 10 + verify(format: :txt) { Article.count } end it "does not re-create article if it already exist" do described_class.new.get_all_articles - article_count_before = Article.all.length + expect { described_class.new.get_all_articles }.not_to change(Article, :count) + end + + it "parses correctly" do described_class.new.get_all_articles - expect(Article.all.length).to eq(article_count_before) + verify format: :txt do + User.find_by(feed_url: nonpermanent_link).articles.first.body_markdown + end end it "gets articles for user" do + # the result within the approval file depends on the feed described_class.new.fetch_user(User.first) - expect(Article.all.length).to be > 0 + verify(format: :txt) { Article.count } end it "does not set featured_number" do