From b2b67a2b0dbeb35f7625dc00cd5c7156d469d9b2 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Sat, 26 Mar 2016 22:21:47 -0400 Subject: [PATCH 1/4] Introduce Links - version specific linkset `Links` uses version to generate urls that contain `version.number`. This logic is moved from `rubygems.rb` and `rubygems_helper.rb` to links It is essential a decorator, but for rss to use it, it is kept in "app/model" --- app/controllers/application_controller.rb | 4 ++ .../reverse_dependencies_controller.rb | 1 + app/controllers/rubygems_controller.rb | 1 + app/controllers/versions_controller.rb | 1 + app/helpers/rubygems_helper.rb | 9 --- app/models/links.rb | 67 +++++++++++++++++++ app/models/rubygem.rb | 17 +++-- app/models/version.rb | 4 -- app/views/rubygems/_aside.html.erb | 14 +--- test/functional/rubygems_controller_test.rb | 2 + test/functional/versions_controller_test.rb | 1 + test/unit/helpers/rubygems_helper_test.rb | 17 ----- test/unit/links_test.rb | 38 +++++++++++ test/unit/rubygem_test.rb | 5 +- 14 files changed, 131 insertions(+), 50 deletions(-) create mode 100644 app/models/links.rb create mode 100644 test/unit/links_test.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 003f90d831b..363357efacd 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -68,6 +68,10 @@ def find_rubygem end end + def find_versioned_links + @versioned_links = @rubygem.links(@latest_version) + end + def set_page @page = params[:page].respond_to?(:to_i) ? [1, params[:page].to_i].max : 1 end diff --git a/app/controllers/reverse_dependencies_controller.rb b/app/controllers/reverse_dependencies_controller.rb index 023ff1a49e9..b8610bef5ef 100644 --- a/app/controllers/reverse_dependencies_controller.rb +++ b/app/controllers/reverse_dependencies_controller.rb @@ -3,6 +3,7 @@ class ReverseDependenciesController < ApplicationController before_action :find_rubygem, only: [:index] before_action :latest_version, only: [:index] before_action :set_page, only: [:index] + before_action :find_versioned_links, only: [:index] def index @reverse_dependencies = @rubygem.reverse_dependencies diff --git a/app/controllers/rubygems_controller.rb b/app/controllers/rubygems_controller.rb index 020fc3ac0f9..2995693e0bd 100644 --- a/app/controllers/rubygems_controller.rb +++ b/app/controllers/rubygems_controller.rb @@ -4,6 +4,7 @@ class RubygemsController < ApplicationController before_action :set_blacklisted_gem, only: [:show], if: :blacklisted? before_action :find_rubygem, only: [:edit, :update, :show], unless: :blacklisted? before_action :latest_version, only: [:show], unless: :blacklisted? + before_action :find_versioned_links, only: [:show], unless: :blacklisted? before_action :load_gem, only: [:edit, :update] before_action :set_page, only: :index diff --git a/app/controllers/versions_controller.rb b/app/controllers/versions_controller.rb index 315db73a999..23269c983ab 100644 --- a/app/controllers/versions_controller.rb +++ b/app/controllers/versions_controller.rb @@ -8,6 +8,7 @@ def index def show @latest_version = Version.find_from_slug!(@rubygem.id, params[:id]) @versions = @rubygem.public_versions_with_extra_version(@latest_version) + @versioned_links = @rubygem.links(@latest_version) render "rubygems/show" end end diff --git a/app/helpers/rubygems_helper.rb b/app/helpers/rubygems_helper.rb index 88c9508425d..f26d813337f 100644 --- a/app/helpers/rubygems_helper.rb +++ b/app/helpers/rubygems_helper.rb @@ -72,19 +72,10 @@ def atom_link(rubygem) class: 'gem__link t-list__item', id: :rss end - def download_link(version) - link_to_page :download, "/downloads/#{version.full_name}.gem" - end - def reverse_dependencies_link(rubygem) link_to_page :reverse_dependencies, rubygem_reverse_dependencies_path(rubygem) end - def documentation_link(version, linkset) - return unless linkset.nil? || linkset.docs.blank? - link_to_page :docs, version.documentation_path - end - def badge_link(rubygem) badge_url = "https://badge.fury.io/rb/#{rubygem.name}/install" link_to t(".links.badge"), badge_url, class: "gem__link t-list__item", id: :badge diff --git a/app/models/links.rb b/app/models/links.rb new file mode 100644 index 00000000000..c3fe96cfe0d --- /dev/null +++ b/app/models/links.rb @@ -0,0 +1,67 @@ +class Links + # Links available for indexed gems + LINKS = { + 'home' => 'homepage_uri', + 'code' => 'source_code_uri', + 'docs' => 'documentation_uri', + 'wiki' => 'wiki_uri', + 'mail' => 'mailing_list_uri', + 'bugs' => 'bug_tracker_uri', + 'download' => 'download_uri' + }.freeze + + # Links available for non-indexed gems + NON_INDEXED_LINKS = { + 'docs' => 'documentation_uri' + }.freeze + + attr_accessor :rubygem, :version, :linkset + + def initialize(rubygem = nil, version = nil) + self.rubygem = rubygem + self.version = version + self.linkset = rubygem.linkset + end + + def links + version.indexed ? LINKS : NON_INDEXED_LINKS + end + + delegate :keys, to: :links + + def each + return enum_for(:each) unless block_given? + links.each do |short, long| + value = send(long) + yield short, value if value + end + end + + # documentation uri: + # if metadata has it defined, use that + # or if linksets has it defined, use that + # else, generate one from gem name and version number + def documentation_uri + (linkset && linkset.docs.presence) || "http://www.rubydoc.info/gems/#{rubygem.name}/#{version.number}" + end + + # technically this is a path + def download_uri + "/downloads/#{version.full_name}.gem" if version.indexed + end + + def badge_uri + "https://badge.fury.io/rb/#{rubygem.name}/install" + end + + # define getters for each of the uris (both short `home` or long `homepage_uri` versions) + # don't define for download_uri since it has special logic and is already defined + LINKS.each do |short, long| + unless method_defined?(long) + define_method(long) do + linkset && linkset.public_send(short) + end + end + alias_method short, long + end +end diff --git a/app/models/rubygem.rb b/app/models/rubygem.rb index 0bfc99908d1..24469dabc8d 100644 --- a/app/models/rubygem.rb +++ b/app/models/rubygem.rb @@ -141,7 +141,12 @@ def downloads gem_download.try(:count) || 0 end + def links(version = versions.most_recent) + Links.new(self, version) + end + def payload(version = versions.most_recent, protocol = Gemcutter::PROTOCOL, host_with_port = Gemcutter::HOST) + versioned_links = links(version) deps = version.dependencies.to_a { 'name' => name, @@ -156,12 +161,12 @@ def payload(version = versions.most_recent, protocol = Gemcutter::PROTOCOL, host 'sha' => version.sha256_hex, 'project_uri' => "#{protocol}://#{host_with_port}/gems/#{name}", 'gem_uri' => "#{protocol}://#{host_with_port}/gems/#{version.full_name}.gem", - 'homepage_uri' => linkset.try(:home), - 'wiki_uri' => linkset.try(:wiki), - 'documentation_uri' => linkset.try(:docs).presence || version.documentation_path, - 'mailing_list_uri' => linkset.try(:mail), - 'source_code_uri' => linkset.try(:code), - 'bug_tracker_uri' => linkset.try(:bugs), + 'homepage_uri' => versioned_links.homepage_uri, + 'wiki_uri' => versioned_links.wiki_uri, + 'documentation_uri' => versioned_links.documentation_uri, + 'mailing_list_uri' => versioned_links.mailing_list_uri, + 'source_code_uri' => versioned_links.source_code_uri, + 'bug_tracker_uri' => versioned_links.bug_tracker_uri, 'dependencies' => { 'development' => deps.select { |r| r.rubygem && 'development' == r.scope }, 'runtime' => deps.select { |r| r.rubygem && 'runtime' == r.scope } diff --git a/app/models/version.rb b/app/models/version.rb index e8eed164c47..7e94ad0b0f0 100644 --- a/app/models/version.rb +++ b/app/models/version.rb @@ -350,10 +350,6 @@ def assign_required_rubygems_version! update_column(:required_rubygems_version, required_rubygems_version.to_s) end - def documentation_path - "http://www.rubydoc.info/gems/#{rubygem.name}/#{number}" - end - private def get_spec_attribute(attribute_name) diff --git a/app/views/rubygems/_aside.html.erb b/app/views/rubygems/_aside.html.erb index dc4ba21572e..6e19722d85c 100644 --- a/app/views/rubygems/_aside.html.erb +++ b/app/views/rubygems/_aside.html.erb @@ -61,17 +61,9 @@
<%= link_to t('edit'), edit_rubygem_path(@rubygem), :class => "gem__link t-list__item", :id => "edit" if @rubygem.owned_by?(current_user) %> - <% if @latest_version.indexed %> - <% if @rubygem.linkset.present? %> - <%- Linkset::LINKS.each do |link| %> - <%= link_to_page link, @rubygem.linkset.public_send(link) %> - <%- end %> - <% end %> - - <%= download_link(@latest_version) %> - <% end %> - - <%= documentation_link(@latest_version, @rubygem.linkset) %> + <%- @versioned_links.each do |name, link| %> + <%= link_to_page name, link %> + <%- end %> <%= badge_link(@rubygem) %> <%= subscribe_link(@rubygem) if @latest_version.indexed %> <%= unsubscribe_link(@rubygem) %> diff --git a/test/functional/rubygems_controller_test.rb b/test/functional/rubygems_controller_test.rb index 4213a7d42b7..63a6f9be68d 100644 --- a/test/functional/rubygems_controller_test.rb +++ b/test/functional/rubygems_controller_test.rb @@ -267,6 +267,7 @@ class RubygemsControllerTest < ActionController::TestCase setup do @latest_version = create(:version, created_at: 1.minute.ago) @rubygem = @latest_version.rubygem + @versioned_links = @rubygem.links(@latest_version) get :show, id: @rubygem.to_param end @@ -285,6 +286,7 @@ class RubygemsControllerTest < ActionController::TestCase setup do @latest_version = create(:version) @rubygem = @latest_version.rubygem + @versioned_links = @rubygem.links(@latest_version) end should "render plural licenses header for other than one license" do @latest_version.update_attributes(licenses: nil) diff --git a/test/functional/versions_controller_test.rb b/test/functional/versions_controller_test.rb index af6c0b54acd..c1c7f5906aa 100644 --- a/test/functional/versions_controller_test.rb +++ b/test/functional/versions_controller_test.rb @@ -69,6 +69,7 @@ class VersionsControllerTest < ActionController::TestCase setup do @latest_version = create(:version, built_at: 1.week.ago, created_at: 1.day.ago) @rubygem = @latest_version.rubygem + @versioned_links = @rubygem.links(@latest_version) @versions = (1..5).map do FactoryGirl.create(:version, rubygem: @rubygem) end diff --git a/test/unit/helpers/rubygems_helper_test.rb b/test/unit/helpers/rubygems_helper_test.rb index d3b79e85af5..8247036348f 100644 --- a/test/unit/helpers/rubygems_helper_test.rb +++ b/test/unit/helpers/rubygems_helper_test.rb @@ -56,23 +56,6 @@ class RubygemsHelperTest < ActionView::TestCase assert_equal "March 18, 2011", nice_date_for(time) end - should "link to docs if no docs link is set" do - version = build(:version) - linkset = build(:linkset, docs: nil) - - @virtual_path = "rubygems.show" - link = documentation_link(version, linkset) - assert link.include?(version.documentation_path) - end - - should "not link to docs if docs link is set" do - version = build(:version) - linkset = build(:linkset) - - link = documentation_link(version, linkset) - assert link.blank? - end - should "link to the badge" do rubygem = create(:rubygem) url = "https://badge.fury.io/rb/#{rubygem.name}/install" diff --git a/test/unit/links_test.rb b/test/unit/links_test.rb new file mode 100644 index 00000000000..14e7d7ea51c --- /dev/null +++ b/test/unit/links_test.rb @@ -0,0 +1,38 @@ +require 'test_helper' + +class LinksTest < ActiveSupport::TestCase + # #documentation_uri + should "use linkset documentation_uri" do + version = build(:version) + rubygem = build(:rubygem, linkset: build(:linkset, docs: "http://example.com/doc"), versions: [version]) + links = rubygem.links(version) + + assert_match "http://example.com/doc", links.documentation_uri + end + + should "fallback to rubygems documentation_uri" do + version = build(:version) + rubygem = build(:rubygem, linkset: build(:linkset, docs: nil), versions: [version]) + links = rubygem.links(version) + + assert_equal "http://www.rubydoc.info/gems/#{rubygem.name}/#{version.number}", links.documentation_uri + end + + should "use all fields when indexed" do + version = build(:version, indexed: true) + rubygem = build(:rubygem, linkset: build(:linkset, docs: nil), versions: [version]) + links = rubygem.links(version) + + assert links.links.keys.include?('home') + assert links.links.keys.include?('docs') + end + + should "use partial fields when not indexed" do + version = build(:version, indexed: false) + rubygem = build(:rubygem, linkset: build(:linkset, docs: nil), versions: [version]) + links = rubygem.links(version) + + refute links.links.keys.include?('home') + assert links.links.keys.include?('docs') + end +end diff --git a/test/unit/rubygem_test.rb b/test/unit/rubygem_test.rb index c63231887f7..dc0c79ff2da 100644 --- a/test/unit/rubygem_test.rb +++ b/test/unit/rubygem_test.rb @@ -471,12 +471,11 @@ class RubygemTest < ActiveSupport::TestCase assert_equal @rubygem.linkset.bugs, hash["bug_tracker_uri"] end - should "return version documentation url if linkset docs is empty" do + should "return version documentation uri if linkset docs is empty" do @rubygem.linkset.docs = "" - @rubygem.save hash = JSON.load(@rubygem.to_json) - assert_equal @version.documentation_path, hash["documentation_uri"] + assert_equal "http://www.rubydoc.info/gems/#{@rubygem.name}/#{@version.number}", hash["documentation_uri"] end should "return a bunch of XML" do From a5c4e269a56038a58e62b8becd3abc2e0c31c87e Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Sun, 18 Oct 2015 16:36:55 -0400 Subject: [PATCH 2/4] ruby gems api uses metadata or linkset uris --- app/models/links.rb | 6 ++++-- test/unit/rubygem_test.rb | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/app/models/links.rb b/app/models/links.rb index c3fe96cfe0d..b8b43928257 100644 --- a/app/models/links.rb +++ b/app/models/links.rb @@ -42,7 +42,9 @@ def each # or if linksets has it defined, use that # else, generate one from gem name and version number def documentation_uri - (linkset && linkset.docs.presence) || "http://www.rubydoc.info/gems/#{rubygem.name}/#{version.number}" + (version.metadata && version.metadata["documentation_uri"].presence) || + (linkset && linkset.docs.presence) || + "http://www.rubydoc.info/gems/#{rubygem.name}/#{version.number}" end # technically this is a path @@ -59,7 +61,7 @@ def badge_uri LINKS.each do |short, long| unless method_defined?(long) define_method(long) do - linkset && linkset.public_send(short) + (version.metadata && version.metadata[long].presence) || (linkset && linkset.public_send(short)) end end alias_method short, long diff --git a/test/unit/rubygem_test.rb b/test/unit/rubygem_test.rb index dc0c79ff2da..78e9c2c18e0 100644 --- a/test/unit/rubygem_test.rb +++ b/test/unit/rubygem_test.rb @@ -454,6 +454,44 @@ class RubygemTest < ActiveSupport::TestCase assert_equal run_dep.name, doc.at_css("dependencies runtime dependency name").content end + context "with metadata" do + setup do + @version = create(:version, rubygem: @rubygem) + @rubygem = build(:rubygem, versions: [@version]) + end + + should "prefer metadata over links in JSON" do + @version.update_attributes!( + metadata: { + "homepage_uri" => "http://example.com/home", + "wiki_uri" => "http://example.com/wiki", + "documentation_uri" => "http://example.com/docs", + "mailing_list_uri" => "http://example.com/mail", + "source_code_uri" => "http://example.com/code", + "bug_tracker_uri" => "http://example.com/bugs" + } + ) + + hash = MultiJson.load(@rubygem.to_json) + + assert_equal "http://example.com/home", hash["homepage_uri"] + assert_equal "http://example.com/wiki", hash["wiki_uri"] + assert_equal "http://example.com/docs", hash["documentation_uri"] + assert_equal "http://example.com/mail", hash["mailing_list_uri"] + assert_equal "http://example.com/code", hash["source_code_uri"] + assert_equal "http://example.com/bugs", hash["bug_tracker_uri"] + end + + should "return version documentation url if metadata and linkset docs is empty" do + @version.update_attributes!(metadata: {}) + @rubygem.linkset.update_attributes(:docs, "") if @rubygem.linkset + @rubygem.reload + hash = JSON.load(@rubygem.to_json) + + assert_equal "http://www.rubydoc.info/gems/#{@rubygem.name}/#{@version.number}", hash["documentation_uri"] + end + end + context "with a linkset" do setup do @rubygem = build(:rubygem) From 1703140e102dcf067fdbdd1d93184f038e4d3a80 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Mon, 26 Dec 2016 16:55:57 -0500 Subject: [PATCH 3/4] rubygem_test rubocop - &. --- test/unit/rubygem_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/rubygem_test.rb b/test/unit/rubygem_test.rb index 78e9c2c18e0..2806552c57e 100644 --- a/test/unit/rubygem_test.rb +++ b/test/unit/rubygem_test.rb @@ -484,7 +484,7 @@ class RubygemTest < ActiveSupport::TestCase should "return version documentation url if metadata and linkset docs is empty" do @version.update_attributes!(metadata: {}) - @rubygem.linkset.update_attributes(:docs, "") if @rubygem.linkset + @rubygem.linkset&.update_attributes(:docs, "") @rubygem.reload hash = JSON.load(@rubygem.to_json) From 5c37cc9cd96dd572e7339fa984a4d352215ad4c9 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 28 Dec 2016 10:56:00 -0500 Subject: [PATCH 4/4] links#version and metadata is not nil --- app/models/links.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/links.rb b/app/models/links.rb index b8b43928257..c4b233bcbde 100644 --- a/app/models/links.rb +++ b/app/models/links.rb @@ -17,7 +17,7 @@ class Links attr_accessor :rubygem, :version, :linkset - def initialize(rubygem = nil, version = nil) + def initialize(rubygem, version) self.rubygem = rubygem self.version = version self.linkset = rubygem.linkset @@ -42,8 +42,8 @@ def each # or if linksets has it defined, use that # else, generate one from gem name and version number def documentation_uri - (version.metadata && version.metadata["documentation_uri"].presence) || - (linkset && linkset.docs.presence) || + version.metadata["documentation_uri"].presence || + linkset&.docs.presence || "http://www.rubydoc.info/gems/#{rubygem.name}/#{version.number}" end @@ -52,16 +52,12 @@ def download_uri "/downloads/#{version.full_name}.gem" if version.indexed end - def badge_uri - "https://badge.fury.io/rb/#{rubygem.name}/install" - end - # define getters for each of the uris (both short `home` or long `homepage_uri` versions) # don't define for download_uri since it has special logic and is already defined LINKS.each do |short, long| unless method_defined?(long) define_method(long) do - (version.metadata && version.metadata[long].presence) || (linkset && linkset.public_send(short)) + version.metadata[long].presence || linkset&.public_send(short) end end alias_method short, long