diff --git a/app/liquid_tags/github_tag.rb b/app/liquid_tags/github_tag.rb index 78d3fa20e..168838209 100644 --- a/app/liquid_tags/github_tag.rb +++ b/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 diff --git a/app/liquid_tags/github_tag/github_readme_tag.rb b/app/liquid_tags/github_tag/github_readme_tag.rb index 73d5a5ff7..58c6ecc7e 100644 --- a/app/liquid_tags/github_tag/github_readme_tag.rb +++ b/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) diff --git a/app/liquid_tags/unified_embed/tag.rb b/app/liquid_tags/unified_embed/tag.rb index 6d2a21a64..e397bcf95 100644 --- a/app/liquid_tags/unified_embed/tag.rb +++ b/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 diff --git a/config/locales/liquid_tags/en.yml b/config/locales/liquid_tags/en.yml index 84d61451b..2e36db1ff 100644 --- a/config/locales/liquid_tags/en.yml +++ b/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 diff --git a/config/locales/liquid_tags/fr.yml b/config/locales/liquid_tags/fr.yml index c32e5ba05..1f3d36088 100644 --- a/config/locales/liquid_tags/fr.yml +++ b/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 diff --git a/spec/liquid_tags/unified_embed/registry_spec.rb b/spec/liquid_tags/unified_embed/registry_spec.rb index 777d0eddc..5d05c2220 100644 --- a/spec/liquid_tags/unified_embed/registry_spec.rb +++ b/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)) diff --git a/spec/liquid_tags/unified_embed/tag_spec.rb b/spec/liquid_tags/unified_embed/tag_spec.rb index 1c1441c1b..5968b012d 100644 --- a/spec/liquid_tags/unified_embed/tag_spec.rb +++ b/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"