Merge pull request activeadmin#1563 from gregbell/fix-collections-with-group-by

Fix collections with group by

Author: gregbell

features/index/index_scopes.feature

@@ -3,31 +3,31 @@ Feature: Index Scoping
   Viewing resources and scoping them
 
   Scenario: Viewing resources with one scope and no default
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
         scope :all
       end
       """
     Then I should see the scope "All" not selected
-    And I should see the scope "All" with the count 10
-    And I should see 10 posts in the table
+    And I should see the scope "All" with the count 3
+    And I should see 3 posts in the table
 
   Scenario: Viewing resources with one scope as the default
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
         scope :all, :default => true
       end
       """
     Then I should see the scope "All" selected
-    And I should see the scope "All" with the count 10
-    And I should see 10 posts in the table
+    And I should see the scope "All" with the count 3
+    And I should see 3 posts in the table
 
   Scenario: Viewing resources with one scope and no results
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
      """
      ActiveAdmin.register Post do
@@ -36,15 +36,15 @@ Feature: Index Scoping
      end
      """
 
-    When I fill in "Search Title" with "Hello World 17"
+    When I fill in "Search Title" with "Non Existing Post"
     And I press "Filter"
     And I should not see the scope "All"
 
     When I am on the index page for posts
     Then I should see the scope "All" selected
 
   Scenario: Viewing resources with a scope but scope_count turned off
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
@@ -54,11 +54,11 @@ Feature: Index Scoping
       """
     Then I should see the scope "All" selected
     And I should see the scope "All" with no count
-    And I should see 10 posts in the table
+    And I should see 3 posts in the table
 
   @scope
   Scenario: Viewing resources with a scope and scope count turned off for a single scope
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
@@ -67,11 +67,11 @@ Feature: Index Scoping
       """
     Then I should see the scope "All" selected
     And I should see the scope "All" with no count
-    And I should see 10 posts in the table
+    And I should see 3 posts in the table
 
   Scenario: Viewing resources when scoping
-    Given 6 posts exist
-    And 4 published posts exist
+    Given 2 posts exist
+    And 3 published posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
@@ -81,15 +81,49 @@ Feature: Index Scoping
         end
       end
       """
-    Then I should see the scope "All" with the count 10
-    And I should see 10 posts in the table
-    Then I should see the scope "Published" with the count 4
+    Then I should see the scope "All" with the count 5
+    And I should see 5 posts in the table
+    And I should see the scope "Published" with the count 3
     When I follow "Published"
     Then I should see the scope "Published" selected
-    And I should see 4 posts in the table
+    And I should see 3 posts in the table
+
+  Scenario: Viewing resources when scoping and filtering
+    Given 2 posts written by "Daft Punk" exist
+    Given 1 published posts written by "Daft Punk" exist
+
+    Given 1 posts written by "Alfred" exist
+    Given 2 published posts written by "Alfred" exist
+
+    And an index configuration of:
+      """
+      ActiveAdmin.register Post do
+        scope :all, :default => true
+        scope :published do |posts|
+          posts.where("published_at IS NOT NULL")
+        end
+      end
+      """
+    Then I should see the scope "All" with the count 6
+    And I should see the scope "Published" with the count 3
+    And I should see 6 posts in the table
+
+    When I follow "Published"
+    Then I should see the scope "Published" selected
+    And I should see the scope "All" with the count 6
+    And I should see the scope "Published" with the count 3
+    And I should see 3 posts in the table
+
+    When I select "daft_punk" from "Author"
+    And I press "Filter"
+
+    Then I should see the scope "Published" selected
+    And I should see the scope "All" with the count 3
+    And I should see the scope "Published" with the count 1
+    And I should see 1 posts in the table
 
   Scenario: Viewing resources with optional scopes
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
     """
     ActiveAdmin.register Post do
@@ -109,10 +143,10 @@ Feature: Index Scoping
     And I should not see the scope "All"
     And I should not see the scope "Today"
     And I should see the scope "Shown"
