Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ gem 'sentry-rails'
gem 'sentry-ruby'
gem 'stackprof'

# Sitemap generation
gem 'sitemap_generator'

# Storext for easier json attributes, custom fork for Better Together
gem 'storext', github: 'better-together-org/storext'

Expand Down
5 changes: 5 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ PATH
ruby-openai
sidekiq-scheduler
simple_calendar
sitemap_generator
sprockets-rails
stackprof
stimulus-rails (~> 1.3)
Expand Down Expand Up @@ -753,6 +754,8 @@ GEM
logger (>= 1.6.2)
rack (>= 3.1.0)
redis-client (>= 0.23.2)
sitemap_generator (6.3.0)
builder (~> 3.0)
sidekiq-scheduler (6.0.1)
rufus-scheduler (~> 3.2)
sidekiq (>= 7.3, < 9)
Expand Down Expand Up @@ -895,6 +898,8 @@ DEPENDENCIES
sentry-ruby
shoulda-callback-matchers
shoulda-matchers
sidekiq (~> 8.0.7)
sitemap_generator
sidekiq (~> 8.0.9)
simplecov
spring
Expand Down
15 changes: 15 additions & 0 deletions app/controllers/better_together/sitemaps_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

module BetterTogether
# Serves the generated sitemap stored in Active Storage
class SitemapsController < ApplicationController
def show
sitemap = Sitemap.current(helpers.host_platform)
if sitemap.file.attached?
redirect_to sitemap.file.url, allow_other_host: true
else
head :not_found
end
end
end
end
16 changes: 16 additions & 0 deletions app/jobs/better_together/sitemap_refresh_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

require 'rake'

module BetterTogether
# Generates the sitemap in a background job so newly published pages are included
class SitemapRefreshJob < ApplicationJob
queue_as :default

def perform
Rails.application.load_tasks unless Rake::Task.task_defined?('sitemap:refresh')
Rake::Task['sitemap:refresh'].invoke
Rake::Task['sitemap:refresh'].reenable
end
end
end
8 changes: 8 additions & 0 deletions app/models/better_together/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ class Page < ApplicationRecord
scope :published, -> { where.not(published_at: nil).where('published_at <= ?', Time.zone.now) }
scope :by_publication_date, -> { order(published_at: :desc) }

after_commit :refresh_sitemap, on: %i[create update destroy]

def hero_block
@hero_block ||= blocks.where(type: 'BetterTogether::Content::Hero').with_attached_background_image_file.with_translations.first
end
Expand Down Expand Up @@ -103,6 +105,12 @@ def url

private

def refresh_sitemap
return if Rails.env.test?

SitemapRefreshJob.perform_later
end

def add_creator_as_author
return unless respond_to?(:creator_id) && creator_id.present?

Expand Down
2 changes: 2 additions & 0 deletions app/models/better_together/platform.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ class Platform < ApplicationRecord
has_one_attached :profile_image
has_one_attached :cover_image

has_one :sitemap, class_name: '::BetterTogether::Sitemap', dependent: :destroy

has_many :platform_blocks, dependent: :destroy, class_name: 'BetterTogether::Content::PlatformBlock'
has_many :blocks, through: :platform_blocks

Expand Down
16 changes: 16 additions & 0 deletions app/models/better_together/sitemap.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# frozen_string_literal: true

module BetterTogether
# Stores the generated sitemap in Active Storage for serving via S3
class Sitemap < ApplicationRecord
belongs_to :platform

has_one_attached :file

validates :platform_id, uniqueness: true

def self.current(platform)
find_or_create_by!(platform: platform)
end
end
end
Comment on lines +1 to +16
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Missing factory for the Sitemap model. According to the coding guidelines, "Every Better Together model needs a corresponding FactoryBot factory with proper engine namespace handling." Add a factory file at spec/factories/better_together/sitemaps.rb to support testing.

Copilot generated this review using guidance from repository custom instructions.
1 change: 1 addition & 0 deletions app/views/layouts/better_together/application.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
<% end %>
<%= csrf_meta_tags %>
<%= csp_meta_tag %>
<link rel="sitemap" type="application/xml" href="<%= sitemap_path %>">
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The sitemap link tag is missing the locale parameter required by the engine's routing constraints. This will cause the link to fail to resolve properly. Update to:

