Fix sitemap ranking generation

Author: ciembor

lib/polish_open_source_rank/web/composition.rb

@@ -78,13 +78,10 @@ def package_ranking_read_model
       def sitemap_ranking_catalog
         contexts[:sitemap_ranking_catalog] ||= SitemapRankingCatalog.new(
           catalogs: [
-            PublicRankingSitemapCatalog.new(show_ranking_detail: publication.show_ranking_detail),
-            LanguageRankingSitemapCatalog.new(
-              show_language_ranking_detail: languages.show_language_ranking_detail
-            ),
+            PublicRankingSitemapCatalog.new,
+            LanguageRankingSitemapCatalog.new,
             PackageRankingSitemapCatalog.new(
-              package_ranking_read_model: package_ranking_read_model,
-              show_package_ranking_detail: packages.show_package_ranking_detail
+              package_ranking_read_model: package_ranking_read_model
             )
           ]
         )

lib/polish_open_source_rank/web/composition/sitemap_ranking_catalog.rb

@@ -3,39 +3,6 @@
 module PolishOpenSourceRank
   module Web
     class Composition
-      # Expands one canonical ranking path into every non-empty paginated URL.
-      module SitemapPageSeries
-        module_function
-
-        def paths(path)
-          paths = [path]
-          2.upto(Presentation::RankingPaginator::MAX_PAGE) do |page|
-            break if yield((page - 1) * Presentation::RankingPaginator::PER_PAGE).empty?
-
-            paths << "#{path}/page/#{page}"
-          end
-          paths
-        end
-      end
-
-      # Couples one canonical path with the query that determines its non-empty pages.
-      class SitemapQuerySeries
-        def initialize(path, query)
-          @path = path
-          @query = query
-        end
-
-        def paths(reader)
-          SitemapPageSeries.paths(path) do |offset|
-            reader.call(**query, limit: 1, offset: offset)
-          end
-        end
-
-        private
-
-        attr_reader :path, :query
-      end
-
       # Combines independently owned ranking URL inventories.
       class SitemapRankingCatalog
         def initialize(catalogs:)
@@ -57,16 +24,6 @@ class PublicRankingSitemapCatalog
         Route = Data.define(:kind, :metric, :suffix)
         # Binds a ranking route to its public prefix, geographic scope, and snapshot.
         Scope = Data.define(:prefix, :slug, :period_start) do
-          def series(route)
-            SitemapQuerySeries.new(
-              "#{prefix}/#{route.suffix}",
-              scope: slug,
-              kind: route.kind,
-              metric: route.metric,
-              period_start: period_start
-            )
-          end
-
           def people_prefix
             slug == 'poland' ? '/people' : "/people/locations/#{slug}"
           end
@@ -105,18 +62,12 @@ def ranking_scopes
           %w[organization-repositories trending repositories/trending]
         ].map { |values| Route.new(*values) }.freeze
 
-        def initialize(show_ranking_detail:)
-          @show_ranking_detail = show_ranking_detail
-        end
-
         def paths(latest_period:, period_slugs:)
           latest_paths(latest_period) + period_slugs.flat_map { |slug| historical_paths(slug) }
         end
 
         private
 
-        attr_reader :show_ranking_detail
-
         def latest_paths(period_start)
           latest_scope_paths(Scope.new(nil, 'poland', period_start)) +
             city_slugs.flat_map do |slug|
@@ -149,11 +100,7 @@ def historical_city_indexes(prefix)
         end
 
         def route_paths(scope, routes)
-          routes.flat_map { |route| route_page_paths(scope, route) }
-        end
-
-        def route_page_paths(scope, route)
-          scope.series(route).paths(show_ranking_detail)
+          routes.map { |route| "#{scope.prefix}/#{route.suffix}" }
         end
 
         def city_slugs
@@ -165,27 +112,17 @@ def city_slugs
       class LanguageRankingSitemapCatalog
         # Binds language ranking metrics to a public period prefix and snapshot.
         Period = Data.define(:prefix, :start) do
-          def series(metric)
-            SitemapQuerySeries.new(
-              "#{prefix}/languages/#{metric.slug}",
-              metric: metric.key,
-              period_start: start
-            )
+          def path(metric)
+            "#{prefix}/languages/#{metric.slug}"
           end
         end
 
-        def initialize(show_language_ranking_detail:)
-          @show_language_ranking_detail = show_language_ranking_detail
-        end
-
         def paths(latest_period:, period_slugs:)
           current_paths(latest_period) + period_slugs.flat_map { |slug| historical_paths(slug) }
         end
 
         private
 
-        attr_reader :show_language_ranking_detail
-
         def current_paths(latest_period)
           latest_period ? period_paths(Period.new('', latest_period)) : []
         end
@@ -201,7 +138,7 @@ def period_paths(period)
         end
 
         def metric_paths(period, metric)
-          period.series(metric).paths(show_language_ranking_detail)
+          period.path(metric)
         end
       end
 
@@ -215,27 +152,21 @@ def ecosystem(name)
         end
         # Owns package metric URL generation for one ecosystem and snapshot.
         Ecosystem = Data.define(:path, :name, :period_start) do
