fix content tag state synchronization

test plan (non-draft-state):
 - as a teacher
   * create each of the following:
     - assignment
     - discussion
     - page (hidden from students)
     - page (not hidden from students)
     - quiz (leaving it unpublished)
   * add each of the above items to a module
   * refresh the modules page
     - each of the above items should be listed
 - as a student
   * navigate to the modules page
     - the hidden page and quiz should not be listed
     - all other items should be listed
   * click on the first item in the module
   * click through each of the 'Next' buttons
     - only items listed in the module should be presented

test plan (draft-state):
 - as a teacher
   * create each of the following:
     - assignment
     - discussion
     - page
     - quiz
   * add each of the above items to a module
   * refresh the modules page
     - each of the above items should be listed
 - as a student
   * navigate to the modules page
     - only published items should be visible
   * click on the first item in the module
   * click through each of the 'Next' buttons
     - only published items should be presented

   * publishing items (as a teacher) should make them available to the
     student in the module progression as well as the modules page

closes CNVS-10831

Change-Id: Ia84e56f42438ff531a4e15784eefaec2ead8ecdd
Reviewed-on: https://gerrit.instructure.com/30312
Reviewed-by: Jeremy Stanley <[email protected]>
Product-Review: Jeremy Stanley <[email protected]>
Tested-by: Jenkins <[email protected]>
QA-Review: Jeremy Stanley <[email protected]>

app/models/assignment.rb

@@ -740,6 +740,12 @@ def locked_for?(user, opts={})
     end
   end
 
+  def clear_locked_cache(user)
+    super
+    Rails.cache.delete(discussion_topic.locked_cache_key(user)) if self.submission_types == 'discussion_topic' && discussion_topic
+    Rails.cache.delete(quiz.locked_cache_key(user)) if self.submission_types == 'online_quiz' && quiz
+  end
+
   def submission_types_array
     (self.submission_types || "").split(",")
   end

app/models/content_tag.rb

@@ -191,43 +191,72 @@ def asset_safe_title(column)
     name
   end
 
+  def self.asset_workflow_state(asset)
+    if asset.respond_to?(:published?)
+      if asset.respond_to?(:deleted?) && asset.deleted?
+        'deleted'
+      elsif asset.published?
+        'active'
+      else
+        'unpublished'
+      end
+    else
+      if asset.respond_to?(:workflow_state)
+        workflow_state = asset.workflow_state.to_s
+        if ['active', 'available', 'published'].include?(workflow_state)
+          'active'
+        elsif ['unpublished', 'deleted'].include?(workflow_state)
+          workflow_state
+        end
+      else
+        nil
+      end
+    end
+  end
+
+  def asset_workflow_state
+    ContentTag.asset_workflow_state(self.content)
+  end
+
+  def asset_context_matches?
+    self.content && self.content.respond_to?(:context) && self.content.context == context
+  end
+
   def update_asset_name!
     return unless self.sync_title_to_asset_title?
-    correct_context = content && content.respond_to?(:context) && content.context == context
-    if correct_context
-      # Assignment proxies name= and name to title= and title, which breaks the asset_safe_title logic
-      if content.respond_to?("name=") && content.respond_to?("name") && !content.is_a?(Assignment)
-        content.name = asset_safe_title('name')
-      elsif content.respond_to?("title=")
-        content.title = asset_safe_title('title')
-      elsif content.respond_to?("display_name=")
-        content.display_name = asset_safe_title('display_name')
-      end
-      content.save if content.changed?
+    return unless self.asset_context_matches?
+
+    # Assignment proxies name= and name to title= and title, which breaks the asset_safe_title logic
+    if content.respond_to?("name=") && content.respond_to?("name") && !content.is_a?(Assignment)
+      content.name = asset_safe_title('name')
+    elsif content.respond_to?("title=")
+      content.title = asset_safe_title('title')
+    elsif content.respond_to?("display_name=")
+      content.display_name = asset_safe_title('display_name')
     end
+    content.save if content.changed?
   end
 
   def update_asset_workflow_state!
     return unless self.sync_workflow_state_to_asset?