<link rel="sitemap" type="application/xml" href="<%= sitemap_path(locale: I18n.locale) %>">
Suggested change
<link rel="sitemap" type="application/xml" href="<%= sitemap_path %>">
<link rel="sitemap" type="application/xml" href="<%= sitemap_path(locale: I18n.locale) %>">

Copilot uses AI. Check for mistakes.

<!-- Default Stylesheets -->
<link rel="stylesheet" type="text/css" href="https://unpkg.com/[email protected]/dist/trix.css">
Expand Down
1 change: 1 addition & 0 deletions better_together.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'ruby-openai'
spec.add_dependency 'sidekiq-scheduler'
spec.add_dependency 'simple_calendar'
spec.add_dependency 'sitemap_generator'
spec.add_dependency 'sprockets-rails'
spec.add_dependency 'stackprof'
spec.add_dependency 'stimulus-rails', '~> 1.3'
Expand Down
4 changes: 3 additions & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@

require 'sidekiq/web'

BetterTogether::Engine.routes.draw do # rubocop:todo Metrics/BlockLength
BetterTogether::Engine.routes.draw do # rubocop:todo Metrics/BlockLength
get '/sitemap.xml.gz', to: 'sitemaps#show', as: :sitemap

scope ':locale', # rubocop:todo Metrics/BlockLength
locale: /#{I18n.available_locales.join('|')}/ do
Comment on lines +6 to 9
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The sitemap route is defined outside the :locale scope (line 6), which means it won't have access to locale-aware URL generation in the controller. However, the sitemap configuration file (config/sitemap.rb) generates locale-specific URLs using locale: I18n.default_locale for all paths. This could cause a mismatch between the sitemap route handling and the URLs it generates. Consider whether the sitemap should be locale-specific or if it should include URLs for all locales.

Suggested change
get '/sitemap.xml.gz', to: 'sitemaps#show', as: :sitemap
scope ':locale', # rubocop:todo Metrics/BlockLength
locale: /#{I18n.available_locales.join('|')}/ do
scope ':locale', # rubocop:todo Metrics/BlockLength
locale: /#{I18n.available_locales.join('|')}/ do
get '/sitemap.xml.gz', to: 'sitemaps#show', as: :sitemap

Copilot uses AI. Check for mistakes.
# bt base path
Expand Down
34 changes: 34 additions & 0 deletions config/sitemap.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# frozen_string_literal: true

SitemapGenerator::Sitemap.default_host =
"#{ENV.fetch('APP_PROTOCOL', 'http')}://#{ENV.fetch('APP_HOST', 'localhost:3000')}"

helpers = BetterTogether::Engine.routes.url_helpers

SitemapGenerator::Sitemap.create do
add helpers.home_page_path(locale: I18n.default_locale)

add helpers.communities_path(locale: I18n.default_locale)
BetterTogether::Community.find_each do |community|
add helpers.community_path(community, locale: I18n.default_locale), lastmod: community.updated_at
end

add helpers.conversations_path(locale: I18n.default_locale)
BetterTogether::Conversation.find_each do |conversation|
add helpers.conversation_path(conversation, locale: I18n.default_locale), lastmod: conversation.updated_at
end

add helpers.posts_path(locale: I18n.default_locale)
BetterTogether::Post.published.find_each do |post|
add helpers.post_path(post, locale: I18n.default_locale), lastmod: post.updated_at
end

add helpers.events_path(locale: I18n.default_locale)
BetterTogether::Event.find_each do |event|
Comment on lines +12 to +27
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Missing privacy filtering for Communities, Events, and Conversations. The sitemap includes all Communities (line 12), Conversations (line 17), and Events (line 27) without filtering by privacy level. Since these models include the Privacy concern (Community and Event) or contain private data (Conversation), they should be filtered to only include public items:

  • Line 12: BetterTogether::Community.privacy_public.find_each
  • Line 17: Conversations should likely not be in the sitemap at all since they are private by nature
  • Line 27: BetterTogether::Event.privacy_public.find_each

Additionally, Posts should also filter by privacy: line 22 should be BetterTogether::Post.published.privacy_public.find_each

