needs_grading by section for mobile

fixes CNVS-14375

Previously grading counts were only available
aggregated as a count for the assignment. This
lets you consume them split out by section
rather than.

needs_grading_count_by_section is the flag
to send.  This also splits out the needs_grading
query off into it's own object to reduce the
cognitive load of working in the assignment model.

TEST PLAN:
 - setup a course with multiple sections and
    assignments that need grading for students in
    each
 - hit the assignments API index with the
    "needs_grading_count_by_section" parameter set
    to true
 - the JSON response should include a
     "needs_grading_count_by_section" key for
     each assignment and it should have a hash
     of section ids and counts

Change-Id: I290a080b5f657996423798cacfb37660f5ed427c
Reviewed-on: https://gerrit.instructure.com/39627
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Mike Nomitch <mnomitch@instructure.com>
Reviewed-by: Cameron Matheson <cameron@instructure.com>
Product-Review: Hilary Scharton <hilary@instructure.com>
QA-Review: Amber Taniuchi <amber@instructure.com>

app/controllers/assignments_api_controller.rb

@@ -312,6 +312,14 @@
 #           "example": 17,
 #           "type": "integer"
 #         },
+#         "needs_grading_count_by_section": {
+#           "description": "if the requesting user has grading rights and the 'needs_grading_count_by_section' flag is specified, the number of submissions that need grading split out by section. NOTE: This key is NOT present unless you pass the 'needs_grading_count_by_section' argument as true.  ANOTHER NOTE: it's possible to be enrolled in multiple sections, and if a student is setup that way they will show an assignment that needs grading in multiple sections (effectively the count will be duplicated between sections)",
+#           "example": [
+#             {"section_id":"123456","needs_grading_count":5},
+#             {"section_id":"654321","needs_grading_count":0}
+#           ],
+#           "type": "array"
+#         },
 #         "position": {
 #           "description": "the sorting order of the assignment in the group",
 #           "example": 1,
@@ -479,6 +487,8 @@ class AssignmentsApiController < ApplicationController
   #   The partial title of the assignments to match and return.
   # @argument override_assignment_dates [Boolean]
   #   Apply assignment overrides for each assignment, defaults to true.
+  # @argument needs_grading_count_by_section [Boolean]
+  #   Split up "needs_grading_count" by sections into the "needs_grading_count_by_section" key, defaults to false
   # @returns [Assignment]
   def index
     if authorized_action(@context, @current_user, :read)
@@ -534,11 +544,15 @@ class AssignmentsApiController < ApplicationController
 
       include_visibility = Array(params[:include]).include?('assignment_visibility')
 
+      needs_grading_by_section_param = params[:needs_grading_count_by_section] || false
+      needs_grading_count_by_section = value_to_boolean(needs_grading_by_section_param)
+
       hashes = assignments.map do |assignment|
         submission = submissions[assignment.id]
         assignment_json(assignment, @current_user, session,
                         submission: submission, override_dates: override_dates,
-                        include_visibility: include_visibility)
+                        include_visibility: include_visibility,
+                        needs_grading_count_by_section: needs_grading_count_by_section)
       end
 
       render :json => hashes
@@ -552,6 +566,8 @@ class AssignmentsApiController < ApplicationController
   #   requires that the Differentiated Assignments course feature be turned on.
   # @argument override_assignment_dates [Boolean]
   #   Apply assignment overrides to the assignment, defaults to true.
+  # @argument needs_grading_count_by_section [Boolean]
+  #   Split up "needs_grading_count" by sections into the "needs_grading_count_by_section" key, defaults to false
   # @returns Assignment
   def show
     @assignment = @context.active_assignments.find(params[:id],
@@ -568,11 +584,15 @@ class AssignmentsApiController < ApplicationController
       override_param = params[:override_assignment_dates] || true
       override_dates = value_to_boolean(override_param)
 
+      needs_grading_by_section_param = params[:needs_grading_count_by_section] || false
+      needs_grading_count_by_section = value_to_boolean(needs_grading_by_section_param)
+
       @assignment.context_module_action(@current_user, :read) unless @assignment.locked_for?(@current_user, :check_policies => true)
       render :json => assignment_json(@assignment, @current_user, session,
-                                      submission: submission,
-                                      override_dates: override_dates,
-                                      include_visibility: include_visibility)
+                  submission: submission,
+                  override_dates: override_dates,
+                  include_visibility: include_visibility,
+                  needs_grading_count_by_section: needs_grading_count_by_section)
     end
   end
 