-    And I should see the scope "Default" with the count 10
+    And I should see the scope "Default" with the count 3
 
   Scenario: Viewing resources with multiple scopes as blocks
-    Given 10 posts exist
+    Given 3 posts exist
     And an index configuration of:
       """
       ActiveAdmin.register Post do
@@ -126,17 +160,17 @@ Feature: Index Scoping
       """
     Then I should see the scope "Today" selected
     And I should see the scope "Tomorrow" not selected
-    And I should see the scope "Today" with the count 10
+    And I should see the scope "Today" with the count 3
     And I should see the scope "Tomorrow" with the count 0
-    And I should see 10 posts in the table
+    And I should see 3 posts in the table
     And I should see a link to "Tomorrow"
 
     When I follow "Tomorrow"
     Then I should see the scope "Tomorrow" selected
     And I should see the scope "Today" not selected
     And I should see a link to "Today"
 
-  Scenario: Viewing resources with scopes when a filter is applied
+  Scenario: Viewing resources with scopes when scoping to user
     Given 2 posts written by "Daft Punk" exist
     And a post with the title "Monkey Wrench" written by "Foo Fighters" exists
     And a post with the title "Everlong" written by "Foo Fighters" exists
@@ -145,6 +179,7 @@ Feature: Index Scoping
         ActiveAdmin.register Post do
           scope_to :current_user
           scope :all, :default => true
+
           filter :title
 
           controller do
@@ -160,3 +195,49 @@ Feature: Index Scoping
     And I press "Filter"
     Then I should see the scope "All" not selected
     And I should see the scope "All" with the count 1
+
+  Scenario: Viewing resources when scoping and filtering and group bys and stuff
+    Given 2 posts written by "Daft Punk" exist
+    Given 1 published posts written by "Daft Punk" exist
+
+    Given 1 posts written by "Alfred" exist
+
+    And an index configuration of:
+      """
+      ActiveAdmin.register Post do
+        scope :all, :default => true
+        scope :published do |posts|
+          posts.where("published_at IS NOT NULL")
+        end
+
+        index do
+          column :author_id
+          column :count
+        end
+
+        config.sort_order = "author_id_asc"
+
+        controller do
+          def scoped_collection
+            Post.select("author_id, count(*) as count").group("author_id")
+          end
+        end
+      end
+      """
+    Then I should see the scope "All" with the count 2
+    And I should see the scope "Published" with the count 1
+    And I should see 2 posts in the table
+
+    When I follow "Published"
+    Then I should see the scope "Published" selected
+    And I should see the scope "All" with the count 2
+    And I should see the scope "Published" with the count 1
+    And I should see 1 posts in the table
+
+    When I select "daft_punk" from "Author"
+    And I press "Filter"
+
+    Then I should see the scope "Published" selected
+    And I should see the scope "All" with the count 1
+    And I should see the scope "Published" with the count 1
+    And I should see 1 posts in the table

features/step_definitions/factory_steps.rb

@@ -13,11 +13,11 @@
   Post.create! :title => title, :author => author, :published_at => published_at
 end
 
-Given /^(\d+) posts? written by "([^"]*)" exist$/ do |count, author_name|
+Given /^(\d+)( published)? posts? written by "([^"]*)" exist$/ do |count, published, author_name|
   first, last = author_name.split(' ')
   author = User.find_or_create_by_first_name_and_last_name(first, last, :username => author_name.gsub(' ', '').underscore)
   (0...count.to_i).each do |i|
-    Post.create! :title => "Hello World #{i}", :author => author
+    Post.create! :title => "Hello World #{i}", :author => author, :published_at => (published ? Time.now : nil)
   end
 end
 

lib/active_admin/helpers/collection.rb