Suggested change
BetterTogether::Community.find_each do |community|
add helpers.community_path(community, locale: I18n.default_locale), lastmod: community.updated_at
end
add helpers.conversations_path(locale: I18n.default_locale)
BetterTogether::Conversation.find_each do |conversation|
add helpers.conversation_path(conversation, locale: I18n.default_locale), lastmod: conversation.updated_at
end
add helpers.posts_path(locale: I18n.default_locale)
BetterTogether::Post.published.find_each do |post|
add helpers.post_path(post, locale: I18n.default_locale), lastmod: post.updated_at
end
add helpers.events_path(locale: I18n.default_locale)
BetterTogether::Event.find_each do |event|
BetterTogether::Community.privacy_public.find_each do |community|
add helpers.community_path(community, locale: I18n.default_locale), lastmod: community.updated_at
end
# Conversations are private and should not be included in the sitemap.
add helpers.posts_path(locale: I18n.default_locale)
BetterTogether::Post.published.privacy_public.find_each do |post|
add helpers.post_path(post, locale: I18n.default_locale), lastmod: post.updated_at
end
add helpers.events_path(locale: I18n.default_locale)
BetterTogether::Event.privacy_public.find_each do |event|

Copilot uses AI. Check for mistakes.
add helpers.event_path(event, locale: I18n.default_locale), lastmod: event.updated_at
end
Comment on lines +11 to +29
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Missing after_commit :refresh_sitemap callbacks for Post, Community, and Event models. Currently, only the Page model (line 60 in app/models/better_together/page.rb) triggers sitemap regeneration on changes. Since the sitemap includes Posts, Communities, and Events (config/sitemap.rb lines 22, 12, 27), these models should also trigger sitemap refresh when created, updated, or destroyed to keep the sitemap current.

Copilot uses AI. Check for mistakes.

BetterTogether::Page.published.privacy_public.find_each do |page|
add helpers.render_page_path(path: page.slug, locale: I18n.default_locale), lastmod: page.updated_at
end
end
11 changes: 11 additions & 0 deletions db/migrate/20250821120000_create_better_together_sitemaps.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

class CreateBetterTogetherSitemaps < ActiveRecord::Migration[7.1]
def change
create_bt_table :sitemaps do |t|
t.bt_references :platform,
null: false,
index: { unique: true, name: 'unique_sitemaps_platform' }
end
end
end
29 changes: 29 additions & 0 deletions lib/tasks/sitemap.rake
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# frozen_string_literal: true

namespace :sitemap do
desc 'Generate sitemap and upload to Active Storage'
task refresh: :environment do
require 'sitemap_generator'

SitemapGenerator::Sitemap.public_path = Rails.root.join('tmp')
SitemapGenerator::Sitemap.sitemaps_path = ''

load Rails.root.join('config/sitemap.rb')
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The rake task loads the sitemap configuration from the wrong location. Line 11 uses Rails.root.join('config/sitemap.rb') which looks for the file in the host application's config directory, but the sitemap.rb file is located in the engine's config directory (as shown in the diff). This should be BetterTogether::Engine.root.join('config/sitemap.rb') to correctly load the engine's sitemap configuration file.

Suggested change
load Rails.root.join('config/sitemap.rb')
load BetterTogether::Engine.root.join('config/sitemap.rb')

Copilot uses AI. Check for mistakes.

file_path = Rails.root.join('tmp', 'sitemap.xml.gz')
platform = BetterTogether::Platform.find_by!(host: true)
BetterTogether::Sitemap.current(platform).file.attach(
io: File.open(file_path),
filename: 'sitemap.xml.gz',
content_type: 'application/gzip'
)
end
end

begin
Rake::Task['assets:precompile'].enhance do
Rake::Task['sitemap:refresh'].invoke
end
rescue RuntimeError
# assets:precompile may not be defined in some environments
end
Comment on lines +23 to +29
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

The rake task hooks into assets:precompile (lines 24-26) which may not be appropriate for sitemap generation. Asset precompilation typically happens during deployment build phase, before the database is available. Sitemap generation requires database access to query Pages, Posts, Events, etc. This could cause deployment failures if the database isn't accessible during asset precompilation. Consider using a different hook (e.g., after deployment) or making this integration optional.

