paging for flag list

corrected reload behavior on flag list
refactored post actions ... extracted flag queries

app/assets/javascripts/admin/controllers/admin_flags_controller.js

@@ -78,6 +78,16 @@ Discourse.AdminFlagsController = Ember.ArrayController.extend({
 
     @property adminActiveFlagsView
   **/
-  adminActiveFlagsView: Em.computed.equal('query', 'active')
+  adminActiveFlagsView: Em.computed.equal('query', 'active'),
+
+  loadMore: function(){
+    var flags = this.get('model');
+    return Discourse.FlaggedPost.findAll(this.get('query'),flags.length+1).then(function(data){
+      if(data.length===0){
+        flags.set('allLoaded',true);
+      }
+      flags.addObjects(data);
+    });
+  }
 
 });

app/assets/javascripts/admin/models/flagged_post.js

@@ -103,10 +103,13 @@ Discourse.FlaggedPost = Discourse.Post.extend({
 });
 
 Discourse.FlaggedPost.reopenClass({
-  findAll: function(filter) {
+  findAll: function(filter, offset) {
+
+    offset = offset || 0;
+
     var result = Em.A();
     result.set('loading', true);
-    Discourse.ajax('/admin/flags/' + filter + '.json').then(function(data) {
+    return Discourse.ajax('/admin/flags/' + filter + '.json?offset=' + offset).then(function(data) {
       var userLookup = {};
       _.each(data.users,function(user) {
         userLookup[user.id] = Discourse.AdminUser.create(user);
@@ -117,8 +120,8 @@ Discourse.FlaggedPost.reopenClass({
         result.pushObject(f);
       });
       result.set('loading', false);
+      return result;
     });
-    return result;
   }
 });
 

app/assets/javascripts/admin/routes/admin_flags_active_route.js

@@ -1,23 +0,0 @@
-/**
-  Handles routes related to viewing active flags.
-
-  @class AdminFlagsActiveRoute
-  @extends Discourse.Route
-  @namespace Discourse
-  @module Discourse
-**/
-Discourse.AdminFlagsActiveRoute = Discourse.Route.extend({
-
-  model: function() {
-    return Discourse.FlaggedPost.findAll('active');
-  },
-
-  setupController: function(controller, model) {
-    var adminFlagsController = this.controllerFor('adminFlags');
-    adminFlagsController.set('content', model);
-    adminFlagsController.set('query', 'active');
-  }
-
-});
-
-

app/assets/javascripts/admin/routes/admin_flags_old_route.js

@@ -1,23 +0,0 @@
-/**
-  Handles routes related to viewing old flags.
-
-  @class AdminFlagsOldRoute
-  @extends Discourse.Route
-  @namespace Discourse
-  @module Discourse
-**/
-Discourse.AdminFlagsOldRoute = Discourse.Route.extend({
-
-  model: function() {
-    return Discourse.FlaggedPost.findAll('old');
-  },
-
-  setupController: function(controller, model) {
-    var adminFlagsController = this.controllerFor('adminFlags');
-    adminFlagsController.set('content', model);
-    adminFlagsController.set('query', 'old');
-  }
-
-});
-
-

app/assets/javascripts/admin/routes/admin_flags_route.js

@@ -1,14 +1,30 @@
-/**
-  Handles routes related to viewing flags.
-
-  @class AdminFlagsRoute
-  @extends Discourse.Route
-  @namespace Discourse
-  @module Discourse
-**/
-
-Discourse.AdminFlagsRoute = Discourse.Route.extend({
+Discourse.AdminFlagsIndexRoute = Discourse.Route.extend({
   redirect: function() {
     this.transitionTo('adminFlags.active');
   }
 });
+
+Discourse.AdminFlagsRouteType = Discourse.Route.extend({
+  model: function() {
+    return Discourse.FlaggedPost.findAll(this.get('filter'));
+  },
+
+  setupController: function(controller, model) {
+    var adminFlagsController = this.controllerFor('adminFlags');
+    adminFlagsController.set('content', model);
+    adminFlagsController.set('query', this.get('filter'));
+  }
+
+});
+
+Discourse.AdminFlagsActiveRoute = Discourse.AdminFlagsRouteType.extend({
+  filter: 'active'
+});
+
+
+Discourse.AdminFlagsOldRoute = Discourse.AdminFlagsRouteType.extend({
+  filter: 'old'
+});
+
+
+

app/assets/javascripts/admin/routes/admin_routes.js

@@ -25,6 +25,7 @@ Discourse.Route.buildRoutes(function() {
     this.resource('adminReports', { path: '/reports/:type' });
 
     this.resource('adminFlags', { path: '/flags' }, function() {
+      this.route('index', { path: '/' });
       this.route('active', { path: '/active' });
       this.route('old', { path: '/old' });
     });

app/assets/javascripts/admin/templates/flags.js.handlebars

@@ -85,6 +85,10 @@
 
       </tbody>
     </table>
+    {{#if view.loading}}
+      <div class='admin-loading'>{{i18n loading}}</div>
+    {{/if}}
+
   {{else}}
     <p>{{i18n admin.flags.no_results}}</p>
   {{/if}}

app/assets/javascripts/admin/views/admin_flags_view.js

@@ -0,0 +1,13 @@
+Discourse.AdminFlagsView = Discourse.View.extend(Discourse.LoadMore, {
+  loading: false,
+  eyelineSelector: '.admin-flags tbody tr',
+  loadMore: function() {
+    var view = this;
+    if(this.get("loading") || this.get("model.allLoaded")) { return; }
+    this.set("loading", true);
+    this.get("controller").loadMore().then(function(){
+      view.set("loading", false);
+    });
+  }
+
+});

app/assets/stylesheets/application/topic-list.css.scss

@@ -272,22 +272,23 @@
 
 #topic-list-bottom {
   margin: 20px 0;
-  .topics-loading {
-    width: 200px;
-    margin: 0 auto;
-    padding: 10px 0 10px 43px;
-    color: $white;
-    font-size: 18px;
-    line-height: 25px;
-    background: {
-      color: $black;
-      image: image-url("spinner_96_w.gif");
-      repeat: no-repeat;
-      position: 10px 50%;
-      size: 25px;
-    };
-    @include border-radius-all(12px);
-  }
+}
+
+.topics-loading {
+  width: 200px;
+  margin: 0 auto;
+  padding: 10px 0 10px 43px;
+  color: $white;
+  font-size: 18px;
+  line-height: 25px;
+  background: {
+    color: $black;
+    image: image-url("spinner_96_w.gif");
+    repeat: no-repeat;
+    position: 10px 50%;
+    size: 25px;
+  };
+  @include border-radius-all(12px);
 }
 
 // Misc. stuff
@@ -335,4 +336,4 @@
       image: image-url("posted.png");
     };
   }
-}
+}

app/controllers/admin/flags_controller.rb

@@ -1,8 +1,10 @@
+require 'flag_query'
+
 class Admin::FlagsController < Admin::AdminController
   def index
     # we may get out of sync, fix it here
     PostAction.update_flagged_posts_count
-    posts, users = PostAction.flagged_posts_report(params[:filter])
+    posts, users = FlagQuery.flagged_posts_report(params[:filter], params[:offset].to_i, 10)
 
     if posts.blank?
       render json: {users: [], posts: []}

app/models/post_action.rb

@@ -91,51 +91,52 @@ class PostAction < ActiveRecord::Base
     update_flagged_posts_count
   end
 
-  def self.act(user, post, post_action_type_id, opts={})
-    begin
-      title, target_usernames, target_group_names, subtype, body = nil
+  def self.create_message_for_post_action(user, post, post_action_type_id, opts)
+    post_action_type = PostActionType.types[post_action_type_id]
 
-      if opts[:message]
-        [:notify_moderators, :notify_user].each do |k|
-          if post_action_type_id == PostActionType.types[k]
-            if k == :notify_moderators
-              target_group_names = target_moderators
-            else
-              target_usernames = post.user.username
-            end
-            title = I18n.t("post_action_types.#{k}.email_title",
-                            title: post.topic.title)
-            body = I18n.t("post_action_types.#{k}.email_body",
-                          message: opts[:message],
-                          link: "#{Discourse.base_url}#{post.url}")
-            subtype = k == :notify_moderators ? TopicSubtype.notify_moderators : TopicSubtype.notify_user
-          end
-        end
-      end
+    return unless opts[:message] && [:notify_moderators, :notify_user].include?(post_action_type)
 
-      related_post_id = nil
-      if target_usernames.present? || target_group_names.present?
-        related_post_id = PostCreator.new(user,
-                              target_usernames: target_usernames,
-                              target_group_names: target_group_names,
-                              archetype: Archetype.private_message,
-                              subtype: subtype,
-                              title: title,
-                              raw: body
-                       ).create.id
-      end
+    target_group_names, target_usernames = nil
 
-      create( post_id: post.id,
-              user_id: user.id,
-              post_action_type_id: post_action_type_id,
-              message: opts[:message],
-              staff_took_action: opts[:take_action] || false,
-              related_post_id: related_post_id )
-    rescue ActiveRecord::RecordNotUnique
-      # can happen despite being .create
-      # since already bookmarked
-      true
+    if post_action_type == :notify_moderators
+      target_group_names = target_moderators
+    else
+      target_usernames = post.user.username
     end
+    title = I18n.t("post_action_types.#{post_action_type}.email_title",
+                    title: post.topic.title)
+    body = I18n.t("post_action_types.#{post_action_type}.email_body",
+                  message: opts[:message],
+                  link: "#{Discourse.base_url}#{post.url}")
+
+    subtype = post_action_type == :notify_moderators ? TopicSubtype.notify_moderators : TopicSubtype.notify_user
+
+    if target_usernames.present? || target_group_names.present?
+      PostCreator.new(user,
+              target_usernames: target_usernames,
+              target_group_names: target_group_names,
+              archetype: Archetype.private_message,
+              subtype: subtype,
+              title: title,
+              raw: body
+       ).create.id
+    end
+  end
+
+  def self.act(user, post, post_action_type_id, opts={})
+
+    related_post_id = create_message_for_post_action(user,post,post_action_type_id,opts)
+
+    create( post_id: post.id,
+            user_id: user.id,
+            post_action_type_id: post_action_type_id,
+            message: opts[:message],
+            staff_took_action: opts[:take_action] || false,
+            related_post_id: related_post_id )
+  rescue ActiveRecord::RecordNotUnique
+    # can happen despite being .create
+    # since already bookmarked
+    true
   end
 
   def self.remove_act(user, post, post_action_type_id)
@@ -298,90 +299,8 @@ class PostAction < ActiveRecord::Base
       Post.hidden_reasons[:flag_threshold_reached]
   end
 
-  def self.flagged_posts_report(filter)
-
-    actions = flagged_post_actions(filter)
-
-    post_ids = actions.limit(300).pluck(:post_id).uniq
-    return nil if post_ids.blank?
-
-    posts = SqlBuilder.new("SELECT p.id, t.title, p.cooked, p.user_id,
-      p.topic_id, p.post_number, p.hidden, t.visible topic_visible,
-      p.deleted_at, t.deleted_at topic_deleted_at
-      FROM posts p
-      JOIN topics t ON t.id = p.topic_id
-      WHERE p.id in (:post_ids)").map_exec(OpenStruct, post_ids: post_ids)
-
-    post_lookup = {}
-    users = Set.new
-
-    posts.each do |p|
-      users << p.user_id
-      p.excerpt = Post.excerpt(p.cooked)
-      p.topic_slug = Slug.for(p.title)
-      post_lookup[p.id] = p
-    end
-
-    # maintain order
-    posts = post_ids.map{|id| post_lookup[id]}
-
-    post_actions = actions.where(:post_id => post_ids)
-    # TODO this is so far from optimal, it should not be
-    # selecting all the columns but the includes stops working
-    # with the code below
-    #
-                          # .select('post_actions.id,
-                          #          post_actions.user_id,
-                          #          post_action_type_id,
-                          #          post_actions.created_at,
-                          #          post_actions.post_id,
-                          #          post_actions.message')
-                          # .to_a
-
-    post_actions.each do |pa|
-      post = post_lookup[pa.post_id]
-      post.post_actions ||= []
-      action = pa.attributes
-      action[:name_key] = PostActionType.types.key(pa.post_action_type_id)
-      if (pa.related_post && pa.related_post.topic)
-        action.merge!(topic_id: pa.related_post.topic_id,
-                     slug: pa.related_post.topic.slug,
-                     permalink: pa.related_post.topic.url)
-      end
-      post.post_actions << action
-      users << pa.user_id
-    end
-
-    # TODO add serializer so we can skip this
-    posts.map!(&:marshal_dump)
-    [posts, User.where(id: users.to_a).to_a]
-  end
-
   protected
 
-  def self.flagged_post_actions(filter)
-    post_actions = PostAction
-                      .includes({:related_post => :topic})
-                      .where(post_action_type_id: PostActionType.notify_flag_type_ids)
-                      .joins(:post => :topic)
-                      .order('post_actions.created_at DESC')
-
-    if filter == 'old'
-      post_actions
-        .with_deleted
-        .where('post_actions.deleted_at IS NOT NULL OR
-                defer = true OR
-                topics.deleted_at IS NOT NULL OR
-                posts.deleted_at IS NOT NULL')
-    else
-      post_actions
-        .where('defer IS NULL OR
-                defer = false')
-        .where('posts.deleted_at IS NULL AND
-                topics.deleted_at IS NULL')
-    end
-  end
-
   def self.target_moderators
     Group[:moderators].name
   end

lib/flag_query.rb

@@ -0,0 +1,97 @@
+module FlagQuery
+  def self.flagged_posts_report(filter, offset = 0, per_page = 25)
+
+    actions = flagged_post_actions(filter)
+
+    post_ids = actions
+              .limit(per_page)
+              .offset(offset)
+              .group(:post_id)
+              .order('min(post_actions.created_at) DESC')
+              .pluck(:post_id).uniq
+
+    return nil if post_ids.blank?
+
+    actions = actions
+                .order('post_actions.created_at DESC')
+                .includes({:related_post => :topic})
+
+    posts = SqlBuilder.new("SELECT p.id, t.title, p.cooked, p.user_id,
+      p.topic_id, p.post_number, p.hidden, t.visible topic_visible,
+      p.deleted_at, t.deleted_at topic_deleted_at
+      FROM posts p
+      JOIN topics t ON t.id = p.topic_id
+      WHERE p.id in (:post_ids)").map_exec(OpenStruct, post_ids: post_ids)
+
+    post_lookup = {}
+    users = Set.new
+
+    posts.each do |p|
+      users << p.user_id
+      p.excerpt = Post.excerpt(p.cooked)
+      p.topic_slug = Slug.for(p.title)
+      post_lookup[p.id] = p
+    end
+
+    # maintain order
+    posts = post_ids.map{|id| post_lookup[id]}
+
+    post_actions = actions.where(:post_id => post_ids)
+
+    post_actions.each do |pa|
+      post = post_lookup[pa.post_id]
+      post.post_actions ||= []
+      action = pa.attributes
+      action[:name_key] = PostActionType.types.key(pa.post_action_type_id)
+      if (pa.related_post && pa.related_post.topic)
+        action.merge!(topic_id: pa.related_post.topic_id,
+                     slug: pa.related_post.topic.slug,
+                     permalink: pa.related_post.topic.url)
+      end
+      post.post_actions << action
+      users << pa.user_id
+    end
+
+    # TODO add serializer so we can skip this
+    posts.map!(&:marshal_dump)
+    [posts, User.where(id: users.to_a).to_a]
+  end
+
+  protected
+
+  def self.flagged_post_ids(filter, offset, limit)
+    sql = <<SQL
+
+    SELECT p.id from posts p
+    JOIN topics t ON t.id = p.topic_id
+    WHERE p.id IN (
+      SELECT post_id from post_actions
+      WHERE
+    )
+    /*offset*/
+    /*limit*/
+
+SQL
+  end
+
+  def self.flagged_post_actions(filter)
+    post_actions = PostAction
+                      .where(post_action_type_id: PostActionType.notify_flag_type_ids)
+                      .joins(:post => :topic)
+
+    if filter == 'old'
+      post_actions
+        .with_deleted
+        .where('post_actions.deleted_at IS NOT NULL OR
+                defer = true OR
+                topics.deleted_at IS NOT NULL OR
+                posts.deleted_at IS NOT NULL')
+    else
+      post_actions
+        .where('defer IS NULL OR
+                defer = false')
+        .where('posts.deleted_at IS NULL AND
+                topics.deleted_at IS NULL')
+    end
+  end
+end

spec/components/flag_query_spec.rb

@@ -0,0 +1,40 @@
+require 'spec_helper'
+require_dependency 'flag_query'
+
+describe FlagQuery do
+
+  let(:codinghorror) { Fabricate(:coding_horror) }
+
+  describe "flagged_posts_report" do
+    it "operates correctly" do
+      post = create_post
+      post2 = create_post
+
+      user2 = Fabricate(:user)
+      user3 = Fabricate(:user)
+
+      PostAction.act(codinghorror, post, PostActionType.types[:spam])
+      PostAction.act(user2, post, PostActionType.types[:spam])
+      mod_message = PostAction.act(user3, post, PostActionType.types[:notify_moderators], message: "this is a 10")
+
+      PostAction.act(codinghorror, post2, PostActionType.types[:spam])
+      PostAction.act(user2, post2, PostActionType.types[:spam])
+
+      posts, users = FlagQuery.flagged_posts_report("")
+      posts.count.should == 2
+      first = posts.first
+
+      users.count.should == 5
+      first[:post_actions].count.should == 2
+
+      second = posts[1]
+
+      second[:post_actions].count.should == 3
+      second[:post_actions].first[:permalink].should == mod_message.related_post.topic.url
+
+      posts, users = FlagQuery.flagged_posts_report("",offset=1)
+      posts.count.should == 1
+
+    end
+  end
+end

spec/models/post_action_spec.rb

@@ -12,21 +12,6 @@ describe PostAction do
   let(:post) { Fabricate(:post) }
   let(:bookmark) { PostAction.new(user_id: post.user_id, post_action_type_id: PostActionType.types[:bookmark] , post_id: post.id) }
 
-  describe "flagged_posts_report" do
-    it "operates correctly" do
-      post = create_post
-      PostAction.act(codinghorror, post, PostActionType.types[:spam])
-      mod_message = PostAction.act(Fabricate(:user), post, PostActionType.types[:notify_moderators], message: "this is a 10")
-
-      posts, users = PostAction.flagged_posts_report("")
-      posts.count.should == 1
-      first = posts.first
-
-      users.count.should == 3
-      first[:post_actions].count.should == 2
-      first[:post_actions].first[:permalink].should == mod_message.related_post.topic.url
-    end
-  end
 
   describe "messaging" do