app/models/assignment.rb

@@ -1865,34 +1865,6 @@ class Assignment < ActiveRecord::Base
     end
   end
 
-  def needs_grading_count_for_user(user)
-    vis = self.context.section_visibilities_for(user)
-    self.shard.activate do
-      # the needs_grading_count trigger should change self.updated_at, invalidating the cache
-      Rails.cache.fetch(['assignment_user_grading_count', self, user].cache_key) do
-        case self.context.enrollment_visibility_level_for(user, vis)
-          when :full, :limited
-            self.needs_grading_count
-          when :sections
-            self.submissions.joins("INNER JOIN enrollments e ON e.user_id = submissions.user_id").
-                where(<<-SQL, self, self.context, vis.map {|v| v[:course_section_id]}).count(:id, :distinct => true)
-              submissions.assignment_id = ?
-                AND e.course_id = ?
-                AND e.course_section_id in (?)
-                AND e.type IN ('StudentEnrollment', 'StudentViewEnrollment')
-                AND e.workflow_state = 'active'
-                AND submissions.submission_type IS NOT NULL
-                AND (submissions.workflow_state = 'pending_review'
-                  OR (submissions.workflow_state = 'submitted'
-                    AND (submissions.score IS NULL OR NOT submissions.grade_matches_current_submission)))
-              SQL
-          else
-            0
-        end
-      end
-    end
-  end
-
   def update_cached_due_dates
     if due_at_changed? || workflow_state_changed?
       DueDateCacher.recompute(self)

app/models/assignments/needs_grading_count_query.rb

@@ -0,0 +1,81 @@
+module Assignments
+  class NeedsGradingCountQuery
+    attr_reader :assignment, :user
+
+    def initialize(_assignment, _user)
+      @assignment = _assignment
+      @user = _user
+    end
+
+    def count
+      assignment.shard.activate do
+        # the needs_grading_count trigger should change assignment.updated_at, invalidating the cache
+        Rails.cache.fetch(['assignment_user_grading_count', assignment, user].cache_key) do
+          case visibility_level
+          when :full, :limited
+            assignment.needs_grading_count
+          when :sections
+            section_filtered_submissions.count(:id, distinct: true)
+          else
+            0
+          end
+        end
+      end
+    end
+
+    def count_by_section
+      assignment.shard.activate do
+        Rails.cache.fetch(['assignment_user_grading_count_by_section', assignment, user].cache_key) do
+          if visibility_level == :sections
+            submissions = section_filtered_submissions
+          else
+            submissions = all_submissions
+          end
+
+          submissions.
+            group("e.course_section_id").
+            count.
+            map{|k,v| {section_id: k.to_i, needs_grading_count: v}}
+        end
+      end
+    end
+
+    private
+    def section_filtered_submissions
+      all_submissions.where('e.course_section_id in (?)', visible_section_ids)
+    end
+
+    def all_submissions
+      joined_submissions.where(<<-SQL, assignment, course)
+        submissions.assignment_id = ?
+          AND e.course_id = ?
+          AND e.type IN ('StudentEnrollment', 'StudentViewEnrollment')
+          AND e.workflow_state = 'active'
+          AND submissions.submission_type IS NOT NULL
+          AND (submissions.workflow_state = 'pending_review'
+            OR (submissions.workflow_state = 'submitted'
+              AND (submissions.score IS NULL OR NOT submissions.grade_matches_current_submission)))
+        SQL
+    end
+
+    def joined_submissions
+      assignment.submissions.joins("INNER JOIN enrollments e ON e.user_id = submissions.user_id")
+    end
+
+    def visible_section_ids
+      section_visibilities.map{|v| v[:course_section_id]}
+    end
+
+    def visibility_level
+      course.enrollment_visibility_level_for(user, section_visibilities)
+    end
+
+    def section_visibilities
+      course.section_visibilities_for(user)
+    end
+
+    def course
+      assignment.context
+    end
+  end
+end

