ignore scores for unpublished assignments

fixes CNVS-10244

test plan:
- create a new course with a teacher and student
- create two assignments, and publish them
- grade them both
- unpublish one of them
- look at all the places you can see grades, and make sure they all
  consistently show only the grades for the published assignment (the
  unpublished assignment should be completely ignored)
  * GB2 with and without 'treat ungraded as 0'
  * GB1 with and without 'incluce ungraded'
  * GB csv download
  * student grades summary page with and without what-if scores
  * /grades page as a student and as a teacher

Change-Id: I32aca28fe377e59cb2e0c6278b59eb74c87c73b4
Reviewed-on: https://gerrit.instructure.com/28047
Reviewed-by: Cameron Matheson <cameron@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Caleb Guanzon <cguanzon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

app/controllers/gradebooks_controller.rb

@@ -76,7 +76,8 @@ class GradebooksController < ApplicationController
 
   def light_weight_ags_json(assignment_groups)
     assignment_groups.map do |ag|
-      assignments = ag.active_assignments.map do |a|
+      assignment_scope = AssignmentGroup.assignment_scope_for_grading(@context)
+      assignments = ag.send(assignment_scope).map do |a|
         {
           :id => a.id,
           :submission_types => a.submission_types_array,
@@ -242,7 +243,8 @@ class GradebooksController < ApplicationController
           "grade_group_students_individually"].map do |field|
             "assignments.#{field}"
         end
-        render :json => @context.assignments.active.gradeable.
+        workflow_scope = @context.feature_enabled?(:draft_state) ? :published : :active
+        render :json => @context.assignments.send(workflow_scope).gradeable.
           select(assignment_fields + ["group_categories.name as group_category", "quizzes.id as quiz_id"]).
           joins("LEFT OUTER JOIN group_categories ON group_categories.id=assignments.group_category_id").
           joins("LEFT OUTER JOIN quizzes on quizzes.assignment_id=assignments.id") + groups_as_assignments
@@ -507,8 +509,12 @@ class GradebooksController < ApplicationController
 
 
   def assignment_groups_json
