master courses: link graded discussions/quizzes with assignments

should use the same migration_id + master/child content tags
so they'll be restricted/unlinked together

closes #MC-63

Change-Id: I815c76d79b7407ecbcb2bff88dea9c90188800eb
Reviewed-on: https://gerrit.instructure.com/99887
Reviewed-by: Dan Minkevitch <dan@instructure.com>
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
Product-Review: James Williams  <jamesw@instructure.com>
QA-Review: James Williams  <jamesw@instructure.com>
Tested-by: Jenkins

app/controllers/assignment_groups_controller.rb

@@ -406,6 +406,10 @@ class AssignmentGroupsController < ApplicationController
       assignments.each { |a| a.has_no_overrides = true if a.assignment_overrides.size == 0 }
     end
 
+    if master_courses? && context.grants_right?(@current_user, session, :manage_assignments)
+      MasterCourses::Restrictor.preload_restrictions(assignments)
+    end
+
     assignments
   end
 

app/models/assignment.rb

@@ -45,11 +45,14 @@ class Assignment < ActiveRecord::Base
 
   include MasterCourses::Restrictor
   restrict_columns :content, [:title, :description]
-  restrict_columns :settings, [:points_possible, :assignment_group_id,
+
+  RESTRICTED_SETTINGS = [:points_possible, :assignment_group_id,
     :grading_type, :omit_from_final_grade, :submission_types,
     :group_category, :group_category_id,
     :grade_group_students_individually, :peer_reviews,
-    :moderated_grading]
+    :moderated_grading,
+    :due_at, :lock_at, :unlock_at, :peer_reviews_due_at].freeze
+  restrict_columns :settings, RESTRICTED_SETTINGS
 
   has_many :submissions, :dependent => :destroy
   has_many :provisional_grades, :through => :submissions
@@ -1022,6 +1025,17 @@ class Assignment < ActiveRecord::Base
     ].include?(self.submission_types)
   end
 
+  def submittable_object
+    case self.submission_types
+    when 'online_quiz'
+      self.quiz
+    when 'discussion_topic'
+      self.discussion_topic
+    when 'wiki_page'
+      self.wiki_page
+    end
+  end
+
   def each_submission_type
     if block_given?
       submittable_types = %i(discussion_topic quiz)
@@ -1860,7 +1874,8 @@ class Assignment < ActiveRecord::Base
 
   scope :include_submittables, -> { preload(:quiz, :discussion_topic, :wiki_page) }
 
-  scope :no_submittables, -> { where.not(submission_types: %w(online_quiz discussion_topic wiki_page)) }
+  SUBMITTABLE_TYPES = %w(online_quiz discussion_topic wiki_page).freeze
+  scope :no_submittables, -> { where.not(submission_types: SUBMITTABLE_TYPES) }
 
   scope :with_submissions, -> { preload(:submissions) }
 

app/models/discussion_topic.rb

@@ -37,6 +37,7 @@ class DiscussionTopic < ActiveRecord::Base
   restrict_columns :content, [:title, :message]
   restrict_columns :settings, [:delayed_post_at, :require_initial_post, :discussion_type,
                                :lock_at, :pinned, :locked, :allow_rating, :only_graders_can_rate, :sort_by_rating]
+  restrict_columns :settings, Assignment::RESTRICTED_SETTINGS
 
   attr_accessor :user_has_posted, :saved_by
 

app/models/importers/quiz_importer.rb

@@ -255,7 +255,7 @@ module Importers
       item.reload # reload to catch question additions
 
       if hash[:assignment]
-        if hash[:assignment][:migration_id]
+        if hash[:assignment][:migration_id] && !hash[:assignment][:migration_id].start_with?(MasterCourses::MIGRATION_ID_PREFIX)
           item.assignment ||= Quizzes::Quiz.where(context_type: context.class.to_s, context_id: context, migration_id: hash[:assignment][:migration_id]).first
         end
         item.assignment = nil if item.assignment && item.assignment.quiz && item.assignment.quiz.id != item.id

app/models/master_courses/master_template.rb

@@ -69,6 +69,9 @@ class MasterCourses::MasterTemplate < ActiveRecord::Base
   end
 
   def migration_id_for(obj, prepend="")
