Allow option for "no readme" with GitHub embeds (#16895)

* Implementation and specs

* update docs

* refactor

* remove unneeded commented code

* remove real auth string

* complete stubbing with vcr cassette

* clearer error messaging

* activate VCR conventional way

* added naming clarity

* more robust options checks

* update docs

* nudge Travis

* clearer docs

* fix failing specs

* fix boolean method

* damn code

app/liquid_tags/github_tag.rb

@@ -1,5 +1,5 @@
 class GithubTag < LiquidTagBase
-  REGISTRY_REGEXP = %r{https://github\.com/[\w\-.]{1,39}/[\w\-.]{1,39}/?((issues|pull)/\d+((#issuecomment-|#discussion_|#pullrequestreview-)\w+)?)?}
+  REGISTRY_REGEXP = %r{https://github\.com/[\w\-.]{1,39}/[\w\-.]{1,39}/?((issues|pull)/\d+((#issuecomment-|#discussion_|#pullrequestreview-)\w+)?)?(\sno-?readme\$)?}
 
   def initialize(_tag_name, link, _parse_context)
     super

app/liquid_tags/github_tag/github_readme_tag.rb

@@ -3,8 +3,7 @@ class GithubTag
     PARTIAL = "liquids/github_readme".freeze
     README_REGEXP = %r{https://github\.com/[\w\-.]{1,39}/[\w\-.]{1,39}/?}
     GITHUB_DOMAIN_REGEXP = %r{.*github.com/}
-    OPTION_NO_README = "no-readme".freeze
-    VALID_OPTIONS = [OPTION_NO_README].freeze
+    NOREADME_OPTIONS = %w[no-readme noreadme].freeze
 
     def initialize(input)
       @repository_path, @options = parse_input(input)
@@ -49,15 +48,15 @@ class GithubTag
 
     def validate_options!(*options)
       return if options.empty?
-      return if options.all? { |o| VALID_OPTIONS.include?(o) }
+      return if options.all? { |o| NOREADME_OPTIONS.include?(o) }
 
       message = I18n.t("liquid_tags.github_tag.github_readme_tag.invalid_options",
-                       invalid: (options - VALID_OPTIONS), valid: VALID_OPTIONS)
+                       invalid: (options - NOREADME_OPTIONS), valid: NOREADME_OPTIONS)
       raise StandardError, message
     end
 
     def show_readme?
-      options.none?(OPTION_NO_README)
+      options.none?
     end
 
     def fetch_readme(repository_path, repository_url)

app/liquid_tags/unified_embed/tag.rb

@@ -16,20 +16,23 @@ module UnifiedEmbed
     #
     # @param tag_name [String] in the UI, this was liquid tag name
     #        (e.g., `{% tag_name link %}`)
-    # @param link [String] the URL and additional options for that
-    #        particular service.
+    # @param input [String] the URL and additional options for that
+    #        particular embed.
     # @param parse_context [Liquid::ParseContext]
     #
     # @return [LiquidTagBase]
-    def self.new(tag_name, link, parse_context)
-      stripped_link = ActionController::Base.helpers.strip_tags(link).strip
+    def self.new(tag_name, input, parse_context)
+      stripped_input = ActionController::Base.helpers.strip_tags(input).strip
+
+      # This line handles the few instances where options are passed in with the embed URL
+      actual_link = stripped_input.split.length > 1 ? stripped_input.split[0] : stripped_input
 
       # Before matching against the embed registry, we check if the link
       # is valid (e.g. no typos).
       # If the link is invalid, we raise an error encouraging the user to
       # check their link and try again.
-      validated_link = validate_link(stripped_link)
-      klass = UnifiedEmbed::Registry.find_liquid_tag_for(link: validated_link)
+      validate_link!(actual_link)
+      klass = UnifiedEmbed::Registry.find_liquid_tag_for(link: stripped_input)
 
       # If the link is valid but doesn't match the registry, we return
       # an "unsupported URL" error. Eventually we shall render a fallback
@@ -41,10 +44,10 @@ module UnifiedEmbed
       # Why the __send__?  Because a LiquidTagBase class "privatizes"
       # the `.new` method.  And we want to instantiate the specific
       # liquid tag for the given link.
-      klass.__send__(:new, tag_name, validated_link, parse_context)
+      klass.__send__(:new, tag_name, stripped_input, parse_context)
     end
 
-    def self.validate_link(link)
+    def self.validate_link!(link)
       uri = URI.parse(link)
       http = Net::HTTP.new(uri.host, uri.port)
       http.use_ssl = true if http.port == 443
@@ -54,8 +57,6 @@ module UnifiedEmbed
       unless response.is_a?(Net::HTTPSuccess) || response.is_a?(Net::HTTPMovedPermanently)
         raise StandardError, I18n.t("liquid_tags.unified_embed.tag.not_found")
       end
-
-      link
     rescue SocketError
       raise StandardError, I18n.t("liquid_tags.unified_embed.tag.invalid_url")
     end

config/locales/liquid_tags/en.yml

@@ -27,7 +27,7 @@ en:
         invalid_github_issue_pull: Invalid GitHub issue, pull request or comment link
         posted_on: posted on
       github_readme_tag:
-        invalid_options: 'GitHub tag: invalid options: %{invalid} - supported options: %{valid}'
+        invalid_options: 'GitHub tag: invalid options: %{invalid} | supported options: %{valid}'
         invalid_github_repository: Invalid GitHub repository path or URL
     glitch_tag:
       invalid_glitch_id: Invalid Glitch ID

config/locales/liquid_tags/fr.yml

@@ -27,7 +27,7 @@ fr:
         invalid_github_issue_pull: Invalid GitHub issue, pull request or comment link
         posted_on: posted on
       github_readme_tag:
-        invalid_options: 'GitHub tag: invalid options: %{invalid} - supported options: %{valid}'
+        invalid_options: 'GitHub tag: invalid options: %{invalid} | supported options: %{valid}'
         invalid_github_repository: Invalid GitHub repository path or URL
     glitch_tag:
       invalid_glitch_id: Invalid Glitch ID

spec/liquid_tags/unified_embed/registry_spec.rb

@@ -156,6 +156,22 @@ RSpec.describe UnifiedEmbed::Registry do
         .to eq(GistTag)
     end
 
+    it "returns GithubTag for a github repository url (with or without option)", :aggregate_failures do
+      expect(described_class.find_liquid_tag_for(link: "https://github.com/forem/forem"))
+        .to eq(GithubTag)
+
+      expect(described_class.find_liquid_tag_for(link: "https://github.com/forem/forem noreadme"))
+        .to eq(GithubTag)
+    end
+
+    it "returns GithubTag for a github issue url", :aggregate_failures do
+      expect(described_class.find_liquid_tag_for(link: "https://github.com/forem/forem/issues/16673"))
+        .to eq(GithubTag)
+
+      expect(described_class.find_liquid_tag_for(link: "https://github.com/forem/forem/issues/16673#issue-1148186725"))
+        .to eq(GithubTag)
+    end
+
     it "returns GlitchTag for a valid glitch url", :aggregate_failures do
       valid_glitch_url_formats.each do |url|
         expect(described_class.find_liquid_tag_for(link: url))

spec/liquid_tags/unified_embed/tag_spec.rb

@@ -12,6 +12,20 @@ RSpec.describe UnifiedEmbed::Tag, type: :liquid_tag do
     expect(GistTag).to have_received(:new)
   end
 
+  it "delegates parsing to the link-matching class when there are options", vcr: true do
+    link = "https://github.com/rust-lang/rust"
+
+    allow(GithubTag).to receive(:new).and_call_original
+
+    VCR.use_cassette("github_client_repository_no_readme") do
+      stub_request_head(link)
+      parsed_tag = Liquid::Template.parse("{% embed #{link} noreadme %}")
+
+      expect { parsed_tag.render }.not_to raise_error
+      expect(GithubTag).to have_received(:new)
+    end
+  end
+
   it "raises an error when link 404s" do
     link = "https://takeonrules.com/goes-nowhere"