Fix legacy sitemap logic (#14926)

* Fix legacy sitemap logic

* Add aggregate failures

* Fix formatting

* Adjust robots.txt spec

* Fix index minimum to >=

Gemfile

@@ -100,7 +100,6 @@ gem "s3_direct_upload", "~> 0.1" # Direct Upload to Amazon S3
 gem "sidekiq", "~> 6.2.2" # Sidekiq is used to process background jobs with the help of Redis
 gem "sidekiq-cron", "~> 1.1" # Allows execution of scheduled cron jobs as specific times
 gem "sidekiq-unique-jobs", "~> 7.0.12" # Ensures that Sidekiq jobs are unique when enqueued
-gem "sitemap_generator", "~> 6.1" # SitemapGenerator is a framework-agnostic XML Sitemap generator
 gem "slack-notifier", "~> 2.4" # A slim ruby wrapper for posting to slack webhooks
 gem "sprockets", "~> 4.0" # Sprockets is a Rack-based asset packaging system
 gem "staccato", "~> 0.5" # Ruby Google Analytics Measurement

Gemfile.lock

@@ -759,8 +759,6 @@ GEM
       simplecov_json_formatter (~> 0.1)
     simplecov-html (0.12.3)
     simplecov_json_formatter (0.1.2)
-    sitemap_generator (6.1.2)
-      builder (~> 3.0)
     slack-notifier (2.4.0)
     smart_properties (1.15.0)
     spring (3.0.0)
@@ -996,7 +994,6 @@ DEPENDENCIES
   sidekiq-cron (~> 1.1)
   sidekiq-unique-jobs (~> 7.0.12)
   simplecov (~> 0.21.2)
-  sitemap_generator (~> 6.1)
   slack-notifier (~> 2.4)
   spring (~> 3.0)
   spring-commands-rspec (~> 1.0)

app/controllers/sitemaps_controller.rb

@@ -5,22 +5,46 @@ class SitemapsController < ApplicationController
   RESULTS_LIMIT = Rails.env.production? ? 10_000 : 5
 
   def show
-    if params[:sitemap].start_with? "sitemap-posts"
-      posts_sitemap
+    if params[:sitemap].start_with? "sitemap-index"
+      sitemap_index
+    elsif valid_resource_sitemap?
+      resource_sitemap(resource_string)
     else
       monthly_sitemap
     end
     set_cache_control_headers(@max_age,
                               stale_while_revalidate: @stale_while_revalidate,
                               stale_if_error: @stale_if_error)
-    render layout: false
+    render @view_template, layout: false
   end
 
   private
 
-  def posts_sitemap
-    @articles = Article.published.order("published_at DESC").limit(RESULTS_LIMIT).offset(offset).pluck(:path, :last_comment_at)
-    set_surrogate_controls(Time.now)
+  def sitemap_index
+    set_surrogate_controls(Time.current)
+    @articles_count = Article.published
+      .where("score >= ?", Settings::UserExperience.index_minimum_score).size
+    @page_limit = RESULTS_LIMIT
+    @view_template = "index"
+  end
+
+  def resource_sitemap(resource)
+    case resource
+    when "users"
+      @users = User.order("comments_count DESC")
+        .where("score > -1") # Spam mitigation
+        .limit(RESULTS_LIMIT).offset(offset).pluck(:username, :profile_updated_at)
+    when "posts"
+      @articles = Article.published.order("published_at DESC")
+        .where("score >= ?", Settings::UserExperience.index_minimum_score)
+        .limit(RESULTS_LIMIT).offset(offset).pluck(:path, :last_comment_at)
+    when "tags" # tags
+      @tags = Tag.order("hotness_score DESC")
+        .where(supported: true)
+        .limit(RESULTS_LIMIT).offset(offset).pluck(:name, :updated_at)
+    end
+    set_surrogate_controls(Time.current)
+    @view_template = resource
   end
 
   def monthly_sitemap
@@ -33,11 +57,12 @@ class SitemapsController < ApplicationController
     end
 
     @articles = Article.published
-      .where("published_at > ? AND published_at < ? AND score > ?",
+      .where("published_at > ? AND published_at < ? AND score >= ?",
              date, date.end_of_month, Settings::UserExperience.index_minimum_score)
       .pluck(:path, :last_comment_at)
 
     set_surrogate_controls(date)
+    @view_template = "posts"
   end
 
   def set_surrogate_controls(date)
@@ -51,6 +76,14 @@ class SitemapsController < ApplicationController
     end
   end
 
+  def valid_resource_sitemap?
+    %w[posts users tags].include?(resource_string)
+  end
+
+  def resource_string
+    params[:sitemap].gsub(".xml","").split("-")[1]
+  end
+
   def offset
     params[:sitemap].split("-")[2].to_i * RESULTS_LIMIT # elvaluates to 0 if not present or not a number
   end

app/views/pages/robots.text.erb

@@ -11,4 +11,4 @@ Disallow: /mod/*
 Disallow: /mod?*
 Disallow: /admin/*
 
-Sitemap: https://<%= ApplicationConfig["AWS_BUCKET_NAME"] %>.s3.amazonaws.com/sitemaps/sitemap.xml.gz
+Sitemap: <%= URL.url("sitemap-index.xml") %>

app/views/sitemaps/index.xml.erb

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
+  <% if @articles_count > @page_limit %>
+    <% (0..(@articles_count/@page_limit)).each do |n| %>
+      <sitemap>
+          <loc><%= app_url("sitemap-posts-#{n}.xml") %></loc>
+      </sitemap>
+    <% end %>
+  <% else %>
+      <sitemap>
+        <loc><%= app_url("sitemap-posts.xml") %></loc>
+    </sitemap>
+  <% end %>
+  <sitemap>
+      <loc><%= app_url("sitemap-users.xml") %></loc>
+  </sitemap>
+  <sitemap>
+      <loc><%= app_url("sitemap-tags.xml") %></loc>
+  </sitemap>
+</sitemapindex>

app/views/sitemaps/tags.xml.erb

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
+  <% @tags.each do |name, updated_at| %>
+    <url>
+        <loc><%= app_url("/t/#{name}") %></loc>
+        <lastmod><%= updated_at.strftime("%F") %></lastmod>
+    </url>
+  <% end %>
+</urlset>

app/views/sitemaps/users.xml.erb

@@ -0,0 +1,9 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">
+  <% @users.each do |username, updated_at| %>
+    <url>
+        <loc><%= app_url(username) %></loc>
+        <lastmod><%= updated_at.strftime("%F") %></lastmod>
+    </url>
+  <% end %>
+</urlset>

app/workers/sitemap_refresh_worker.rb

@@ -1,13 +0,0 @@
-Rails.application.load_tasks
-
-class SitemapRefreshWorker
-  include Sidekiq::Worker
-
-  sidekiq_options queue: :low_priority, retry: 10
-
-  def perform
-    sitemap_task = ForemInstance.local? ? "sitemap:refresh:no_ping" : "sitemap:refresh"
-
-    Rake::Task[sitemap_task].invoke
-  end
-end

config/sitemap.rb

@@ -1,53 +0,0 @@
-require "sitemap_generator/s3_adapter"
-
-# @forem/systems: It's fine if this doesn't 100% work correctly right now, as long as it doesn't break stuff at runtime.
-
-if Rails.env.production?
-  region = ApplicationConfig["AWS_UPLOAD_REGION"].presence || ApplicationConfig["AWS_DEFAULT_REGION"]
-  s3_config_hash = if ENV["FOREM_CONTEXT"] == "forem_cloud" # @forem/systems jdoss's special sauce.
-                     # Excluding the aws_access_key_id and aws_secret_access_key causes use_iam_profile
-                     # to be set to true by the S3Adapter
-                     # https://github.com/kjvarga/sitemap_generator/blob/0b847f1e7a544ea8ef87bb643a732e30a07a14c9/lib/sitemap_generator/adapters/s3_adapter.rb#L39
-                     {
-                       fog_provider: "AWS",
-                       fog_directory: ApplicationConfig["AWS_BUCKET_NAME"],
-                       fog_region: "us-east-2",
-                       fog_public: false
-                     }
-                   elsif %w[AWS_ID AWS_SECRET AWS_BUCKET_NAME].all? { |key| ApplicationConfig[key] }
-                     {
-                       fog_provider: "AWS",
-                       aws_access_key_id: ApplicationConfig["AWS_ID"],
-                       aws_secret_access_key: ApplicationConfig["AWS_SECRET"],
-                       fog_directory: ApplicationConfig["AWS_BUCKET_NAME"],
-                       fog_region: region
-                     }
-                   end
-
-  if s3_config_hash
-    SitemapGenerator::Sitemap.adapter = SitemapGenerator::S3Adapter.new(s3_config_hash)
-    SitemapGenerator::Sitemap.sitemaps_host = "https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/"
-    SitemapGenerator::Sitemap.public_path = "tmp/"
-  else
-    SitemapGenerator::Sitemap.adapter = SitemapGenerator::FileAdapter.new
-    SitemapGenerator::Sitemap.public_path = "public/"
-  end
-  SitemapGenerator::Sitemap.sitemaps_path = "sitemaps/"
-end
-
-SitemapGenerator::Sitemap.default_host = "#{ApplicationConfig['APP_PROTOCOL']}#{ApplicationConfig['APP_DOMAIN']}"
-
-SitemapGenerator::Sitemap.create do
-  Article.published.where("score > ? OR featured = ?", 12, true)
-    .limit(38_000).find_each do |article|
-    add article.path, lastmod: article.last_comment_at, changefreq: "daily"
-  end
-
-  User.order(comments_count: :desc).where("updated_at > ?", 5.days.ago).limit(8000).find_each do |user|
-    add "/#{user.username}", changefreq: "daily"
-  end
-
-  Tag.order(hotness_score: :desc).limit(250).find_each do |tag|
-    add "/t/#{tag.name}", changefreq: "daily"
-  end
-end

spec/requests/pages_spec.rb

@@ -231,7 +231,7 @@ RSpec.describe "Pages", type: :request do
     it "has proper text" do
       get "/robots.txt"
 
-      text = "Sitemap: https://#{ApplicationConfig['AWS_BUCKET_NAME']}.s3.amazonaws.com/sitemaps/sitemap.xml.gz"
+      text = "Sitemap: #{URL.url("sitemap-index.xml")}"
       expect(response.body).to include(text)
     end
   end

spec/requests/sitemaps_spec.rb

@@ -41,38 +41,127 @@ RSpec.describe "Sitemaps", type: :request do
       expect(response.media_type).to eq("application/xml")
     end
 
-    it "renders most recent posts if /sitemap-posts", :aggregate_failures do
-      create_list(:article, 8)
-      get "/sitemap-posts.xml"
-      expect(response.body).to include(Article.order("published_at DESC").first.path)
-      expect(response.body).not_to include(Article.order("published_at DESC").last.path)
+    context "with index in param" do
+      it "renders basic index", :aggregate_failures do
+        get "/sitemap-index.xml"
+        expect(response.body).to include("<sitemapindex xmlns=")
+        expect(response.body).to include("sitemap-posts.xml")
+        expect(response.body).to include("sitemap-users.xml")
+        expect(response.body).to include("sitemap-tags.xml")
+      end
+
+      it "renders multiple posts pages if enough posts", :aggregate_failures do
+        create_list(:article, 13, score: 10)
+        get "/sitemap-index.xml"
+        expect(response.body).not_to include("sitemap-posts.xml")
+        expect(response.body).to include("sitemap-posts-0.xml")
+        expect(response.body).to include("sitemap-posts-2.xml")
+        expect(response.body).not_to include("sitemap-posts-3.xml")
+      end
     end
 
-    it "renders second page if /sitemap-posts-1", :aggregate_failures do
-      create_list(:article, 8)
-      get "/sitemap-posts-1.xml"
-      expect(response.body).not_to include(Article.order("published_at DESC").first.path)
-      expect(response.body).to include(Article.order("published_at DESC").last.path)
+    context "with posts in param" do
+      before do
+        create_list(:article, 8, score: 10)
+      end
+
+      it "renders most recent posts if /sitemap-posts", :aggregate_failures do
+        get "/sitemap-posts.xml"
+        expect(response.body).to include(Article.order("published_at DESC").first.path)
+        expect(response.body).not_to include(Article.order("published_at DESC").last.path)
+      end
+
+      it "renders second page if /sitemap-posts-1", :aggregate_failures do
+        get "/sitemap-posts-1.xml"
+        expect(response.body).not_to include(Article.order("published_at DESC").first.path)
+        expect(response.body).to include(Article.order("published_at DESC").last.path)
+      end
+
+      it "renders first page if /sitemap-posts-randomn0tnumber", :aggregate_failures do
+        get "/sitemap-posts-randomn0tnumber.xml"
+        expect(response.body).to include(Article.order("published_at DESC").first.path)
+        expect(response.body).not_to include(Article.order("published_at DESC").last.path)
+      end
+
+      it "renders empty if /sitemap-posts-2", :aggregate_failures do
+        # no posts this far down.
+        get "/sitemap-posts-2.xml"
+        expect(response.body).not_to include(Article.order("published_at DESC").first.path)
+        expect(response.body).not_to include(Article.order("published_at DESC").last.path)
+      end
+
+      it "renders 'recent' version of surrogate control" do
+        get "/sitemap-posts-2.xml"
+        expect(response.header["Surrogate-Control"]).to include("8640")
+      end
     end
 
-    it "renders first page if /sitemap-posts-randomn0tnumber", :aggregate_failures do
-      create_list(:article, 8)
-      get "/sitemap-posts-randomn0tnumber.xml"
-      expect(response.body).to include(Article.order("published_at DESC").first.path)
-      expect(response.body).not_to include(Article.order("published_at DESC").last.path)
+    context "with tags in param" do
+      before do
+        create_list(:tag, 8)
+        Tag.all.each do |tag|
+          tag.update_column(:hotness_score, rand(100_000))
+        end
+      end
+
+      it "renders hottest tags if /sitemap-tags", :aggregate_failures do
+        get "/sitemap-tags.xml"
+        expect(response.body).to include(Tag.order("hotness_score DESC").first.name)
+        expect(response.body).not_to include(Tag.order("hotness_score DESC").last.name)
+      end
+
+      it "renders second page if /sitemap-tags-1", :aggregate_failures do
+        get "/sitemap-tags-1.xml"
+        expect(response.body).not_to include(Tag.order("hotness_score DESC").first.name)
+        expect(response.body).to include(Tag.order("hotness_score DESC").last.name)
+      end
+
+      it "renders first page if /sitemap-tags-randomn0tnumber", :aggregate_failures do
+        get "/sitemap-tags-randomn0tnumber.xml"
+        expect(response.body).to include(Tag.order("hotness_score DESC").first.name)
+        expect(response.body).not_to include(Tag.order("hotness_score DESC").last.name)
+      end
+
+      it "renders empty if /sitemap-tags-2", :aggregate_failures do
+        # no posts this far down.
+        get "/sitemap-tags-2.xml"
+        expect(response.body).not_to include(Tag.order("hotness_score DESC").first.name)
+        expect(response.body).not_to include(Tag.order("hotness_score DESC").last.name)
+      end
     end
 
-    it "renders empty if /sitemap-posts-2", :aggregate_failures do
-      # no posts this far down.
-      create_list(:article, 8)
-      get "/sitemap-posts-2.xml"
-      expect(response.body).not_to include(Article.order("published_at DESC").first.path)
-      expect(response.body).not_to include(Article.order("published_at DESC").last.path)
-    end
+    context "with users in param" do
+      before do
+        create_list(:user, 8)
+        User.all.each do |user|
+          user.update_column(:comments_count, rand(100_000))
+        end
+      end
 
-    it "renders 'recent' version of surrogate control" do
-      get "/sitemap-posts-2.xml"
-      expect(response.header["Surrogate-Control"]).to include("8640")
+      it "renders hottest tags if /sitemap-users", :aggregate_failures do
+        get "/sitemap-users.xml"
+        expect(response.body).to include(User.order("comments_count DESC").first.username)
+        expect(response.body).not_to include(User.order("comments_count DESC").last.username)
+      end
+
+      it "renders second page if /sitemap-users-1", :aggregate_failures do
+        get "/sitemap-users-1.xml"
+        expect(response.body).not_to include(User.order("comments_count DESC").first.username)
+        expect(response.body).to include(User.order("comments_count DESC").last.username)
+      end
+
+      it "renders first page if /sitemap-users-randomn0tnumber", :aggregate_failures do
+        get "/sitemap-users-randomn0tnumber.xml"
+        expect(response.body).to include(User.order("comments_count DESC").first.username)
+        expect(response.body).not_to include(User.order("comments_count DESC").last.username)
+      end
+
+      it "renders empty if /sitemap-users-2", :aggregate_failures do
+        # no posts this far down.
+        get "/sitemap-users-2.xml"
+        expect(response.body).not_to include(User.order("comments_count DESC").first.username)
+        expect(response.body).not_to include(User.order("comments_count DESC").last.username)
+      end
     end
   end
 end

spec/workers/sitemap_refresh_worker_spec.rb

@@ -1,31 +0,0 @@
-require "rails_helper"
-
-RSpec.describe SitemapRefreshWorker, type: :woker do
-  include_examples "#enqueues_on_correct_queue", "low_priority"
-
-  describe "#perform" do
-    let(:worker) { subject }
-    let(:mock_task) { instance_double(Rake::Task, invoke: true) }
-
-    before do
-      allow(Rails.application).to receive(:load_tasks)
-      allow(Rake::Task).to receive(:[]).and_return(mock_task)
-    end
-
-    it "runs sitemap:refresh:no_ping Rake task locally" do
-      allow(ForemInstance).to receive(:local?).and_return(true)
-
-      worker.perform
-
-      expect(Rake::Task).to have_received(:[]).with("sitemap:refresh:no_ping")
-    end
-
-    it "runs sitemap:refresh Rake task on regular installations" do
-      allow(ForemInstance).to receive(:local?).and_return(false)
-
-      worker.perform
-
-      expect(Rake::Task).to have_received(:[]).with("sitemap:refresh")
-    end
-  end
-end