app/models/user.rb

@@ -1423,7 +1423,7 @@ class User < ActiveRecord::Base
               not_ignored_by(self, 'grading').
               need_grading_info(limit)
             Assignment.send :preload_associations, as, :context
-            as.reject{|a| a.needs_grading_count_for_user(self) == 0}
+            as.reject{|a| Assignments::NeedsGradingCountQuery.new(a, self).count == 0 }
           end
           # outer limit, since there could be limit * n_shards results
           result = result[0...limit] if limit

app/views/courses/_assignments_needing_grading.html.erb

@@ -43,7 +43,9 @@ cache(safe_cache_key([@current_user, contexts, 'a_need_grading']), cache_opts) d
             </span>
           <% end %>
           <b><%= t 'headings.grade', 'Grade %{assignment}', :assignment => assignment.title %></b>
-          <em><%= t 'need_grading_count', { :one => '1 needs grading', :other => '%{count} need grading' }, :count => assignment.needs_grading_count_for_user(@current_user) %></em>
+          <em>
+            <%= t 'need_grading_count', { one: '1 needs grading', other: '%{count} need grading' }, count: Assignments::NeedsGradingCountQuery.new(assignment, @current_user).count %>
+          </em>
         </a>
         <a class='disable_item_link grading' title="<%= t('links.title.ignore', %{Ignore this assignment}) %>" href="#" data-api-href="<%= api_v1_users_todo_ignore_url(assignment.asset_string, 'grading') %>">
           <span class="screenreader-only"><%= t('images.alt.ignore', 'ignore') %></span>

app/views/shared/_menu_assignment.html.erb

@@ -10,7 +10,7 @@
         <% if key == :assignments_needing_submitting %>
           <%= before_label :due, "due" %> <%= datetime_string(menu_assignment.due_at, :due_date, nil, true) %>
         <% elsif key == :assignments_needing_grading %>
-          <%= t :needs_grading_count, { :one => "1 needs grading", :other => "%{count} need grading"}, :count => menu_assignment.needs_grading_count_for_user(@current_user) %>
+          <%= t :needs_grading_count, { one: "1 needs grading", other: "%{count} need grading"}, count: Assignments::NeedsGradingCountQuery.new(menu_assignment, @current_user).count %>
         <% elsif key == :assignments_recently_graded %>
           <% if menu_assignment.grading_type == 'points' %>
               <%= "#{menu_assignment.score}/#{menu_assignment.points_possible}" %>

lib/api/v1/assignment.rb

@@ -61,7 +61,8 @@ module Api::V1::Assignment
     opts.reverse_merge!(
       include_discussion_topic: true,
       include_all_dates: false,
-      override_dates: true
+      override_dates: true,
+      needs_grading_count_by_section: false
     )
 
     if opts[:override_dates] && !assignment.new_record?
@@ -114,7 +115,11 @@ module Api::V1::Assignment
     end
 
     if assignment.grants_right?(user, :grade)
-      hash['needs_grading_count'] = assignment.needs_grading_count_for_user user
+      query = Assignments::NeedsGradingCountQuery.new(assignment, user)
+      if opts[:needs_grading_count_by_section]
+        hash['needs_grading_count_by_section'] = query.count_by_section
+      end
+      hash['needs_grading_count'] = query.count
     end
 
     if assignment.context.grants_any_right?(user, :read_sis, :manage_sis)

lib/api/v1/course_json.rb

@@ -69,7 +69,7 @@ module Api::V1
 
     def needs_grading_count(enrollments, course)
       if include_grading && enrollments && enrollments.any? { |e| e.participating_instructor? }
-        course.assignments.active.to_a.sum{|a| a.needs_grading_count_for_user(user)}
+        course.assignments.active.to_a.sum{|a| Assignments::NeedsGradingCountQuery.new(a, user).count }
       end
     end
 

lib/api/v1/todo_item.rb

@@ -31,7 +31,7 @@ module Api::V1::TodoItem
         "#{course_assignment_url(assignment.context_id, assignment.id)}#submit"
     }).tap do |hash|
       if todo_type == 'grading'
