make observers viewing discussions vdd lock date aware

fixes CNVS-518

also included:
- observers now get visible students' section overrides in their
  AssignmentOverride.visible_to scope
- fixed a bug where calling some DatesOverridable methods would
  use an overridden date where you wouldn't expect (see the specs)
- added a method to get the original object from an overridden one
- made DiscussionTopicPresenter handle due dates overridden to nil

test plan notes
- keep an eye out for regressions in displayed due dates
- discussion locking behavior should be as follows
-- viewing the discussion page before the earliest applicable
   unlock date should show a locked discussion page that lists
   the earliest unlock date
-- viewing the discussion after the earliest unlock date should
   show the discussion
-- the discussion page should show the due date when only one
   due date applies
-- the discussion page should show a "multiple due dates" ui when
   more than one due date applies
-- the "multiple due dates" ui should only display entries for
   sections that the observer's linked students are in
-- the "multiple due dates" ui should display lock dates from the
   associated section override or from the original assignment
   if the section override does not override lock dates

test plan
- check an observer not observing student
-- ensure that the discussion locking behavior behaves according
   to the observer's section's override
- check an observer observing one student
-- ensure that the discussion locking behaves according to the
   student's section's override
- check an observer observing multiple student's in more than one
  section
-- make overrides for each student's section that differ in due
   dates and lock dates
-- ensure that the discussion locking behaves according to the
   students' sections' combined lock dates
-- ensure that the discussion page shows a "multiple due dates"
   ui when the discussion is unlocked

Change-Id: I8f2970f0962cdc60cf9a423f01a876bf0ae909d4
Reviewed-on: https://gerrit.instructure.com/17452
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Marc LeGendre <marc@instructure.com>
Reviewed-by: Mark Ericksen <marke@instructure.com>

app/models/assignment_override.rb

@@ -256,6 +256,12 @@ class AssignmentOverride < ActiveRecord::Base
       :joins => "INNER JOIN assignment_overrides ON assignment_overrides.set_type='CourseSection' AND course_sections.id=assignment_overrides.set_id"
     )
 
+    # section overrides for visible students
+    scopes << course.enrollments_visible_to(admin).scoped(
+      :select => "assignment_overrides.id",
+      :joins => "INNER JOIN assignment_overrides ON assignment_overrides.set_type='CourseSection' AND enrollments.course_section_id=assignment_overrides.set_id"
+    )
+
     # union the visible override subselects and join against them
     subselect = scopes.map{ |scope| scope.construct_finder_sql({}) }.join(' UNION ')
     { :joins => "INNER JOIN (#{subselect}) AS visible_overrides ON visible_overrides.id=assignment_overrides.id" }

app/presenters/discussion_topic_presenter.rb

@@ -7,28 +7,27 @@ class DiscussionTopicPresenter
     @topic = discussion_topic
     @user  = current_user
 
-    @assignment = if topic.assignment
-      AssignmentOverrideApplicator.assignment_overridden_for(topic.assignment,
-                                                             user)
+    @assignment = if @topic.for_assignment?
+      AssignmentOverrideApplicator.assignment_overridden_for(@topic.assignment, @user)
     else
       nil
     end
   end
 
+  # Public: Return a presenter using an unoverridden copy of the topic's assignment
+  #
+  # Returns a DiscussionTopicPresenter
+  def unoverridden
+    self.class.new(@topic, nil)
+  end
+  
   # Public: Return a date string for the discussion assignment's lock at date.
   #
   # due_date - A due date as a hash.
   #
   # Returns a date or date/time string.
   def lock_at(due_date = {})
-    lock_at = if due_date[:override].present?
-      due_date[:override].lock_at
-    else
-      assignment.try(:lock_at)
-    end
-
-    formatted_lock_at = lock_at.present? ? datetime_string(lock_at) : '-'
-    formatted_lock_at.match(/11:59/) ? date_string(lock_at) : formatted_lock_at
+    formatted_date_string(:lock_at, due_date)
   end
 
   # Public: Return a date string for the discussion assignment's unlock at date.
@@ -37,14 +36,7 @@ class DiscussionTopicPresenter
   #
   # Returns a date or date/time string.
   def unlock_at(due_date = {})