-    @context.assignment_groups.active.map { |g|
-      assignment_group_json(g, @current_user, session, ['assignments'], {stringify_json_ids: stringify_json_ids?})
+    assignment_scope = AssignmentGroup.assignment_scope_for_grading(@context)
+    @context.assignment_groups.active.includes(assignment_scope).map { |g|
+      assignment_group_json(g, @current_user, session, ['assignments'], {
+        stringify_json_ids: stringify_json_ids?,
+        assignment_group_assignment_scope: assignment_scope
+      })
     }
   end
 end

app/models/assignment.rb

@@ -236,7 +236,10 @@ class Assignment < ActiveRecord::Base
   end
 
   def update_grades_if_details_changed
-    if @points_possible_was != self.points_possible || @grades_affected || @muted_was != self.muted
+    if @points_possible_was != self.points_possible ||
+        @grades_affected ||
+        @muted_was != self.muted ||
+        workflow_state_changed?
       connection.after_transaction_commit { self.context.recompute_student_scores }
     end
     true

app/models/assignment_group.rb

@@ -272,4 +272,11 @@ class AssignmentGroup < ActiveRecord::Base
     self.reload
   end
 
+  def self.assignment_scope_for_grading(context)
+    if context.feature_enabled?(:draft_state)
+      :published_assignments
+    else
+      :active_assignments
+    end
+  end
 end

app/presenters/grade_summary_presenter.rb

@@ -104,7 +104,7 @@ class GradeSummaryPresenter
   end
 
   def relevant_assignments_scope
-   @context.feature_enabled?(:draft_state) ? :published_assignments : :active_assignments
+    AssignmentGroup.assignment_scope_for_grading(@context)
   end
 
   def submissions

lib/api/v1/assignment_group.rb

@@ -37,23 +37,17 @@ module Api::V1::AssignmentGroup
     hash['rules'] = group.rules_hash(stringify_json_ids: opts[:stringify_json_ids])
 
     if includes.include?('assignments')
-      assignment_scope = group.active_assignments
-
-      # fake assignment used for checking if the @current_user can read unpublished assignments
-      fake = group.context.assignments.new
-      fake.workflow_state = 'unpublished'
-      if group.context.feature_enabled?(:draft_state) && !fake.grants_right?(user, session, :read)
-        # user should not see unpublished assignments
-        assignment_scope = assignment_scope.published
-      end
+      assignment_scope   = opts[:assignment_group_assignment_scope]
+      assignment_scope ||= assignment_group_assignment_scope(group.context, user)
+      assignments = group.send(assignment_scope)
 
       user_content_attachments   = opts[:preloaded_user_content_attachments]
       user_content_attachments ||= api_bulk_load_user_content_attachments(
-        assignment_scope.map(&:description),
+        assignments.map(&:description),
         group.context,
         user
       )
-      hash['assignments'] = assignment_scope.map { |a|
+      hash['assignments'] = assignments.map { |a|
         a.context = group.context
         assignment_json(a, user, session,
           include_discussion_topic: includes.include?('discussion_topic'),
@@ -80,4 +74,18 @@ module Api::V1::AssignmentGroup
 
     assignment_group.save
   end
+
+  def assignment_group_assignment_scope(context, user)
+    scope = :active_assignments
+
+    # fake assignment used for checking if the @current_user can read unpublished assignments
+    fake = context.assignments.new
+    fake.workflow_state = 'unpublished'
+    if context.feature_enabled?(:draft_state) && !fake.grants_right?(user, session, :read)
+      # user should not see unpublished assignments
+      scope = :published_assignments
+    end
+
+    scope
+  end
 end

lib/grade_calculator.rb

@@ -26,10 +26,9 @@ class GradeCalculator
       @course = course :
       @course = Course.find(course)
     @course_id = @course.id
-    @groups = @course.assignment_groups.active.includes(:assignments)
-    @assignments = @groups.map(&:assignments).flatten.select { |a|
-      a.graded? && a.active?
-    }
+    assignment_scope = AssignmentGroup.assignment_scope_for_grading(@course)
+    @groups = @course.assignment_groups.active.includes(assignment_scope)
+    @assignments = @groups.flat_map(&assignment_scope).select(&:graded?)
     @user_ids = Array(user_ids).map(&:to_i)
     @current_updates = []
     @final_updates = []

spec/controllers/gradebooks_controller_spec.rb

@@ -349,6 +349,30 @@ describe GradebooksController do
         data.first(2).sort_by{ |a| a['assignment']['title'] }.map{ |a| a['assignment']['group_category'] }.
           should == [assignment1, assignment2].map{ |a| a.group_category.name }
       end
+
+      context "draft state" do
+        it "should should not return unpublished assignments" do
+          course_with_teacher_logged_in(:active_all => true)
+          @course.account.enable_feature!(:draft_state)
+          ag = @course.assignment_groups.create! group_weight: 100
+          a1 = ag.assignments.create! :submission_types => 'online_upload',
+                                      :points_possible  => 10,
+                                      :context  => @course
+          a2 = ag.assignments.build :title => "unpub assign",
+                                    :submission_types => 'online_upload',
+                                    :points_possible  => 10,
+                                    :context  => @course
+          a2.workflow_state = 'unpublished'
+          a2.save!
+
+          get 'show', :course_id => @course.id, :init => 1, :assignments => 1, :format => 'json'
+          response.should be_success
+          data = json_parse
+          data.should_not be_nil
+          data.size.should == 3 # 1 assignment (no unpublished) + an assignment group + a total
+          data.map{|a| a['assignment']['title']}.include?(a2.title).should be_false
+        end
+      end
     end
 
     describe "csv" do
@@ -378,6 +402,10 @@ describe GradebooksController do
         assign_ids.include?(assignment2.id).should be_true
         assign_ids.include?("group-#{assignment2.assignment_group.id}").should be_true
         assign_ids.include?("final-grade").should be_true
+
+        ag_assign_ids = assigns[:js_env][:assignment_groups].first['assignments'].map{|a| a['id']}
+        ag_assign_ids.include?(assignment1.id).should be_false
+        ag_assign_ids.include?(assignment2.id).should be_true
       end
     end
   end
@@ -561,6 +589,7 @@ describe GradebooksController do
       a  = ag.assignments.create! :submission_types => 'online_upload',
                                   :points_possible  => 10,
                                   :context  => @course
+      @controller.instance_variable_set(:@context, @course)
       @controller.light_weight_ags_json([ag]).should == [
         {
           id: ag.id,
@@ -576,5 +605,37 @@ describe GradebooksController do
         },
       ]
     end
+
+    context 'draft state' do
+      it 'should not return unpublished assignments' do
+        course_with_teacher(:active_all => true)
+        @course.account.enable_feature!(:draft_state)
+        ag = @course.assignment_groups.create! group_weight: 100
+        a1 = ag.assignments.create! :submission_types => 'online_upload',
+                                    :points_possible  => 10,
+                                    :context  => @course
+        a2 = ag.assignments.build :submission_types => 'online_upload',
+                                  :points_possible  => 10,
+                                  :context  => @course
+        a2.workflow_state = 'unpublished'
+        a2.save!
+
+      @controller.instance_variable_set(:@context, @course)
+      @controller.light_weight_ags_json([ag]).should == [
+        {
+          id: ag.id,
+          rules: {},
+          group_weight: 100,
+          assignments: [
+            {
+              id: a1.id,
+              points_possible: 10,
+              submission_types: ['online_upload'],
+            }
+          ],
+        },
+      ]
+      end
+    end
   end
 end

spec/lib/grade_calculator_spec.rb

@@ -336,7 +336,7 @@ describe GradeCalculator do
 
     it "should treat muted assignments as if there is no submission" do
       # should have same scores as previous spec despite having a grade
-      nil_graded_assignment()
+      nil_graded_assignment
 
       @assignment_1.mute!
       @assignment_1.grade_student(@user, :grade => 500)
@@ -352,6 +352,29 @@ describe GradeCalculator do
       @user.enrollments.first.computed_current_score.should eql(58.3)
       @user.enrollments.first.computed_final_score.should eql(48.4)
     end
+
+    context "draft state" do
+      it "should not include unpublished assignments when draft state is enabled" do
+        two_graded_assignments
+
+        @course.account.enable_feature!(:draft_state)
+        @assignment2.unpublish
+
+        @user.reload
+        @user.enrollments.first.computed_current_score.should eql(40.0)
+        @user.enrollments.first.computed_final_score.should eql(40.0)
+      end
+
+      it "should include unpublished assignments when draft state is disabled" do
+        two_graded_assignments
+
+        @assignment2.unpublish
+
+        @user.reload
+        @user.enrollments.first.computed_current_score.should eql(60.0)
+        @user.enrollments.first.computed_final_score.should eql(60.0)
+      end
+    end
   end
 
   it "should return grades in the order they are requested" do

spec/models/assignment_spec.rb

@@ -2550,6 +2550,37 @@ describe Assignment do
       @assignment.readable_submission_types.should == 'on paper'
     end
   end
+
+  describe '#update_grades_if_details_changed' do
+    before do
+      assignment_model
+    end
+
+    it "should update grades if points_possible changes" do
+      @assignment.context.expects(:recompute_student_scores).once
+      @assignment.points_possible = 3
+      @assignment.save!
+    end
+
+    it "should update grades if muted changes" do
+      @assignment.context.expects(:recompute_student_scores).once
+      @assignment.muted = true
+      @assignment.save!
+    end
+
+    it "should update grades if workflow_state changes" do
+      @assignment.context.expects(:recompute_student_scores).once
+      @assignment.unpublish
+    end
+
+    it "should not update grades otherwise" do
+      @assignment.context.expects(:recompute_student_scores).never
+      @assignment.title = 'hi'
+      @assignment.due_at = 1.hour.ago
+      @assignment.description = 'blah'
+      @assignment.save!
+    end
+  end
 end
 
 def setup_assignment_with_group