@@ -0,0 +1,23 @@
+module ActiveAdmin
+  module Helpers
+    module Collection
+      # Works around this issue: https://github.com/rails/rails/issues/7121
+      #
+      # GROUP BY + COUNT drops SELECT statement. This leads to SQL error when
+      # the ORDER statement mentions a column defined in the SELECT statement.
+      #
+      # We remove the ORDER statement to work around this issue.
+      def collection_size(collection=collection)
+        size = collection.reorder("").count
+        # when GROUP BY is used, AR returns Hash instead of Fixnum for .size
+        size = size.size if size.kind_of?(Hash)
+
+        size
+      end
+
+      def collection_is_empty?(collection=collection)
+        collection_size(collection) == 0
+      end
+    end
+  end
+end

lib/active_admin/resource_controller/collection.rb

@@ -88,17 +88,20 @@ def active_admin_collection
         end
 
         def scope_current_collection(chain)
+          @collection_before_scope = chain
+
           if current_scope
-            @before_scope_collection = chain
             scope_chain(current_scope, chain)
-          elsif active_admin_config.scope_to
-            @before_scope_collection = scoped_collection
-            chain
           else
             chain
           end
         end
 
+        def collection_before_scope
+          @collection_before_scope
+        end
+
+
         include ActiveAdmin::ScopeChain
 
         def current_scope

lib/active_admin/views/components/paginated_collection.rb

@@ -1,3 +1,5 @@
+require 'active_admin/helpers/collection'
+
 module ActiveAdmin
   module Views
 
@@ -85,12 +87,14 @@ def build_download_format_links(formats = [:csv, :xml, :json])
         end
       end
 
+      include ::ActiveAdmin::Helpers::Collection
+
       # modified from will_paginate
       def page_entries_info(options = {})
         if options[:entry_name]
           entry_name = options[:entry_name]
           entries_name = options[:entries_name] || entry_name.pluralize
-        elsif collection.empty?
+        elsif collection_is_empty?
           entry_name = I18n.translate("active_admin.pagination.entry", :count => 1, :default => 'entry')
           entries_name = I18n.translate("active_admin.pagination.entry", :count => 2, :default => 'entries')
         else
@@ -99,17 +103,15 @@ def page_entries_info(options = {})
         end
 
         if collection.num_pages < 2
-          case collection.size
+          case collection_size
           when 0; I18n.t('active_admin.pagination.empty', :model => entries_name)
           when 1; I18n.t('active_admin.pagination.one', :model => entry_name)
           else;   I18n.t('active_admin.pagination.one_page', :model => entries_name, :n => collection.total_count)
           end
         else
-          size = collection.size
-          size = size.size if size.kind_of? Hash # when GROUP BY is used, AR returns Hash instead of Fixnum for .size
           offset = (collection.current_page - 1) * collection.limit_value
           total = collection.total_count
-          I18n.t('active_admin.pagination.multiple', :model => entries_name, :from => offset + 1, :to => offset + size, :total => total)
+          I18n.t('active_admin.pagination.multiple', :model => entries_name, :from => offset + 1, :to => offset + collection_size, :total => total)
         end
       end
 

lib/active_admin/views/components/scopes.rb

@@ -1,3 +1,5 @@
+require 'active_admin/helpers/collection'
+
 module ActiveAdmin
   module Views
 
@@ -7,6 +9,8 @@ class Scopes < ActiveAdmin::Component
       builder_method :scopes_renderer
 
       include ActiveAdmin::ScopeChain
+      include ::ActiveAdmin::Helpers::Collection
+
 
       def default_class_name
         "scopes table_tools_segmented_control"
@@ -54,22 +58,13 @@ def current_scope?(scope)
       end
 
       def current_filter_search_empty?
-        collection.empty? && params.include?(:q)
+        params.include?(:q) && collection_is_empty?
       end
 
       # Return the count for the scope passed in.
       def get_scope_count(scope)
-        if params[:q]
-          search(scope_chain(scope, scoping_class)).relation.count
-        else 
-          scope_chain(scope, scoping_class).count
-        end
+        collection_size(scope_chain(scope, collection_before_scope))
       end
-
-      def scoping_class
-        assigns[:before_scope_collection] || active_admin_config.resource_class
-      end
-
     end
   end
 end