-          def paths(reader)
+          def paths
             [path] + Contexts::Packages::Domain::PackageRankingMetric.all(ecosystem: name).flat_map do |metric|
-              metric_paths(metric, reader)
+              metric_path(metric)
             end
           end
 
           private
 
-          def metric_paths(metric, reader)
-            SitemapQuerySeries.new(
-              "#{path}/#{metric.slug}",
-              ecosystem: name,
-              metric: metric.key,
-              period_start: period_start
-            ).paths(reader)
+          def metric_path(metric)
+            "#{path}/#{metric.slug}"
           end
         end
 
-        def initialize(package_ranking_read_model:, show_package_ranking_detail:)
+        def initialize(package_ranking_read_model:)
           @package_ranking_read_model = package_ranking_read_model
-          @show_package_ranking_detail = show_package_ranking_detail
         end
 
         def paths(latest_period:, period_slugs:)
@@ -244,7 +175,7 @@ def paths(latest_period:, period_slugs:)
 
         private
 
-        attr_reader :package_ranking_read_model, :show_package_ranking_detail
+        attr_reader :package_ranking_read_model
 
         def current_paths(latest_period)
           latest_period ? period_paths(Period.new('', latest_period)) : []
@@ -265,7 +196,7 @@ def ecosystems(period)
         end
 
         def ecosystem_paths(period, ecosystem)
-          period.ecosystem(ecosystem).paths(show_package_ranking_detail)
+          period.ecosystem(ecosystem).paths
         end
 
         def package_ranking_ecosystem?(ecosystem)

spec/polish_open_source_rank/web/app_spec.rb

@@ -1185,7 +1185,7 @@ def user(_access_token)
     expect(sitemap.content_type).to include('application/xml')
     sitemap_locations = REXML::XPath.match(xml_document(sitemap.body), '//url/loc').map(&:text)
     expect(sitemap_locations).to include('https://rank.example/people')
-    expect(sitemap_locations).to include('https://rank.example/people/users/top/page/2')
+    expect(sitemap_locations).not_to include('https://rank.example/people/users/top/page/2')
     expect(sitemap_locations).to include('https://rank.example/organizations')
     expect(sitemap_locations).to include('https://rank.example/organizations/active')
     expect(sitemap_locations).to include('https://rank.example/organizations/locations/krakow')

spec/polish_open_source_rank/web/composition/sitemap_ranking_catalog_spec.rb

@@ -9,35 +9,13 @@
       ecosystems: ['npm']
     )
   end
-  let(:show_ranking_detail) do
-    lambda do |scope:, kind:, metric:, offset:, **|
-      next [] unless scope == 'poland' && kind == 'users' && metric == 'top'
-
-      offset <= 200 ? [{ login: 'alice' }] : []
-    end
-  end
-  let(:show_language_ranking_detail) do
-    lambda do |metric:, offset:, **|
-      metric == 'repository_count' && offset == 100 ? [{ language: 'Ruby' }] : []
-    end
-  end
-  let(:show_package_ranking_detail) do
-    lambda do |ecosystem:, metric:, offset:, **|
-      ecosystem == 'npm' && metric == 'downloads_30d' && offset == 100 ? [{ package_name: 'rack' }] : []
-    end
-  end
   let(:catalog) do
     described_class.new(
       catalogs: [
-        PolishOpenSourceRank::Web::Composition::PublicRankingSitemapCatalog.new(
-          show_ranking_detail: show_ranking_detail
-        ),
-        PolishOpenSourceRank::Web::Composition::LanguageRankingSitemapCatalog.new(
-          show_language_ranking_detail: show_language_ranking_detail
-        ),
+        PolishOpenSourceRank::Web::Composition::PublicRankingSitemapCatalog.new,
+        PolishOpenSourceRank::Web::Composition::LanguageRankingSitemapCatalog.new,
         PolishOpenSourceRank::Web::Composition::PackageRankingSitemapCatalog.new(
-          package_ranking_read_model: package_ranking_read_model,
-          show_package_ranking_detail: show_package_ranking_detail
+          package_ranking_read_model: package_ranking_read_model
         )
       ]
     )
@@ -47,17 +25,18 @@
     allow(PolishOpenSourceRank::Contexts::Ranking::Domain::LocationCatalog).to receive(:city_slugs).and_return([])
   end
 
-  it 'adds every non-empty ranking page and stops at the first empty page', :aggregate_failures do
+  it 'adds canonical ranking pages without probing paginated result sets', :aggregate_failures do
     expect(paths).to include(
       '/people/users/top',
+      '/languages/top',
+      '/packages/npm/top'
+    )
+    expect(paths).not_to include(
       '/people/users/top/page/2',
       '/people/users/top/page/3',
       '/languages/top/page/2',
-      '/packages/npm/top/page/2'
-    )
-    expect(paths).not_to include(
-      '/people/users/top/page/4',
       '/languages/top/page/3',
+      '/packages/npm/top/page/2',
       '/packages/npm/top/page/3'
     )
   end