Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fetch oldest authored_at correctly instead of oldest authored_at per page #4460

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/controllers/versions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class VersionsController < ApplicationController

def index
set_page
@oldest_version_date = @rubygem.versions.oldest_authored_at
@versions = @rubygem.versions.by_position.page(@page).per(Gemcutter::VERSIONS_PER_PAGE)
end

Expand Down
13 changes: 13 additions & 0 deletions app/models/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,19 @@ class Version < ApplicationRecord # rubocop:disable Metrics/ClassLength
validate :metadata_attribute_length
validate :no_dashes_in_version_number, on: :create

scope :oldest_authored_at, lambda {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to make this class level method instead of scope? Scope is usually used to return collection (is chainable).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah that makes sense, changed to a class method and moved closer to #authored_at

minimum(
<<~SQL.squish
CASE WHEN DATE(created_at) = '#{RUBYGEMS_IMPORT_DATE}'
AND built_at <= created_at THEN
built_at
ELSE
created_at
END
SQL
)&.in_time_zone
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimum query by itself returns a Time but #authored_at returns an ActiveSupport::TimeWithZone. So I added in_time_zone to convert the Time ➡️ ActiveSupport::TimeWithZone.

But I don't think it is strictly necessary because the equality check in the tests pass without the conversion and the rendering on the versions page is the same as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no timezone conversion currently happening in the codebase and all times are just printed to the page as stored in DB. IMHO not needed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, removed in_time_zone

}

class AuthorType < ActiveModel::Type::String
def cast_value(value)
if value.is_a?(Array)
Expand Down
2 changes: 1 addition & 1 deletion app/views/versions/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<p><%= t('.not_hosted_notice') %></p>
</div>
<% else %>
<h3 class="t-list__heading"><%= t('.versions_since', :count => @versions.total_count, :since => nice_date_for(@versions.map(&:authored_at).min)) %>:</h3>
<h3 class="t-list__heading"><%= t('.versions_since', :count => @versions.total_count, :since => nice_date_for(@oldest_version_date)) %>:</h3>
<div class="versions">
<ul class="t-list__items">
<%= render @versions %>
Expand Down
34 changes: 22 additions & 12 deletions test/functional/versions_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -114,27 +114,37 @@ class VersionsControllerTest < ActionController::TestCase
end

assert_select ".gem__version__date sup", text: "*", count: 1

assert_select ".t-list__heading", text: /1 version since January 01, 2000/, count: 1
end
end

context "on GET to index" do
setup do
@rubygem = create(:rubygem)
create(:version, number: "1.1.2", rubygem: @rubygem)
@oldest_created_at = Date.parse("2010-01-01")
create(:version, number: "1.1.2", rubygem: @rubygem, position: 0)
create(:version, number: "1.1.1", rubygem: @rubygem, position: 1, created_at: @oldest_created_at)
end

should "get paginated result" do
# first page includes the only version
get :index, params: { rubygem_id: @rubygem.name }

assert_response :success
assert page.has_content?("1.1.2")

# second page does not include the only version
get :index, params: { rubygem_id: @rubygem.name, page: 2 }

assert_response :success
refute page.has_content?("1.1.2")
stub_const(Gemcutter, :VERSIONS_PER_PAGE, 1) do
# first page only includes the version at position 0
get :index, params: { rubygem_id: @rubygem.name }

assert_response :success
assert page.has_content?("1.1.2")
refute page.has_content?("1.1.1")
assert_select ".t-list__heading", text: /2 versions since January 01, 2010/, count: 1

# second page only includes the version at position 1
get :index, params: { rubygem_id: @rubygem.name, page: 2 }

assert_response :success
refute page.has_content?("1.1.2")
assert page.has_content?("1.1.1")
assert_select ".t-list__heading", text: /2 versions since January 01, 2010/, count: 1
end
end
end

Expand Down
19 changes: 19 additions & 0 deletions test/models/version_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,25 @@ class VersionTest < ActiveSupport::TestCase
end
end

context ".oldest_authored_at scope" do
setup do
@built_at = Version::RUBYGEMS_IMPORT_DATE - 60.days
@version.update(built_at: @built_at, created_at: Version::RUBYGEMS_IMPORT_DATE)
_newer_version = create(:version, rubygem: @version.rubygem)
end

should "return the built_at of the oldest version when created_at is the same as RUBYGEMS_IMPORT_DATE" do
assert_equal @built_at, Version.oldest_authored_at
end

should "return the created_at of the oldest version when created_at is not the same as RUBYGEMS_IMPORT_DATE" do
created_at = Version::RUBYGEMS_IMPORT_DATE + 1.day
@version.update(created_at: created_at)

assert_equal created_at, Version.oldest_authored_at
end
end

should "have a rubygems version" do
@version.update(required_rubygems_version: @required_rubygems_version)
new_version = Version.find(@version.id)
Expand Down
Loading