-    unlock_at = if due_date[:override].present?
-      due_date[:override].unlock_at
-    else
-      assignment.try(:unlock_at)
-    end
-
-    formatted_unlock_at = unlock_at.present? ? datetime_string(unlock_at) : '-'
-    formatted_unlock_at.match(/11:59/) ? date_string(unlock_at) : formatted_unlock_at
+    formatted_date_string(:unlock_at, due_date)
   end
 
   # Public: Return a date string for the given due date.
@@ -53,16 +45,24 @@ class DiscussionTopicPresenter
   #
   # Returns a date or date/time string.
   def due_at(due_date = {})
-    due_at = if due_date[:override].present?
-      due_date[:override].due_at
-    else
-      assignment.try(:due_at)
-    end
-
-    formatted_due_at = due_at.present? ? datetime_string(due_at) : '-'
-    formatted_due_at.match(/11:59/) ? date_string(due_at) : formatted_due_at
+    formatted_date_string(:due_at, due_date)
   end
 
+  def formatted_date_string(date_field, date_hash = {})
+    date = date_hash[:override].try(date_field) if date_hash[:override].present?
+    date ||= assignment.try(date_field)
+ 
+    formatted_date = date.present? ? datetime_string(date) : '-'
+    formatted_date.match(/11:59/) ? date_string(date) : formatted_date
+  end
+
+  # Public: Determine if multiple due dates are visible to user.
+  #
+  # Returns a boolean
+  def multiple_due_dates?
+    assignment.multiple_due_dates_apply_to?(user)
+  end
+  
   # Public: Return all due dates visible to user, filtering out assignment info
   #   if it isn't needed (e.g. if all sections have overrides).
   #
@@ -76,8 +76,13 @@ class DiscussionTopicPresenter
     if section_overrides.count > 0 && section_overrides.count == topic.context.course_sections.count
       due_dates.delete_if { |d| d[:override].nil? }
     end
-
-    due_dates.sort_by { |date| date[:due_at] }
+    
+    # Sort by due_at, nils and earliest first
+    due_dates.sort{ |date1, date2|
+      due_at1 = date1[:due_at]
+      due_at2 = date2[:due_at]
+      (due_at1 and due_at2) ? due_at1 <=> due_at2 : (due_at1 ? 1 : -1)
+    }
   end
 
   # Public: Determine if the given user has permissions to manage this discussion.

app/views/discussion_topics/_assignment_details.html.erb

@@ -14,7 +14,7 @@
     <div class="span6 right-align">
       <% if @assignment.try(:due_at).present? %>
         <div class="discussion-pubdate">
-          <% if @assignment.multiple_due_dates? %>
+          <% if @presenter.multiple_due_dates? %>
             <a href="#" class="toggle_due_dates"><%= t(:show_due_dates, 'Show Due Dates') %></a>
 
           <% else %>
@@ -58,10 +58,10 @@
           <tbody>
             <% @presenter.visible_due_dates.each do |due_date| %>
               <tr>
-                <td><%= @presenter.due_at(due_date) %></td>
+                <td><%= @presenter.unoverridden.due_at(due_date) %></td>
                 <td><%= due_date[:title] || t(:everyone, 'Everyone else') %></td>
-                <td><%= @presenter.unlock_at(due_date) %></td>
-                <td><%= @presenter.lock_at(due_date) %></td>
+                <td><%= @presenter.unoverridden.unlock_at(due_date) %></td>
+                <td><%= @presenter.unoverridden.lock_at(due_date) %></td>
               </tr>
             <% end %>
           </tbody>

lib/assignment_override_applicator.rb

@@ -25,9 +25,7 @@ module AssignmentOverrideApplicator
 
     overrides = self.overrides_for_assignment_and_user(assignment_or_quiz, user)
  
-    result_assignment_or_quiz = self.assignment_with_overrides(assignment_or_quiz, overrides) unless overrides.empty?
-    result_assignment_or_quiz ||= assignment_or_quiz
-    result_assignment_or_quiz.overridden = true
+    result_assignment_or_quiz = self.assignment_with_overrides(assignment_or_quiz, overrides)
     result_assignment_or_quiz.overridden_for_user = user
     result_assignment_or_quiz
   end