-    correct_context = self.content && self.content.respond_to?(:context) && self.content.context == self.context
-    if correct_context
-      asset_workflow_state = nil
-      if self.unpublished? && self.content.respond_to?(:unpublished?)
-        asset_workflow_state = 'unpublished'
-      elsif self.active?
-        if self.content.respond_to?(:active?)
-          asset_workflow_state = 'active'
-        elsif self.content.respond_to?(:available?)
-          asset_workflow_state = 'available'
-        elsif self.content.respond_to?(:published?)
-          asset_workflow_state = 'published'
-        end
-      end
-      if asset_workflow_state
-        self.content.update_attribute(:workflow_state, asset_workflow_state)
-        self.class.update_for(self.content)
+    return unless self.asset_context_matches?
+
+    new_asset_workflow_state = nil
+    if self.unpublished? && self.content.respond_to?(:unpublished?)
+      new_asset_workflow_state = 'unpublished'
+    elsif self.active?
+      if self.content.respond_to?(:active?)
+        new_asset_workflow_state = 'active'
+      elsif self.content.respond_to?(:available?)
+        new_asset_workflow_state = 'available'
+      elsif self.content.respond_to?(:published?)
+        new_asset_workflow_state = 'published'
       end
     end
+    if new_asset_workflow_state
+      self.content.update_attribute(:workflow_state, new_asset_workflow_state)
+      self.class.update_for(self.content)
+    end
   end
 
   def self.delete_for(asset)
@@ -296,15 +325,12 @@ def self.update_for(asset)
     # update workflow_state
     tag_ids = tags.select{|t| t.sync_workflow_state_to_asset? }.map(&:id)
     attr_hash = {:updated_at => Time.now.utc}
-    if asset.respond_to?(:workflow_state)
-      if ['active', 'available', 'published'].include?(asset.workflow_state)
-        attr_hash[:workflow_state] = 'active'
-      elsif ['unpublished', 'deleted'].include?(asset.workflow_state)
-        attr_hash[:workflow_state] = asset.workflow_state
-      end
-    end
+
+    workflow_state = asset_workflow_state(asset)
+    attr_hash[:workflow_state] = workflow_state if workflow_state
     ContentTag.where(:id => tag_ids).update_all(attr_hash) if attr_hash[:workflow_state] && !tag_ids.empty?
 
+    # update the module timestamp
     ContentTag.touch_context_modules(module_ids)
   end
 

app/models/context_module.rb

@@ -249,7 +249,7 @@ def content_tags_visible_to(user)
 
   def add_item(params, added_item=nil, opts={})
     params[:type] = params[:type].underscore if params[:type]
-    position = opts[:position] || (self.content_tags.active.maximum(:position) || 0) + 1
+    position = opts[:position] || (self.content_tags.not_deleted.maximum(:position) || 0) + 1
     if params[:type] == "wiki_page" || params[:type] == "page"
       item = opts[:wiki_page] || self.context.wiki.wiki_pages.find_by_id(params[:id])
     elsif params[:type] == "attachment" || params[:type] == "file"
@@ -261,7 +261,7 @@ def add_item(params, added_item=nil, opts={})
     elsif params[:type] == "quiz"
       item = opts[:quiz] || self.context.quizzes.active.find_by_id(params[:id])
     end
-    workflow_state = item.workflow_state if item && item.respond_to?(:workflow_state) && ['active', 'unpublished'].include?(item.workflow_state)
+    workflow_state = ContentTag.asset_workflow_state(item) if item
     workflow_state ||= 'active'
     if params[:type] == 'external_url'
       title = params[:title]

app/models/discussion_topic.rb

@@ -901,6 +901,12 @@ def locked_for?(user, opts={})
     end
   end
 
+  def clear_locked_cache(user)
+    super
+    Rails.cache.delete(assignment.locked_cache_key(user)) if assignment
+    Rails.cache.delete(root_topic.locked_cache_key(user)) if root_topic
+  end
+
   def self.process_migration(data, migration)
     process_announcements_migration(Array(data['announcements']), migration)
     process_discussion_topics_migration(Array(data['discussion_topics']), migration)

app/models/quizzes/quiz.rb

@@ -771,6 +771,11 @@ def locked_for?(user, opts={})
     end
   end
 
+  def clear_locked_cache(user)
+    super
+    Rails.cache.delete(assignment.locked_cache_key(user)) if self.for_assignment?
+  end
+
   def context_module_action(user, action, points=nil)
     tags_to_update = self.context_module_tags.to_a
     if self.assignment

lib/features/draft_state.rb

@@ -24,8 +24,16 @@ def perform
       course.quizzes.where(workflow_state: 'unpublished').update_all(workflow_state: 'edited')
       course.assignments.where(workflow_state: 'unpublished').update_all(workflow_state: 'published')
       course.context_modules.where(workflow_state: 'unpublished').update_all(workflow_state: 'active')