+    if obj.is_a?(Assignment) && submittable = obj.submittable_object
+      obj = submittable # i.e. use the same migration id as the topic on a graded topic's assignment - same restrictions
+    end
     key = obj.is_a?(ActiveRecord::Base) ? obj.global_asset_string : obj.to_s
     "#{MasterCourses::MIGRATION_ID_PREFIX}#{self.shard.id}_#{self.id}_#{Digest::MD5.hexdigest(prepend + key)}"
   end

app/models/master_courses/restrictor.rb

@@ -74,8 +74,13 @@ module MasterCourses::Restrictor
 
     changed_columns ||= self.changes.keys & self.class.base_class.restricted_column_settings.values.flatten
     if changed_columns.any?
+      if self.is_a?(Assignment) && submittable = self.submittable_object
+        tag_content = submittable # mark on the owner's tag
+      else
+        tag_content = self
+      end
       MasterCourses::ChildContentTag.transaction do
-        child_tag = MasterCourses::ChildContentTag.all.polymorphic_where(:content => self).lock.first
+        child_tag = MasterCourses::ChildContentTag.all.polymorphic_where(:content => tag_content).lock.first
         if child_tag
           new_changes = changed_columns - child_tag.downstream_changes
           if new_changes.any?
@@ -100,7 +105,7 @@ module MasterCourses::Restrictor
     return unless @importing_migration && is_child_content?
 
     child_tag = @importing_migration.master_course_subscription.content_tag_for(self) # find or create it
-    return unless child_tag.downstream_changes.present?
+    return unless child_tag && child_tag.downstream_changes.present?
 
     restrictions = nil
     columns_to_restore = []

app/models/master_courses/tag_helper.rb

@@ -19,6 +19,10 @@ module MasterCourses::TagHelper
 
   def content_tag_for(content, defaults={})
     return unless MasterCourses::ALLOWED_CONTENT_TYPES.include?(content.class.base_class.name)
+    if content.is_a?(Assignment) && submittable = content.submittable_object
+      content = submittable # use one child tag
+    end
+
     if @content_tag_index
       tag = (@content_tag_index[content.class.base_class.name] || {})[content.id]
       unless tag
@@ -33,6 +37,7 @@ module MasterCourses::TagHelper
   end
 
   def create_content_tag_for!(content, defaults={})
+    return if content.is_a?(Assignment) && Assignment::SUBMITTABLE_TYPES.include?(content.submission_types)
     self.class.unique_constraint_retry do |retry_count|
       tag = nil
       tag = self.content_tags.polymorphic_where(:content => content).first if retry_count > 0

app/models/quizzes/quiz.rb

@@ -92,6 +92,7 @@ class Quizzes::Quiz < ActiveRecord::Base
     :ip_filter, :require_lockdown_browser, :require_lockdown_browser_for_results,
     :lock_at, :unlock_at
   ]
+  restrict_columns :settings, Assignment::RESTRICTED_SETTINGS
 
   # override has_one relationship provided by simply_versioned
   def current_version_unidirectional
@@ -418,6 +419,7 @@ class Quizzes::Quiz < ActiveRecord::Base
         end
         @notify_of_update ||= a.workflow_state_changed? && a.published?
         a.notify_of_update = @notify_of_update
+        a.mark_as_importing!(@importing_migration) if @importing_migration
         a.with_versioning(false) do
           @notify_of_update ? a.save : a.save_without_broadcasting!
         end

app/models/wiki_page.rb

@@ -37,6 +37,7 @@ class WikiPage < ActiveRecord::Base
   include MasterCourses::Restrictor
   restrict_columns :content, [:body, :title]
   restrict_columns :settings, [:editing_roles]
+  restrict_columns :settings, Assignment::RESTRICTED_SETTINGS
 
   after_update :post_to_pandapub_when_revised
 

lib/api/v1/assignment_group.rb

@@ -57,9 +57,6 @@ module Api::V1::AssignmentGroup
           EffectiveDueDates.for_course(group.context, assignments).to_hash([:in_closed_grading_period])
       end
 
-      check_for_restrictions = master_courses? && group.context.grants_right?(user, session, :manage_assignments)
-      MasterCourses::Restrictor.preload_restrictions(assignments) if check_for_restrictions
-
       hash['assignments'] = assignments.map { |a|
         overrides = opts[:overrides].select{|override| override.assignment_id == a.id } unless opts[:overrides].nil?
         a.context = group.context
@@ -77,7 +74,7 @@ module Api::V1::AssignmentGroup
           include_overrides: opts[:include_overrides],
           needs_grading_course_proxy: needs_grading_course_proxy,
           submission: includes.include?('submission') ? opts[:submissions][a.id] : nil,
-          include_master_course_restrictions: check_for_restrictions
+          include_master_course_restrictions: master_courses? && group.context.grants_right?(user, session, :manage_assignments)
         )
 
         unless opts[:exclude_response_fields].include?('in_closed_grading_period')