-        hash['needs_grading_count'] = assignment.needs_grading_count_for_user(user)
+        hash['needs_grading_count'] = Assignments::NeedsGradingCountQuery.new(assignment, user).count
       end
     end
   end

lib/sorts_assignments.rb

@@ -63,7 +63,7 @@ class SortsAssignments
     assignments.select do |assignment|
       assignment.grants_right?(user, session, :grade) &&
         assignment.expects_submission? &&
-        assignment.needs_grading_count_for_user(user) > 0
+        Assignments::NeedsGradingCountQuery.new(assignment, user).count > 0
     end
   end
 

spec/apis/v1/assignments_api_spec.rb

@@ -413,6 +413,8 @@ describe AssignmentsApiController, type: :request do
       assign['submission'].should ==
         json_parse(controller.submission_json(submission,assignment,@user,session).to_json)
     end
+
+
     it "returns due dates as they apply to the user" do
         course_with_student(:active_all => true)
         @user = @student
@@ -747,6 +749,47 @@ describe AssignmentsApiController, type: :request do
       @section_override.due_at.to_i.should == @section_due_at.to_i
     end
 
+    it 'accepts configuration argument to split needs grading by section' do
+      student_in_course(:course => @course, :active_enrollment => true)
+      @user = @teacher
+
+      json = api_call(:post, "/api/v1/courses/#{@course.id}/assignments.json",
+        { :controller => 'assignments_api',
+          :action => 'create',
+          :format => 'json',
+          :course_id => @course.id.to_s },
+        { :assignment => {
+            'name' => 'some assignment',
+            'assignment_overrides' => {
+              '0' => {
+                'student_ids' => [@student.id],
+                'title' => 'some title'
+              },
+              '1' => {
+                  'course_section_id' => @course.default_section.id
+                }
+            }
+          }
+        })
+
+      assignments_json = api_call(:get, "/api/v1/courses/#{@course.id}/assignments.json",
+        { controller: 'assignments_api',
+          action: 'index',
+          format: 'json',
+          course_id: @course.id.to_s }, {needs_grading_count_by_section: 'true'})
+      assignments_json[0].keys.should include("needs_grading_count_by_section")
+
+      assignment_id = assignments_json[0]['id']
+      show_json = api_call(:get, "/api/v1/courses/#{@course.id}/assignments/#{assignment_id}.json",
+        { controller: 'assignments_api',
+          action: 'show',
+          format:'json',
+          course_id: @course.id.to_s,
+          id: assignment_id.to_s 
+        }, {needs_grading_count_by_section: 'true'})
+      show_json.keys.should include("needs_grading_count_by_section")
+    end
+
     it "takes overrides into account in the assignment-created notification " +
       "for assignments created with overrides" do
       student_in_course(:course => @course, :active_enrollment => true)

spec/lib/api/v1/assignment_spec.rb

@@ -0,0 +1,43 @@
+require_relative '../../../spec_helper.rb'
+
+class AssignmentApiHarness
+  include Api::V1::Assignment
+  def api_user_content(description, course, user, opts)
+    return "api_user_content(#{description}, #{course.id}, #{user.id})"
+  end
+
+  def course_assignment_url(context_id, assignment)
+    "assignment/url/#{context_id}/#{assignment.id}"
+  end
+
+end
+
+describe "Api::V1::Assignment" do
+
+  describe "#assignment_json" do
+    let(:api) { AssignmentApiHarness.new }
+
+    before do
+      @assignment = assignment_model
+      @user = user_model
+      @assignment.stubs(:grants_right?).returns(true)
+      @session = Object.new
+    end
+
+    it "returns json" do
+      json = api.assignment_json(@assignment, @user, @session, {override_dates: false})
+      json.should be_a(Hash)
+      json["needs_grading_count"].should eq(0)
+      json["needs_grading_count_by_section"].should be_nil
+    end
+
+    it "includes section-based counts when grading flag is passed" do
+      json = api.assignment_json(@assignment, @user, @session,
+               {override_dates: false, needs_grading_count_by_section: true})
+      json.should be_a(Hash)
+      json["needs_grading_count"].should eq(0)
+      json["needs_grading_count_by_section"].should == []
+    end
+  end
+
+end