-      course.context_module_tags.where(workflow_state: 'unpublished').update_all(workflow_state: 'active')
       course.discussion_topics.where(workflow_state: 'unpublished').update_all(workflow_state: 'active')
+
+      # content tags referencing wiki pages or quizzes do not need to be updated
+      # wiki pages and quizzes handle unpublished values correctly in non-draft state
+      course.context_module_tags.where(workflow_state: 'unpublished')
+        .where("content_type NOT IN ('WikiPage', 'Quiz', 'Quizzes::Quiz')")
+        .update_all(workflow_state: 'active')
+
+      # invalidate cache for modules
+      course.context_modules.where("workflow_state<>'deleted'").update_all(updated_at: Time.now.utc)
     end
   end
 end

spec/apis/v1/context_module_items_api_spec.rb

@@ -25,6 +25,7 @@
     @assignment = @course.assignments.create!(:name => "pls submit", :submission_types => ["online_text_entry"], :points_possible => 20)
     @assignment_tag = @module1.add_item(:id => @assignment.id, :type => 'assignment')
     @quiz = @course.quizzes.create!(:title => "score 10")
+    @quiz.publish!
     @quiz_tag = @module1.add_item(:id => @quiz.id, :type => 'quiz')
     @topic = @course.discussion_topics.create!(:message => 'pls contribute')
     @topic_tag = @module1.add_item(:id => @topic.id, :type => 'discussion_topic')

spec/controllers/context_modules_controller_spec.rb

@@ -233,6 +233,7 @@
 
       @module = @course.context_modules.create!
       quiz = @course.quizzes.create!
+      quiz.publish!
 
       tag = @module.add_item :type => 'quiz', :id => quiz.id
 

spec/integration/context_module_spec.rb

@@ -112,7 +112,8 @@ def progression_testing(progress_by_item_link)
         @is_attachment = false
         course_with_student_logged_in(:active_all => true)
         @quiz = @course.quizzes.create!(:title => "new quiz", :shuffle_answers => true)
-
+        @quiz.publish!
+
         # separate timestamps so touch_context will actually invalidate caches
         Timecop.freeze(4.seconds.ago) do
           @mod1 = @course.context_modules.create!(:name => "some module")
@@ -129,33 +130,44 @@ def progression_testing(progress_by_item_link)
           @mod2.save!
         end
 
+        # all modules, tags, etc need to be published
+        @mod1.should be_published
+        @mod2.should be_published
+        @quiz.should be_published
+        @tag1.should be_published
+
         yield '<div id="test_content">yay!</div>'
-
+        @tag2.should be_published
+
+        # verify the second item is locked (doesn't display)
         get @test_url
         response.should be_success
         html = Nokogiri::HTML(response.body)
         html.css('#test_content').length.should == (@test_content_length || 0)
-
-        p1 = @mod1.evaluate_for(@user, true, true)
-
-        @quiz_submission = @quiz.generate_submission(@user)
+
+        # complete first module's requirements
+        p1 = @mod1.evaluate_for(@student, true, true)
+        p1.workflow_state.should == 'unlocked'
+
+        @quiz_submission = @quiz.generate_submission(@student)
         @quiz_submission.grade_submission
         @quiz_submission.workflow_state = 'completed'
         @quiz_submission.kept_score = 1
         @quiz_submission.save!
-    
+
         #emulate settings on progression if the user took the quiz but background jobs haven't run yet
         p1.requirements_met = [{:type=>"min_score", :min_score=>"1", :max_score=>nil, :id=>@quiz.id}]
         p1.save!
-
+
+        # navigate to the second item (forcing update to progression)
         next_link = progress_by_item_link ? 
           "/courses/#{@course.id}/modules/items/#{@tag2.id}" :
           "/courses/#{@course.id}/modules/#{@mod2.id}/items/first"
-
         get next_link
         response.should be_redirect
         response.location.ends_with?(@test_url + "?module_item_id=#{@tag2.id}").should be_true
-
+
+        # verify the second item is no accessible
         get @test_url
         response.should be_success
         html = Nokogiri::HTML(response.body)
@@ -173,6 +185,7 @@ def progression_testing(progress_by_item_link)
           asmnt = @course.assignments.create!(:title => 'assignment', :description => content)
           @test_url = "/courses/#{@course.id}/assignments/#{asmnt.id}"
           @tag2 = @mod2.add_item(:type => 'assignment', :id => asmnt.id)
+          @tag2.should be_published
         end
       end
     end