@@ -111,24 +109,29 @@ module AssignmentOverrideApplicator
   # assignment or quiz which can then be used in place of the original object.
   # the clone is marked readonly to prevent saving
   def self.assignment_with_overrides(assignment_or_quiz, overrides)
+    unoverridden_assignment_or_quiz = assignment_or_quiz.without_overrides
     # ActiveRecord::Base#clone nils out the primary key; put it back
-    cloned_assignment_or_quiz = assignment_or_quiz.clone
-    cloned_assignment_or_quiz.id = assignment_or_quiz.id
+    cloned_assignment_or_quiz = unoverridden_assignment_or_quiz.clone
+    cloned_assignment_or_quiz.id = unoverridden_assignment_or_quiz.id
 
     # update attributes with overrides
-    self.collapsed_overrides(assignment_or_quiz, overrides).each do |field,value|
-      # for any times in the value set, bring them back from raw UTC into the
-      # current Time.zone before placing them in the assignment
-      value = value.in_time_zone if value && value.respond_to?(:in_time_zone) && !value.is_a?(Date)
-      cloned_assignment_or_quiz.write_attribute(field, value)
+    if overrides
+      self.collapsed_overrides(unoverridden_assignment_or_quiz, overrides).each do |field,value|
+        # for any times in the value set, bring them back from raw UTC into the
+        # current Time.zone before placing them in the assignment
+        value = value.in_time_zone if value && value.respond_to?(:in_time_zone) && !value.is_a?(Date)
+        cloned_assignment_or_quiz.write_attribute(field, value)
+      end
     end
     cloned_assignment_or_quiz.applied_overrides = overrides
+    cloned_assignment_or_quiz.without_overrides = unoverridden_assignment_or_quiz
+    cloned_assignment_or_quiz.overridden = true
     cloned_assignment_or_quiz.readonly!
 
     # make new_record? match the original (typically always true on AR clones,
     # at least until saved, which we don't want to do)
     klass = class << cloned_assignment_or_quiz; self; end
-    klass.send(:define_method, :new_record?) { assignment_or_quiz.new_record? }
+    klass.send(:define_method, :new_record?) { unoverridden_assignment_or_quiz.new_record? }
 
     cloned_assignment_or_quiz
   end

lib/dates_overridable.rb

@@ -1,5 +1,6 @@
 module DatesOverridable
   attr_accessor :applied_overrides, :overridden_for_user, :overridden
+  attr_writer :without_overrides
 
   def self.included(base)
     base.has_many :assignment_overrides, :dependent => :destroy
@@ -29,6 +30,10 @@ module DatesOverridable
     assignment_overrides.count > 0
   end
 
+  def without_overrides
+    @without_overrides || self
+  end
+
   # returns two values indicating which due dates for this assignment apply
   # and/or are visible to the user.
   #
@@ -50,7 +55,7 @@ module DatesOverridable
     return nil, nil if context.nil?
 
     if user.nil?
-      return self.due_date_hash, nil
+      return self.without_overrides.due_date_hash, nil
     end
 
     if context.user_has_been_student?(user)
@@ -76,7 +81,7 @@ module DatesOverridable
 
   def all_due_dates
     all_dates = assignment_overrides.overriding_due_at.map(&:as_hash)
-    all_dates << due_date_hash.merge(:base => true)
+    all_dates << without_overrides.due_date_hash.merge(:base => true)
   end
 
   def due_dates_visible_to(user)
@@ -85,7 +90,7 @@ module DatesOverridable
     list = overrides.map(&:as_hash)
 
     # Base
-    list << self.due_date_hash.merge(:base => true)
+    list << without_overrides.due_date_hash.merge(:base => true)
   end
 
   def observed_student_due_dates(user)
@@ -165,7 +170,7 @@ module DatesOverridable
 
       as_instructor << {
         :base => true,
-        :unlock_at => self.unlock_at
+        :unlock_at => self.without_overrides.unlock_at
       }
     end
 
@@ -194,7 +199,7 @@ module DatesOverridable
 
       as_instructor << {
         :base => true,
-        :lock_at => self.lock_at
+        :lock_at => self.without_overrides.lock_at
       }
     end
 

spec/lib/assignment_override_applicator_spec.rb