spec/lib/sorts_assignments_spec.rb

@@ -115,15 +115,19 @@ describe SortsAssignments do
     let(:assignment2) { stub }
     let(:assignment3) { stub }
     let(:assignments) { [ assignment1, assignment2, assignment3 ] }
+    let(:one_count_query){ stub(count: 1)}
+    let(:zero_count_query){ stub(count: 0) }
+    let(:bad_count_query){ stub(count: -1) }
 
     before :each do
       assignments.each { |assignment|
         assignment.stubs(
           :grants_right? => true,
-          :expects_submission? => true,
-          :needs_grading_count_for_user => 1
+          :expects_submission? => true
         )
       }
+
+      Assignments::NeedsGradingCountQuery.stubs(new: one_count_query)
     end
 
     it "only includes assignments that current user has permission to view" do
@@ -140,8 +144,8 @@ describe SortsAssignments do
     end
 
     it "only includes assignments that have a grading_count_for_user > 0" do
-      assignment2.expects(:needs_grading_count_for_user).with(user).returns(-1)
-      assignment3.expects(:needs_grading_count_for_user).with(user).returns(0)
+      Assignments::NeedsGradingCountQuery.stubs(:new).with(assignment2, user).returns(bad_count_query)
+      Assignments::NeedsGradingCountQuery.stubs(:new).with(assignment3, user).returns(zero_count_query)
       SortsAssignments.ungraded_for_user_and_session(assignments,user,session).
         should =~ [ assignment1 ]
     end

spec/models/assignment_spec.rb

@@ -234,47 +234,6 @@ describe Assignment do
     end
   end
 
-  context "needs_grading_count_for_user" do
-    it "should only count submissions in the user's visible section(s)" do
-      @section = @course.course_sections.create!(:name => 'section 2')
-      @user2 = user_with_pseudonym(:active_all => true, :name => 'Student2', :username => 'student2@instructure.com')
-      @section.enroll_user(@user2, 'StudentEnrollment', 'active')
-      @user1 = user_with_pseudonym(:active_all => true, :name => 'Student1', :username => 'student1@instructure.com')
-      @course.enroll_student(@user1).update_attribute(:workflow_state, 'active')
-
-      # enroll a section-limited TA
-      @ta = user_with_pseudonym(:active_all => true, :name => 'TA1', :username => 'ta1@instructure.com')
-      ta_enrollment = @course.enroll_ta(@ta)
-      ta_enrollment.limit_privileges_to_course_section = true
-      ta_enrollment.workflow_state = 'active'
-      ta_enrollment.save!
-
-      # make a submission in each section
-      @assignment = @course.assignments.create(:title => "some assignment", :submission_types => ['online_text_entry'])
-      @assignment.submit_homework @user1, :submission_type => "online_text_entry", :body => "o hai"
-      @assignment.submit_homework @user2, :submission_type => "online_text_entry", :body => "haldo"
-      @assignment.reload
-
-      # check the teacher sees both, the TA sees one
-      @assignment.needs_grading_count_for_user(@teacher).should eql(2)
-      @assignment.needs_grading_count_for_user(@ta).should eql(1)
-
-      # grade an assignment
-      @assignment.grade_student(@user1, :grade => "1")
-      @assignment.reload
-
-      # check that the numbers changed
-      @assignment.needs_grading_count_for_user(@teacher).should eql(1)
-      @assignment.needs_grading_count_for_user(@ta).should eql(0)
-
-      # test limited enrollment in multiple sections
-      @course.enroll_user(@ta, 'TaEnrollment', :enrollment_state => 'active', :section => @section,
-                          :allow_multiple_enrollments => true, :limit_privileges_to_course_section => true)
-      @assignment.reload
-      @assignment.needs_grading_count_for_user(@ta).should eql(1)
-    end
-  end
-
   context "differentiated_assignment visibility" do
     describe "students_with_visibility" do
       before :once do

spec/models/assignments/needs_grading_count_query_spec.rb