@@ -183,6 +196,7 @@ def progression_testing(progress_by_item_link)
           discussion = @course.discussion_topics.create!(:title => "topic", :message => content)
           @test_url = "/courses/#{@course.id}/discussion_topics/#{discussion.id}"
           @tag2 = @mod2.add_item(:type => 'discussion_topic', :id => discussion.id)
+          @tag2.should be_published
           @test_content_length = 1
         end
       end
@@ -192,8 +206,10 @@ def progression_testing(progress_by_item_link)
       [true, false].each do |progress_type|
         progression_testing(progress_type) do |content|
           quiz = @course.quizzes.create!(:title => "quiz", :description => content)
+          quiz.publish!
           @test_url = "/courses/#{@course.id}/quizzes/#{quiz.id}"
           @tag2 = @mod2.add_item(:type => 'quiz', :id => quiz.id)
+          @tag2.should be_published
         end
       end
     end
@@ -204,6 +220,7 @@ def progression_testing(progress_by_item_link)
           page = @course.wiki.wiki_pages.create!(:title => "wiki", :body => content)
           @test_url = "/courses/#{@course.id}/wiki/#{page.url}"
           @tag2 = @mod2.add_item(:type => 'wiki_page', :id => page.id)
+          @tag2.should be_published
         end
       end
     end
@@ -215,6 +232,7 @@ def progression_testing(progress_by_item_link)
           att = Attachment.create!(:filename => 'test.html', :display_name => "test.html", :uploaded_data => StringIO.new(content), :folder => Folder.unfiled_folder(@course), :context => @course)
           @test_url = "/courses/#{@course.id}/files/#{att.id}"
           @tag2 = @mod2.add_item(:type => 'attachment', :id => att.id)
+          @tag2.should be_published
         end
       end
     end

spec/lib/draft_state_spec.rb

@@ -33,29 +33,37 @@
     end
 
     it "should kick off a publish job in state change callback" do
-      quiz = t_course.quizzes.create! title: 'a quiz'
-      quiz.workflow_state = 'unpublished'
-      quiz.save!
-
-      page = t_course.wiki.wiki_pages.create title: 'some page'
-      page.workflow_state = 'unpublished'
-      page.save!
+      mod = t_course.context_modules.create name: 'blargh'
+      mod.workflow_state = 'unpublished'
+      mod.save!
 
       topic = t_course.discussion_topics.create title: 'a topic'
       topic.workflow_state = 'unpublished'
       topic.save!
+      topic_item = mod.add_item type: 'discussion_topic', id: topic.id
+      topic_item.workflow_state = 'unpublished'
+      topic_item.save!
 
       assignment = t_course.assignments.create name: 'do this'
       assignment.workflow_state = 'unpublished'
       assignment.save!
+      assignment_item = mod.add_item type: 'assignment', id: assignment.id
+      assignment_item.workflow_state = 'unpublished'
+      assignment_item.save!
 
-      mod = t_course.context_modules.create name: 'blargh'
-      mod.workflow_state = 'unpublished'
-      mod.save!
+      quiz = t_course.quizzes.create! title: 'a quiz'
+      quiz.workflow_state = 'unpublished'
+      quiz.save!
+      quiz_item = mod.add_item type: 'quiz', id: quiz.id
+      quiz_item.workflow_state = 'unpublished'
+      quiz_item.save!
 
-      item = mod.add_item type: 'wiki_page', id: page.id
-      item.workflow_state = 'unpublished'
-      item.save!
+      page = t_course.wiki.wiki_pages.create title: 'some page'
+      page.workflow_state = 'unpublished'
+      page.save!
+      page_item = mod.add_item type: 'wiki_page', id: page.id
+      page_item.workflow_state = 'unpublished'
+      page_item.save!
 
       t_course.disable_feature!(:draft_state)
       expect {
@@ -64,12 +72,21 @@
 
       run_jobs
 
-      quiz.reload.should be_edited
-      page.reload.hide_from_students.should be_true
-      topic.reload.should be_active
-      assignment.reload.should be_published
-      mod.reload.should be_active
-      item.reload.should be_active
+      # make sure everything published properly
+      mod.reload.should             be_published
+
+      topic.reload.should           be_published
+      topic_item.reload.should      be_published
+      assignment.reload.should      be_published
+      assignment_item.reload.should be_published
+
+      # quizzes can be unpublished in non-draft-state, thus should remain unchanged
+      quiz.reload.should            be_unpublished
+      quiz_item.reload.should       be_unpublished
+
+      # pages can be unpublished (hidden from students) in non-draft-state, thus should remain unchanged
+      page.reload.should            be_unpublished
+      page_item.reload.should       be_unpublished
     end
   end