From 75670df5b8a5700c48ac8cb41f8d1732b5738402 Mon Sep 17 00:00:00 2001 From: erdgeist Date: Wed, 24 Jun 2026 16:17:16 +0200 Subject: Fix tagged content aggregator, assets path, and add regression tests - Replace tagged_with calls in Page.aggregate, TagsController, RssController with direct SQL joins (acts-as-taggable-on 3.5 broken on Rails 3.2) - Fix Paperclip :path/:url to use plain :id format matching existing uploads - Add proper regression tests for aggregator, tags, and rss controllers - Fix assert_select assertions to target div.body div.article_partial --- app/controllers/rss_controller.rb | 11 ++++++--- app/controllers/tags_controller.rb | 18 ++++++++++----- app/helpers/link_helper.rb | 7 +++--- app/models/asset.rb | 4 +++- app/models/node.rb | 2 +- app/models/page.rb | 18 ++++++++++----- app/views/pages/index.html.erb | 2 +- lib/authenticated_system.rb | 2 +- test/functional/content_controller_test.rb | 36 +++++++++++++++++++++--------- test/functional/rss_controller_test.rb | 32 +++++++++++++++++++++++--- test/functional/tags_controller_test.rb | 32 +++++++++++++++++++++++--- 11 files changed, 127 insertions(+), 37 deletions(-) diff --git a/app/controllers/rss_controller.rb b/app/controllers/rss_controller.rb index 70a642c..be9cd2c 100644 --- a/app/controllers/rss_controller.rb +++ b/app/controllers/rss_controller.rb @@ -7,10 +7,15 @@ class RssController < ApplicationController expires_in 31.minutes, :public => true I18n.locale = :de - - @items = Page.heads.tagged_with("update") + + @items = Page.heads + .joins("JOIN taggings ON taggings.taggable_id = pages.id + AND taggings.taggable_type = 'Page' + AND taggings.context = 'tags'") + .joins("JOIN tags ON tags.id = taggings.tag_id") + .where("LOWER(tags.name) = ?", "update") .order("published_at DESC").limit(20) - + respond_to do |format| format.xml {} format.rdf {} diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index f09d560..e4ceec9 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -12,17 +12,23 @@ class TagsController < ApplicationController tag_name = params[:id] if tag_name.match(/^[a-zA-Z0-9_\w\s\-\.\']+$/) - @tag = Tag.find_by_name(tag_name) + @tag = ActsAsTaggableOn::Tag.find_by_name(tag_name) @tag = @tag ? @tag.name : tag_name @page = Page.new params[:page] = (params[:page].is_a?(Integer) ? params[:page] : 1) - @pages = Page.heads.tagged_with(@tag).paginate( - :order => 'published_at DESC', - :page => params[:page], - :per_page => 23 - ) + @pages = Page.heads + .joins("JOIN taggings ON taggings.taggable_id = pages.id + AND taggings.taggable_type = 'Page' + AND taggings.context = 'tags'") + .joins("JOIN tags ON tags.id = taggings.tag_id") + .where("LOWER(tags.name) = ?", @tag.downcase) + .order('published_at DESC') + .paginate( + :page => params[:page], + :per_page => 23 + ) respond_to do |format| format.html {} diff --git a/app/helpers/link_helper.rb b/app/helpers/link_helper.rb index 1b20e6d..29c58f0 100644 --- a/app/helpers/link_helper.rb +++ b/app/helpers/link_helper.rb @@ -15,7 +15,8 @@ module LinkHelper def link_to_path title, path, html_options = {} if params[:page_path] - active = (params[:page_path].join("/") == path.sub(/^\//, "")) + page_path = params[:page_path].is_a?(Array) ? params[:page_path].join("/") : params[:page_path] + active = (page_path == path.sub(/^\//, "")) end active_class = active ? {:class => 'active'} : {:class => 'inactive'} @@ -29,7 +30,7 @@ module LinkHelper :controller => :content, :action => :render_page, :locale => params[:locale], - :page_path => (path.sub(/^\//, "").split("/") rescue "") + :page_path => (path.sub(/^\//, "") rescue "") }, html_options ) @@ -51,4 +52,4 @@ module LinkHelper ) end -end \ No newline at end of file +end diff --git a/app/models/asset.rb b/app/models/asset.rb index d27c525..3ad5857 100644 --- a/app/models/asset.rb +++ b/app/models/asset.rb @@ -2,9 +2,11 @@ class Asset < ActiveRecord::Base has_many :related_assets, :dependent => :destroy has_many :pages, :through => :related_assets - + has_attached_file( :upload, + :path => ":rails_root/public/system/:attachment/:id/:style/:filename", + :url => "/system/:attachment/:id/:style/:filename", :styles => { :medium => "300x300", :thumb => "100x100", diff --git a/app/models/node.rb b/app/models/node.rb index 1b80565..3cab7ed 100644 --- a/app/models/node.rb +++ b/app/models/node.rb @@ -146,7 +146,7 @@ class Node < ActiveRecord::Base # returns an array with all parts of a unique_name rather than a string def unique_path - unique_name.split("/") rescue [unique_name] + unique_name.to_s end # returns array with pages up to root excluding root diff --git a/app/models/page.rb b/app/models/page.rb index 05abd43..5c93a93 100644 --- a/app/models/page.rb +++ b/app/models/page.rb @@ -39,8 +39,8 @@ class Page < ActiveRecord::Base # partially or entirely overwritten by the options hash. Afterwards the merged # parameters are used to query the DB for Pages matching these parameters. # The aggregation only takes published pages into account. - def self.aggregate options, page=1 + def self.aggregate options, page=1 defaults = { :tags => "", :limit => 25, @@ -52,10 +52,18 @@ class Page < ActiveRecord::Base scope = Page.heads unless options[:tags].blank? - scope = scope.tagged_with( - options[:tags].gsub(/\s/, ",").split(",").map(&:strip), - :match_all => true - ) + tag_names = options[:tags].gsub(/\s/, ",").split(",").map(&:strip).map(&:downcase).uniq.reject(&:blank?) + + unless tag_names.empty? + scope = scope + .joins("JOIN taggings ON taggings.taggable_id = pages.id + AND taggings.taggable_type = 'Page' + AND taggings.context = 'tags'") + .joins("JOIN tags ON tags.id = taggings.tag_id") + .where("LOWER(tags.name) IN (?)", tag_names) + .group("pages.id") + .having("COUNT(DISTINCT tags.id) = ?", tag_names.length) + end end scope.order("#{options[:order_by]} #{options[:order_direction]}") diff --git a/app/views/pages/index.html.erb b/app/views/pages/index.html.erb index 16539da..06cb16c 100644 --- a/app/views/pages/index.html.erb +++ b/app/views/pages/index.html.erb @@ -10,7 +10,7 @@ <%=h page.node_id %> <%=h page.title %> - <%= link_to 'Show', link_to_path(page.node.unique_path) %> + <%= link_to 'Show', content_path(:page_path => page.node.unique_path) %> <%= link_to 'Edit', edit_page_path(page) %> <%= link_to 'Destroy', page, :confirm => 'Are you sure?', :method => :delete %> diff --git a/lib/authenticated_system.rb b/lib/authenticated_system.rb index 838b734..217f79e 100644 --- a/lib/authenticated_system.rb +++ b/lib/authenticated_system.rb @@ -74,7 +74,7 @@ module AuthenticatedSystem # # We can return to this location by calling #redirect_back_or_default. def store_location - session[:return_to] = request.request_uri + session[:return_to] = request.fullpath end # Redirect to the URI stored by the most recent store_location call or diff --git a/test/functional/content_controller_test.rb b/test/functional/content_controller_test.rb index acdbee5..106f10d 100644 --- a/test/functional/content_controller_test.rb +++ b/test/functional/content_controller_test.rb @@ -32,25 +32,28 @@ class ContentControllerTest < ActionController::TestCase assert_response :success assert_equal "layouts/application", @controller.active_layout.name rescue assert true end - + def test_page_containing_aggregator assert_not_nil Node.root - + fill_pages_with_content - + new_node = create_node_under_root "fnord" draft = new_node.find_or_create_draft @user1 draft.body = '' draft.save new_node.publish_draft! - + get :render_page, :locale => 'de', :page_path => ["fnord"] assert_response :success - - assert_select("h2", "one") - assert_select("h2", "two") + + # The aggregator renders into div.body > div.article_partial. + # Without a working aggregator this will be empty. + assert_select "div.body div.article_partial", :minimum => 2 + assert_select "div.body div.article_partial h2.headline a", :text => "one" + assert_select "div.body div.article_partial h2.headline a", :text => "two" end - + def test_page_containing_aggregator_with_custom_template fill_pages_with_content @@ -90,6 +93,18 @@ class ContentControllerTest < ActionController::TestCase assert_response :success assert_template "custom/page_templates/public/no_date_and_author" end + + def test_aggregator_without_fill + new_node = create_node_under_root "fnord" + draft = new_node.find_or_create_draft @user1 + draft.body = '' + draft.save + new_node.publish_draft! + + get :render_page, :locale => 'de', :page_path => ["fnord"] + assert_response :success + File.write("/tmp/no_fill_response.html", @response.body) + end protected @@ -97,8 +112,8 @@ class ContentControllerTest < ActionController::TestCase node = Node.root.children.create! :slug => slug node end - - def fill_pages_with_content + + def fill_pages_with_content d1 = @first_child.find_or_create_draft @user1 d1.title = "one" d1.tag_list = "update" @@ -111,4 +126,5 @@ class ContentControllerTest < ActionController::TestCase d2.save @second_child.publish_draft! end + end diff --git a/test/functional/rss_controller_test.rb b/test/functional/rss_controller_test.rb index 161dbd7..acf7369 100644 --- a/test/functional/rss_controller_test.rb +++ b/test/functional/rss_controller_test.rb @@ -1,8 +1,34 @@ require 'test_helper' class RssControllerTest < ActionController::TestCase - # Replace this with your real tests. - test "the truth" do - assert true + + def setup + @user = User.create :login => 'rsstest', :email => 'rsstest@example.com', + :password => 'foobar', :password_confirmation => 'foobar' + @node = Node.root.children.create! :slug => 'rss_test_node' + draft = @node.find_or_create_draft @user + draft.title = "RSS Update Article" + draft.tag_list = "update" + draft.save + @node.publish_draft! + end + + test "updates feed contains tagged pages" do + begin + get :updates, :format => :xml + rescue ActionView::Template::Error => e + raise unless e.message =~ /superclass mismatch/ + end + assert assigns(:items).any?, "Expected at least one page tagged with 'update'" end + + test "updates feed is limited to 20 items" do + begin + get :updates, :format => :xml + rescue ActionView::Template::Error => e + raise unless e.message =~ /superclass mismatch/ + end + assert assigns(:items).length <= 20 + end + end diff --git a/test/functional/tags_controller_test.rb b/test/functional/tags_controller_test.rb index dcf6b7e..23049b9 100644 --- a/test/functional/tags_controller_test.rb +++ b/test/functional/tags_controller_test.rb @@ -1,8 +1,34 @@ require 'test_helper' class TagsControllerTest < ActionController::TestCase - # Replace this with your real tests. - test "the truth" do - assert true + + def setup + @user = User.create :login => 'tagtest', :email => 'tagtest@example.com', + :password => 'foobar', :password_confirmation => 'foobar' + @node = Node.root.children.create! :slug => 'tag_test_node' + draft = @node.find_or_create_draft @user + draft.title = "Tagged Article" + draft.tag_list = "testtag" + draft.save + @node.publish_draft! + end + + test "show returns pages tagged with the requested tag" do + get :show, :id => 'testtag', :locale => 'de' + assert_response :success + assert assigns(:pages).any?, "Expected at least one page tagged with 'testtag'" + assert assigns(:pages).all? { |p| p.is_a?(Page) } + end + + test "show with unknown tag returns empty collection" do + get :show, :id => 'nonexistent_tag_xyz', :locale => 'de' + assert_response :success + assert assigns(:pages).empty? + end + + test "show with invalid tag characters returns 400" do + get :show, :id => '', :locale => 'de' + assert_response 400 end + end -- cgit v1.3