@@ -0,0 +1,95 @@
+#
+# Copyright (C) 2014 Instructure, Inc.
+#
+# This file is part of Canvas.
+#
+# Canvas is free software: you can redistribute it and/or modify it under
+# the terms of the GNU Affero General Public License as published by the Free
+# Software Foundation, version 3 of the License.
+#
+# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+# details.
+#
+# You should have received a copy of the GNU Affero General Public License along
+# with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+#
+require_relative "../../spec_helper.rb"
+
+module Assignments
+  describe NeedsGradingCountQuery do
+
+    before :once do
+      course_with_teacher(active_all: true)
+      student_in_course(active_all: true, user_name: "some user")
+    end
+
+    describe "#count" do
+
+      it "should only count submissions in the user's visible section(s)" do
+        @section = @course.course_sections.create!(:name => 'section 2')
+        @user2 = user_with_pseudonym(:active_all => true, :name => 'Student2', :username => 'student2@instructure.com')
+        @section.enroll_user(@user2, 'StudentEnrollment', 'active')
+        @user1 = user_with_pseudonym(:active_all => true, :name => 'Student1', :username => 'student1@instructure.com')
+        @course.enroll_student(@user1).update_attribute(:workflow_state, 'active')
+
+        # enroll a section-limited TA
+        @ta = user_with_pseudonym(:active_all => true, :name => 'TA1', :username => 'ta1@instructure.com')
+        ta_enrollment = @course.enroll_ta(@ta)
+        ta_enrollment.limit_privileges_to_course_section = true
+        ta_enrollment.workflow_state = 'active'
+        ta_enrollment.save!
+
+        # make a submission in each section
+        @assignment = @course.assignments.create(:title => "some assignment", :submission_types => ['online_text_entry'])
+        @assignment.submit_homework @user1, :submission_type => "online_text_entry", :body => "o hai"
+        @assignment.submit_homework @user2, :submission_type => "online_text_entry", :body => "haldo"
+        @assignment.reload
+
+        # check the teacher sees both, the TA sees one
+        NeedsGradingCountQuery.new(@assignment, @teacher).count.should eql(2)
+        NeedsGradingCountQuery.new(@assignment, @ta).count.should eql(1)
+
+        # grade an assignment
+        @assignment.grade_student(@user1, :grade => "1")
+        @assignment.reload
+
+        # check that the numbers changed
+        NeedsGradingCountQuery.new(@assignment, @teacher).count.should eql(1)
+        NeedsGradingCountQuery.new(@assignment, @ta).count.should eql(0)
+
+        # test limited enrollment in multiple sections
+        @course.enroll_user(@ta, 'TaEnrollment', :enrollment_state => 'active', :section => @section,
+                            :allow_multiple_enrollments => true, :limit_privileges_to_course_section => true)
+        @assignment.reload
+        NeedsGradingCountQuery.new(@assignment, @ta).count.should eql(1)
+      end
+
+      it 'breaks them out by section if the by_section flag is passed' do
+        @section = @course.course_sections.create!(:name => 'section 2')
+        @user2 = user_with_pseudonym(:active_all => true, :name => 'Student2', :username => 'student2@instructure.com')
+        @section.enroll_user(@user2, 'StudentEnrollment', 'active')
+        @user1 = user_with_pseudonym(:active_all => true, :name => 'Student1', :username => 'student1@instructure.com')
+        @course.enroll_student(@user1).update_attribute(:workflow_state, 'active')
+
+        @assignment = @course.assignments.create(:title => "some assignment", :submission_types => ['online_text_entry'])
+        @assignment.submit_homework @user1, :submission_type => "online_text_entry", :body => "o hai"
+        @assignment.submit_homework @user2, :submission_type => "online_text_entry", :body => "haldo"
+        @assignment.reload
+
+        NeedsGradingCountQuery.new(@assignment, @teacher).count.should eql(2)
+        sections_grading_counts = NeedsGradingCountQuery.new(@assignment, @teacher).count_by_section
+        sections_grading_counts.should be_a(Array)
+        @course.course_sections.each do |section|
+          sections_grading_counts.should include({
+            section_id: section.id,
+            needs_grading_count: 1
+          })
+        end
+      end
+    end
+
+  end
+end