Suggested change
begin
Rake::Task['assets:precompile'].enhance do
Rake::Task['sitemap:refresh'].invoke
end
rescue RuntimeError
# assets:precompile may not be defined in some environments
end
# The automatic invocation of sitemap:refresh during assets:precompile has been removed.
# To generate the sitemap, run `rake sitemap:refresh` after deployment when the database is available.

Copilot uses AI. Check for mistakes.
9 changes: 9 additions & 0 deletions spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,14 @@
t.index ["reporter_id"], name: "index_better_together_reports_on_reporter_id"
end

create_table "better_together_sitemaps", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
t.integer "lock_version", default: 0, null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.uuid "platform_id", null: false
t.index ["platform_id"], name: "unique_sitemaps_platform", unique: true
end

create_table "better_together_resource_permissions", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
t.integer "lock_version", default: 0, null: false
t.datetime "created_at", null: false
Expand Down Expand Up @@ -1560,6 +1568,7 @@
add_foreign_key "better_together_platforms", "better_together_people", column: "creator_id"
add_foreign_key "better_together_posts", "better_together_people", column: "creator_id"
add_foreign_key "better_together_reports", "better_together_people", column: "reporter_id"
add_foreign_key "better_together_sitemaps", "better_together_platforms", column: "platform_id"
add_foreign_key "better_together_role_resource_permissions", "better_together_resource_permissions", column: "resource_permission_id"
add_foreign_key "better_together_role_resource_permissions", "better_together_roles", column: "role_id"
add_foreign_key "better_together_social_media_accounts", "better_together_contact_details", column: "contact_detail_id"
Expand Down
30 changes: 30 additions & 0 deletions spec/jobs/better_together/sitemap_refresh_job_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# frozen_string_literal: true

require 'rails_helper'
require 'zlib'

RSpec.describe BetterTogether::SitemapRefreshJob, type: :job do
it 'generates and attaches a sitemap' do
host_platform = create(:platform, :host)
BetterTogether::Sitemap.destroy_all

described_class.new.perform

expect(BetterTogether::Sitemap.current(host_platform).file).to be_attached
end

it 'includes only public pages in the sitemap' do
host_platform = create(:platform, :host)
public_page = create(:page, privacy: 'public', slug: 'public-page')
private_page = create(:page, privacy: 'private', slug: 'private-page')
BetterTogether::Sitemap.destroy_all

described_class.perform_now
Comment on lines +11 to +22
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Inconsistent job invocation methods between tests. Line 11 uses described_class.new.perform while line 22 uses described_class.perform_now. Use the same pattern in both tests for consistency. Prefer perform_now which is the standard RSpec pattern for testing ActiveJob jobs.

Copilot uses AI. Check for mistakes.

data = BetterTogether::Sitemap.current(host_platform).file.download
xml = Zlib::GzipReader.new(StringIO.new(data)).read

expect(xml).to include(public_page.slug)
expect(xml).not_to include(private_page.slug)
end
Comment on lines +16 to +29
Copy link

Copilot AI Nov 29, 2025

Choose a reason for hiding this comment

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

Missing test for public posts privacy filtering. While the test on line 16 verifies that only public pages are included in the sitemap, there's no equivalent test to verify that private posts, communities, or events are excluded. Add tests to verify privacy filtering works correctly for all model types included in the sitemap (Posts, Communities, Events).

Copilot uses AI. Check for mistakes.
end
38 changes: 38 additions & 0 deletions spec/requests/better_together/sitemaps_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe 'Sitemap', type: :request do
include BetterTogether::Engine.routes.url_helpers
include BetterTogether::DeviseSessionHelpers

let!(:host_platform) { configure_host_platform }

before do
host! 'www.example.com'
Rails.application.routes.default_url_options[:host] = 'www.example.com'
end

describe 'GET /sitemap.xml.gz' do
context 'when a sitemap is attached' do
it 'redirects to the file' do
sitemap = BetterTogether::Sitemap.current(host_platform)
sitemap.file.attach(io: StringIO.new('test'), filename: 'sitemap.xml.gz', content_type: 'application/gzip')

get sitemap_path

expect(response).to redirect_to(sitemap.file.url)
end
end

context 'when no sitemap exists' do
it 'returns not found' do
BetterTogether::Sitemap.current(host_platform).file.detach

get sitemap_path

expect(response).to have_http_status(:not_found)
end
end
end
end
Loading