adhoc overrides take student names

fixes CNVS-18412
fixes CNVS-19474

also fixes an N+1 during assignment saving
also fixes bug validating assignment override students

test plan:
  - make an adhoc assignment override via the API
    (pass in 5 student_ids in your course as
     "student_ids", dont pass "title")
  - on the assignment index when you look at the
    due_dates popup, it should list two students names
    and "and 3 others"
  - on the assignment show page it does the same
  - repeat with only 3 students, it should just list
    their names
    - note: the assignment edit page wont display
      the override, that is okay (for now)
  - also save an assignment with various combinations
    of due dates for sections - see if it ever errors
    on save

Change-Id: I19e8f186849ee9a2dd1a9055c9ddb0df2a470c5a
Reviewed-on: https://gerrit.instructure.com/48468
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins
QA-Review: Adam Stone <astone@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>

app/coffeescripts/models/Assignment.coffee

@@ -267,6 +267,14 @@ define [
       dateGroups = @get("all_dates")
       dateGroups && dateGroups.length > 1
 
+    nonBaseDates: =>
+      dateGroups = @get("all_dates")
+      return false unless dateGroups
+      withouBase = _.filter(dateGroups.models, (dateGroup) =>
+        dateGroup && !dateGroup.get("base")
+      )
+      withouBase.length > 0
+
     allDates: =>
       groups = @get("all_dates")
       models = (groups and groups.models) or []
@@ -295,7 +303,7 @@ define [
         'frozenAttributes', 'freezeOnCopy', 'canFreeze', 'isSimple',
         'gradingStandardId', 'isLetterGraded', 'isGpaScaled', 'assignmentGroupId', 'iconType',
         'published', 'htmlUrl', 'htmlEditUrl', 'labelId', 'position', 'postToSIS',
-        'multipleDueDates', 'allDates', 'isQuiz', 'singleSectionDueDate'
+        'multipleDueDates', 'nonBaseDates', 'allDates', 'isQuiz', 'singleSectionDueDate'
       ]
       if ENV.DIFFERENTIATED_ASSIGNMENTS_ENABLED
         fields.push 'isOnlyVisibleToOverrides'

app/coffeescripts/models/Quiz.coffee

@@ -113,6 +113,14 @@ define [
       dateGroups = @get("all_dates")
       dateGroups && dateGroups.length > 1
 
+    nonBaseDates: =>
+      dateGroups = @get("all_dates")
+      return false unless dateGroups
+      withouBase = _.filter(dateGroups, (dateGroup) ->
+        dateGroup && !dateGroup.get("base")
+      )
+      withouBase.length > 0
+
     allDates: =>
       groups = @get("all_dates")
       models = (groups and groups.models) or []
@@ -127,7 +135,7 @@ define [
 
     toView: =>
       fields = [
-        'htmlUrl', 'multipleDueDates', 'allDates', 'dueAt', 'lockAt', 'unlockAt', 'singleSectionDueDate'
+        'htmlUrl', 'multipleDueDates', 'nonBaseDates', 'allDates', 'dueAt', 'lockAt', 'unlockAt', 'singleSectionDueDate'
       ]
       hash = id: @get 'id'
       for field in fields

app/coffeescripts/views/assignments/CreateAssignmentView.coffee

@@ -91,7 +91,7 @@ define [
       super
 
       timeField = @$el.find(".datetime_field")
-      if @model.multipleDueDates() || @model.isOnlyVisibleToOverrides()
+      if @model.multipleDueDates() || @model.isOnlyVisibleToOverrides() || @model.nonBaseDates()
         timeField.tooltip
           position: {my: 'center bottom', at: 'center top-10', collision: 'fit fit'},
           tooltipClass: 'center bottom vertical',

app/models/assignment_override.rb

@@ -122,7 +122,7 @@ class AssignmentOverride < ActiveRecord::Base
       self.assignment_version = assignment.version_number if assignment
     end
 
-    self.title = set.name if set_type != 'ADHOC' && set
+    set_title_if_needed
   end
   protected :default_values
 
@@ -250,6 +250,30 @@ class AssignmentOverride < ActiveRecord::Base
       self.due_at_overridden && !Assignment.due_dates_equal?(self.due_at, self.prior_version.due_at))
   end
 
+  def set_title_if_needed
+    return if self.workflow_state == "deleted"
+
+    if set_type != 'ADHOC' && set
+      self.title = set.name
+    elsif set_type == 'ADHOC' && set.any?
+      self.title ||= title_from_students(set)
+    end
+  end
+
+  def title_from_students(students)
+    sorted_students = (students || []).sort_by(&:name)
+    if sorted_students.count > 3
+      others_count = sorted_students.count - 2
+      first_two_students = sorted_students[0..1].map(&:name).join(", ")
+      I18n.t(
+        '%{first_two_students}, and %{others_count} others',
+        {first_two_students: first_two_students, others_count: others_count}
+      )
+    elsif sorted_students.any?
+      sorted_students.map(&:name).to_sentence
+    end
+  end
+
   has_a_broadcast_policy
   set_broadcast_policy do |p|
     p.dispatch :assignment_due_date_changed

app/views/jst/assignments/CreateAssignment.handlebars

@@ -37,7 +37,7 @@
         {{#t "assignment_due_at"}}Due:{{/t}}
       </label>
       <div class="controls">
-      {{#ifAny multipleDueDates isOnlyVisibleToOverrides}}
+      {{#ifAny multipleDueDates isOnlyVisibleToOverrides nonBaseDates}}
         <span class="datetime_field multiple_due_dates" title
               data-tooltip-selector="#vdd_tooltip_{{uniqLabel}}"
               aria-labelledby="vdd_tooltip_{{uniqLabel}}">

lib/api/v1/assignment_override.rb

@@ -92,9 +92,6 @@ module Api::V1::AssignmentOverride
         errors << "student_ids are not valid for group assignments"
       else
         set_type = 'ADHOC'
-
-        # require a title along with student ids on create
-        errors << "title required with student_ids" unless data[:title]
       end
     end
 
@@ -106,7 +103,7 @@ module Api::V1::AssignmentOverride
         students = []
       else
         # look up the students
-        students = api_find_all(assignment.context.students_visible_to(@current_user), student_ids)
+        students = api_find_all(assignment.context.students_visible_to(@current_user), student_ids).uniq
 
         # make sure they were all valid
         found_ids = students.map{ |s| [
@@ -153,7 +150,6 @@ module Api::V1::AssignmentOverride
 
     errors << "one of student_ids, group_id, or course_section_id is required" if !set_type && errors.empty?
 
-    # title of the adhoc override
     if set_type == 'ADHOC' && data.has_key?(:title)
       override_data[:title] = data[:title]
     end
@@ -211,8 +207,10 @@ module Api::V1::AssignmentOverride
       override.set = override_data[:section]
     end
 
-    if override.set_type == 'ADHOC' && override_data.has_key?(:title)
-      override.title = override_data[:title] || override.title
+    if override.set_type == 'ADHOC'
+      override.title = override_data[:title] ||
+                         override.title_from_students(override_data[:students]) ||
+                         override.title
     end
 
     [:due_at, :unlock_at, :lock_at].each do |field|
@@ -234,47 +232,48 @@ module Api::V1::AssignmentOverride
     return false
   end
 
-  def batch_update_assignment_overrides(assignment, overrides)
-    # extract list of kept/new overrides with their interpreted data (applied
-    # but not saved) and track defunct override to delete
-    defunct_override_ids = assignment.assignment_overrides.map(&:id).to_set
-    overrides = overrides.map do |override_params|
-      # find/build override
-      override = find_assignment_override(assignment, override_params[:id])
-      if override
-        defunct_override_ids.delete(override.id)
-      else
-        override = assignment.assignment_overrides.build
-        override.dont_touch_assignment = true
-      end
+  def batch_update_assignment_overrides(assignment, overrides_params)
+    override_param_ids = overrides_params.map{ |ov| ov[:id] }
+    existing_overrides = assignment.assignment_overrides.active
 
-      # interpret and apply the data
+    remaining_overrides = destroy_defunct_overrides(assignment, override_param_ids, existing_overrides)
+    ActiveRecord::Associations::Preloader.new(remaining_overrides, :assignment_override_students).run
+
+    overrides = overrides_params.map do |override_params|
+      override = get_override_from_params(override_params, assignment, remaining_overrides)
       data, errors = interpret_assignment_override_data(assignment, override_params, override.set_type)
       if errors
         override.errors.add(errors)
       else
         update_assignment_override_without_save(override, data)
       end
-
       override
     end
 
-    # delete the defunct overrides first, so that they don't get in the way if
-    # a new override targets the set of a deleted one
-    unless defunct_override_ids.empty?
-      assignment.assignment_overrides.
-        where(:id => defunct_override_ids.to_a).
-        each(&:destroy)
-    end
-
-    # stop now if there were validation errors
     raise ActiveRecord::RecordInvalid.new(assignment) unless assignment.valid?
 
-    # save the new/kept overrides
-    overrides.each{ |override| override.save! }
+    overrides.each(&:save!)
   end
 
-  def deserialize_overrides( overrides )
+  def destroy_defunct_overrides(assignment, override_param_ids, existing_overrides)
+    defunct_override_ids =  existing_overrides.map(&:id) - override_param_ids.map(&:to_i)
+    return existing_overrides if defunct_override_ids.empty?
+
+    assignment.assignment_overrides.where(:id => defunct_override_ids).destroy_all
+    existing_overrides.reject{|override| defunct_override_ids.include?(override.id)}
+  end
+  private :destroy_defunct_overrides
+
+  def get_override_from_params(override_params, assignment, potential_overrides)
+    potential_overrides.detect { |ov|
+      ov.id == override_params[:id].to_i
+    } || assignment.assignment_overrides.build.tap { |ov|
+      ov.dont_touch_assignment = true
+    }
+  end
+  private :get_override_from_params
+
+  def deserialize_overrides(overrides)
     if overrides.is_a?(Hash)
       return unless overrides.keys.all?{ |k| k.to_i.to_s == k.to_s }
       indices = overrides.keys.sort_by(&:to_i)

lib/dates_overridable.rb

@@ -101,7 +101,7 @@ module DatesOverridable
 
     if all_dates
       # remove base if all sections are set
-      overrides = all_dates.select { |d| d[:set_type] == 'CourseSection' }
+      overrides = all_dates.select{ |d| d[:set_type] == 'CourseSection' }
       if overrides.count > 0 && overrides.count == context.active_course_sections.count
         all_dates.delete_if {|d| d[:base] }
       end

spec/apis/v1/assignment_overrides_spec.rb

@@ -381,11 +381,6 @@ describe AssignmentOverridesController, type: :request do
         expect_error("unknown student ids: [\"#{@bad_id}\"]")
       end
 
-      it "should error without a title for an adhoc assignment override" do
-        raw_api_create_override(@course, @assignment, :assignment_override => { :student_ids => [@student.id] })
-        expect_error('title required with student_ids')
-      end
-
       it "should error if the assignment is a group assignment" do
         @assignment.group_category = @course.group_categories.create!(name: "foo")
         @assignment.save!
@@ -393,6 +388,51 @@ describe AssignmentOverridesController, type: :request do
         raw_api_create_override(@course, @assignment, :assignment_override => { :student_ids => [@student.id], :title => @title })
         expect_error('student_ids are not valid for group assignments')
       end
+
+      context "title" do
+        before :once do
+          @override = assignment_override_model
+          names = ["Adam Aardvark", "Ben Banana", "Chipmunk Charlie", "Donald Duck", "Erik Erikson", "Freddy Frog"]
+          @students = names.map do |name|
+            student_in_course(course: @course, :user => user_with_pseudonym(name: name)).user
+          end
+          @course.reload
+        end
+
+        it "should concat students names if there are fewer than 4" do
+          student_ids = @students[0..1].map(&:id)
+          api_create_override(@course, @assignment, :assignment_override => { :student_ids => student_ids})
+          @override = @assignment.assignment_overrides(true).first
+          expect(@override.title).to eq("Adam Aardvark and Ben Banana")
+        end
+
+        it "should add an others count if there are more than 4" do
+          student_ids = @students.map(&:id)
+          api_create_override(@course, @assignment, :assignment_override => { :student_ids => student_ids})
+          @override = @assignment.assignment_overrides(true).first
+          expect(@override.title).to eq("Adam Aardvark, Ben Banana, and 4 others")
+        end
+
+        it "should alphabetize the students names" do
+          reversed_student_ids = @students.reverse.map(&:id)
+
+          api_create_override(@course, @assignment, :assignment_override => { :student_ids => reversed_student_ids})
+          @override = @assignment.assignment_overrides(true).first
+
+          expect(@override.title).to eq("Adam Aardvark, Ben Banana, and 4 others")
+        end
+
+        it "should prefer a given title" do
+          student_ids = @students.map(&:id)
+          api_create_override(
+            @course,
+            @assignment,
+            :assignment_override => { :student_ids => student_ids, title: "Preferred Title"}
+          )
+          @override = @assignment.assignment_overrides(true).first
+          expect(@override.title).to eq("Preferred Title")
+        end
+      end
     end
 
     context "group" do

spec/apis/v1/assignments_api_spec.rb

@@ -892,6 +892,59 @@ describe AssignmentsApiController, type: :request do
       expect(response.code).to eql '400'
     end
 
+    it "allows creating an assignment with overrides via the API" do
+      student_in_course(:course => @course, :active_enrollment => true)
+
+      @adhoc_due_at = 5.days.from_now
+      @section_due_at = 7.days.from_now
+
+      @user = @teacher
+
+      assignment_params = {
+        :assignment => {
+          'name' => 'some assignment',
+          'assignment_overrides' => {
+            '0' => {
+              'student_ids' => [@student.id],
+              'due_at' => @adhoc_due_at.iso8601 },
+            '1' => {
+                'course_section_id' => @course.default_section.id,
+                'due_at' => @section_due_at.iso8601
+              }
+          }
+        }
+      }
+
+      controller_params = {
+        :controller => 'assignments_api',
+        :action => 'create',
+        :format => 'json',
+        :course_id => @course.id.to_s
+      }
+
+      @json = api_call(
+        :post,
+        "/api/v1/courses/#{@course.id}/assignments.json",
+        controller_params,
+        assignment_params
+      )
+
+      @assignment = Assignment.find @json['id']
+      expect(@assignment.assignment_overrides.count).to eq 2
+
+      @adhoc_override = @assignment.assignment_overrides.where(set_type: 'ADHOC').first
+      expect(@adhoc_override).not_to be_nil
+      expect(@adhoc_override.set).to eq [@student]
+      expect(@adhoc_override.due_at_overridden).to be_truthy
+      expect(@adhoc_override.due_at.to_i).to eq @adhoc_due_at.to_i
+      expect(@adhoc_override.title).to eq @student.name
+
+      @section_override = @assignment.assignment_overrides.where(set_type: 'CourseSection').first
+      expect(@section_override).not_to be_nil
+      expect(@section_override.set).to eq @course.default_section
+      expect(@section_override.due_at_overridden).to be_truthy
+      expect(@section_override.due_at.to_i).to eq @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)

spec/models/assignment_override_spec.rb

@@ -286,11 +286,33 @@ describe AssignmentOverride do
       expect(@override.title).to eq @group.name
     end
 
-    it "should not be changed for adhoc sets" do
+    it "should not be changed for adhoc sets if there are no students" do
       @override.title = 'Other Value'
       @override.valid? # trigger bookkeeping
       expect(@override.title).to eq 'Other Value'
     end
+
+    it "should set ADHOC's title to reflect students (with few)" do
+      @override.title = nil
+      @override.set_type = "ADHOC"
+      override_student = @override.assignment_override_students.build
+      override_student.user = student_in_course(course: @override.assignment.context, name: 'Edgar Jones').user
+      override_student.save!
+      @override.valid? # trigger bookkeeping
+      expect(@override.title).to eq 'Edgar Jones'
+    end
+
+    it "should set ADHOC's name to reflect students (with many)" do
+      @override.title = nil
+      @override.set_type = "ADHOC"
+      ["A Student","B Student","C Student","D Student"].each do |student_name|
+        override_student = @override.assignment_override_students.build
+        override_student.user = student_in_course(course: @override.assignment.context, name: student_name).user
+        override_student.save!
+      end
+      @override.valid? # trigger bookkeeping
+      expect(@override.title).to eq 'A Student, B Student, and 2 others'
+    end
   end
 
   def self.describe_override(field, value1, value2)