@@ -721,6 +721,20 @@ describe AssignmentOverrideApplicator do
     end
   end
 
+  describe "without_overrides" do
+    before :each do
+      student_in_course
+      @assignment = assignment_model(:course => @course)
+    end
+
+    it "should return an unoverridden copy of an overridden assignment" do
+      @overridden_assignment = AssignmentOverrideApplicator.assignment_overridden_for(@assignment, @student)
+      @overridden_assignment.overridden_for_user.id.should == @student.id
+      @unoverridden_assignment = @overridden_assignment.without_overrides
+      @unoverridden_assignment.overridden_for_user.should == nil
+    end
+  end
+  
   it "should use the full stack" do
     student_in_course
     original_due_at = 3.days.from_now

spec/lib/dates_overridable_spec.rb

@@ -72,6 +72,12 @@ shared_examples_for "an object whose dates are overridable" do
     end
   end
 
+  describe "without_overrides" do
+    it "returns an object with no overrides applied" do
+      overridable.without_overrides.overridden.should be_false
+    end
+  end
+    
   describe "#overrides_visible_to(user)" do
     before :each do
       override.set = course.default_section
@@ -156,6 +162,11 @@ shared_examples_for "an object whose dates are overridable" do
       end
     end
 
+    it "doesn't use an overridden due date for a nil user's due dates" do
+      as_student, _ = overridable.overridden_for(@student).due_dates_for(nil)
+      as_student[:due_at].should == overridable.due_at
+    end
+
     it "includes the base due date in the list of due dates" do
       _, as_instructor = overridable.due_dates_for(@teacher)
 
@@ -288,6 +299,11 @@ shared_examples_for "an object whose dates are overridable" do
       as_instructor.should include({ :base => true, :unlock_at => overridable.unlock_at })
     end
 
+    it "doesn't use an overridden unlock date as the base unlock date" do
+      _, as_instructor = overridable.overridden_for(@student).unlock_ats_for(@teacher)
+      as_instructor.should include({ :base => true, :unlock_at => overridable.unlock_at})
+    end
+
     it "includes visible unlock date overrides in the list of unlock dates" do
       _, as_instructor = overridable.unlock_ats_for(@teacher)
       as_instructor.should include({
@@ -377,6 +393,11 @@ shared_examples_for "an object whose dates are overridable" do
       as_instructor.should include({ :base => true, :lock_at => overridable.lock_at })
     end
 
+    it "doesn't use an overridden lock date as the base lock date" do
+      _, as_instructor = overridable.overridden_for(@student).lock_ats_for(@teacher)
+      as_instructor.should include({ :base => true, :lock_at => overridable.lock_at})
+    end
+
     it "includes visible lock date overrides in the list of lock dates" do
       _, as_instructor = overridable.lock_ats_for(@teacher)
       as_instructor.detect { |a| a[:override].present? }.should == {
@@ -517,4 +538,4 @@ describe Quiz do
 
   let(:overridable) { quiz_model(:due_at => 5.days.ago) }
   let(:overridable_type) { :quiz }
-end
+end

spec/presenters/discussion_topic_presenter_spec.rb

@@ -19,16 +19,26 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
 
 describe DiscussionTopicPresenter do
-  let (:presenter)  { DiscussionTopicPresenter.new(topic) }
-  let (:course)     { course_model }
   let (:topic)      { DiscussionTopic.new(:title => 'Test Topic', :assignment => assignment) }
+  let (:presenter)  { DiscussionTopicPresenter.new(topic, user_model) }
+  let (:course)     { course_model }
   let (:assignment) {
     Assignment.new(:title => 'Test Topic',
                    :due_at => Time.now,
                    :lock_at => Time.now + 1.week,
-                   :unlock_at => Time.now - 1.week)
+                   :unlock_at => Time.now - 1.week,
+                   :submission_types => 'discussion_topic')
   }
 
+  it 'should override the topic assignment when given a user' do
+    topic.for_assignment?.should == 0
+    presenter.assignment.overridden_for_user.id.should == @user.id
+  end
+  
+  it 'should present an unoverridden copy' do
+    presenter.unoverridden.assignment.overridden_for_user.should be_nil
+  end
+
   context 'a topic with no overrides' do
     context 'with dates' do
       it 'should present lock_at' do