spec/models/master_courses/master_migration_spec.rb

@@ -462,6 +462,76 @@ describe MasterCourses::MasterMigration do
       expect(copied_qq.reload.question_data['question_text']).to eq new_master_text
     end
 
+    it "should handle graded quizzes/discussions/etc better" do
+      @copy_to = course_factory
+      sub = @template.add_child_course!(@copy_to)
+
+      old_due_at = 5.days.from_now
+
+      quiz_assmt = @copy_from.assignments.create!(:due_at => old_due_at, :submission_types => 'online_quiz').reload
+      quiz = quiz_assmt.quiz
+      topic = @copy_from.discussion_topics.new
+      topic.assignment = @copy_from.assignments.build(:due_at => old_due_at)
+      topic.save!
+      topic_assmt = topic.assignment
+
+      run_master_migration
+
+      copied_quiz = @copy_to.quizzes.where(:migration_id => mig_id(quiz)).first
+      copied_quiz_assmt = copied_quiz.assignment
+      expect(copied_quiz_assmt.migration_id).to eq copied_quiz.migration_id # should use the same migration id = same restrictions
+      copied_topic = @copy_to.discussion_topics.where(:migration_id => mig_id(topic)).first
+      copied_topic_assmt = copied_topic.assignment
+      expect(copied_topic_assmt.migration_id).to eq copied_topic.migration_id # should use the same migration id = same restrictions
+
+      new_title = "new master title"
+      quiz.update_attribute(:title, new_title)
+      topic.update_attribute(:title, new_title)
+      [quiz, topic].each {|c| c.class.where(:id => c).update_all(:updated_at => 2.seconds.from_now)} # ensure it gets copied
+
+      run_master_migration
+
+      expect(copied_quiz_assmt.reload.title).to eq new_title # should carry the new title over to the assignments
+      expect(copied_topic_assmt.reload.title).to eq new_title
+
+      expect(sub.child_content_tags.count).to eq 2
+      quiz_child_tag = sub.child_content_tags.polymorphic_where(:content => copied_quiz).first
+      topic_child_tag = sub.child_content_tags.polymorphic_where(:content => copied_topic).first
+      [quiz_child_tag, topic_child_tag].each do |tag|
+        expect(tag.downstream_changes).to be_empty
+      end
+
+      new_child_due_at = 7.days.from_now
+      copied_quiz.update_attribute(:due_at, new_child_due_at)
+      copied_topic_assmt.update_attribute(:due_at, new_child_due_at)
+
+      [quiz_child_tag, topic_child_tag].each do |tag|
+        expect(tag.reload.downstream_changes).to include('due_at') # store the downstream changes on
+      end
+
+      new_master_due_at = 10.days.from_now
+      quiz.update_attribute(:due_at, new_master_due_at)
+      topic_assmt.update_attribute(:due_at, new_master_due_at)
+      [quiz, topic].each {|c| c.class.where(:id => c).update_all(:updated_at => 2.seconds.from_now)} # ensure it gets copied
+
+      run_master_migration # re-copy all the content - but don't actually overwrite anything because it got changed downstream
+
+      expect(copied_quiz_assmt.reload.due_at.to_i).to eq new_child_due_at.to_i # didn't get overwritten
+      expect(copied_topic_assmt.reload.due_at.to_i).to eq new_child_due_at.to_i # didn't get overwritten
+
+      [quiz, topic].each do |c|
+        mtag = @template.content_tag_for(c)
+        Timecop.freeze(2.seconds.from_now) do
+          mtag.update_attribute(:restrictions, {:settings => true}) # lock the quiz/topic master tags
+        end
+      end
+
+      run_master_migration # now, overwrite the due_at's because the tags are locked
+
+      expect(copied_quiz_assmt.reload.due_at.to_i).to eq new_master_due_at.to_i # should have gotten overwritten
+      expect(copied_topic_assmt.reload.due_at.to_i).to eq new_master_due_at.to_i
+    end
+
     context "master courses + external migrations" do
       class TestExternalContentService
         cattr_reader :course, :imported_content