diff --git a/app/controllers/courses_controller.rb b/app/controllers/courses_controller.rb index e3579885877..c2a9b55f05e 100644 --- a/app/controllers/courses_controller.rb +++ b/app/controllers/courses_controller.rb @@ -1438,7 +1438,7 @@ class CoursesController < ApplicationController if value_to_boolean(lock_announcements) @course.lock_all_announcements = true Announcement.where(:context_type => 'Course', :context_id => @course, :workflow_state => 'active'). - update_all(:workflow_state => 'locked') + update_all(:locked => true) elsif @course.lock_all_announcements @course.lock_all_announcements = false end diff --git a/app/controllers/discussion_topics_controller.rb b/app/controllers/discussion_topics_controller.rb index 0c9f128afbd..e4a187b09f9 100644 --- a/app/controllers/discussion_topics_controller.rb +++ b/app/controllers/discussion_topics_controller.rb @@ -482,8 +482,8 @@ class DiscussionTopicsController < ApplicationController @topic.lock_at = discussion_topic_hash[:lock_at] if params.has_key? :lock_at if @topic.delayed_post_at_changed? || @topic.lock_at_changed? - @topic.workflow_state = @topic.desired_workflow_state - + @topic.workflow_state = if @topic.should_not_post_yet then 'post_delayed' else 'active' end + if @topic.should_lock_yet then @topic.lock else @topic.unlock end # Handle locking/unlocking (overrides workflow state if provided). It appears that the locked param as a hash # is from old code and is not being used. Verification requested. elsif params.has_key?(:locked) && !params[:locked].is_a?(Hash) diff --git a/app/models/announcement.rb b/app/models/announcement.rb index d8796dcd879..4c6bec76384 100644 --- a/app/models/announcement.rb +++ b/app/models/announcement.rb @@ -41,7 +41,7 @@ class Announcement < DiscussionTopic protected :infer_content def respect_context_lock_rules - lock if active? && + lock if !locked? && context.is_a?(Course) && context.lock_all_announcements? end diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb index 4bb6d1ef649..3b6ec168f67 100644 --- a/app/models/discussion_topic.rb +++ b/app/models/discussion_topic.rb @@ -27,7 +27,7 @@ class DiscussionTopic < ActiveRecord::Base attr_accessible :title, :message, :user, :delayed_post_at, :lock_at, :assignment, :plaintext_message, :podcast_enabled, :podcast_has_student_posts, - :require_initial_post, :threaded, :discussion_type, :context, :pinned + :require_initial_post, :threaded, :discussion_type, :context, :pinned, :locked module DiscussionTypes SIDE_COMMENT = 'side_comment' @@ -112,8 +112,8 @@ class DiscussionTopic < ActiveRecord::Base end def schedule_delayed_transitions - self.send_at(self.delayed_post_at, :auto_update_workflow) if @should_schedule_delayed_post - self.send_at(self.lock_at, :auto_update_workflow) if @should_schedule_lock_at + self.send_at(self.delayed_post_at, :update_based_on_date) if @should_schedule_delayed_post + self.send_at(self.lock_at, :update_based_on_date) if @should_schedule_lock_at end def update_subtopics @@ -357,55 +357,53 @@ class DiscussionTopic < ActiveRecord::Base scope :by_position, order("discussion_topics.position DESC, discussion_topics.created_at DESC") scope :by_last_reply_at, order("discussion_topics.last_reply_at DESC, discussion_topics.created_at DESC") - def auto_update_workflow - transition_to_workflow_state(desired_workflow_state) - end - alias_method :try_posting_delayed, :auto_update_workflow - - # Determine the desired workflow_state based on current values of delayed_post_at and lock_at - # - # 'delayed_post' if delayed_post_at < now - # 'locked' if lock_at is in the past - def desired_workflow_state(time_to_check = Time.now) - if self.delayed_post_at && time_to_check < self.delayed_post_at - 'post_delayed' - elsif self.lock_at && self.lock_at < time_to_check - 'locked' - else - 'active' - end + def should_lock_yet + self.lock_at && self.lock_at < Time.now end - # Attempts to make valid transitions to the desired workflow state. - # This can move it forward along delayed -> active -> locked, but not - # backward. - def transition_to_workflow_state(desired_state) - if desired_state != 'post_delayed' - self.delayed_post - end - if desired_state == 'locked' - self.lock - end + def should_not_post_yet + self.delayed_post_at && self.delayed_post_at > Time.now end + # There may be delayed jobs that expect to call this to update the topic, so be sure to alias + # the old method name if you change it + def update_based_on_date + lock if should_lock_yet + delayed_post unless should_not_post_yet + end + alias_method :try_posting_delayed, :update_based_on_date + alias_method :auto_update_workflow, :update_based_on_date + workflow do - state :active do - event :lock, :transitions_to => :locked do - raise "cannot lock before due date" if self.assignment.try(:due_at) && self.assignment.due_at > Time.now - end - end + state :active state :post_delayed do event :delayed_post, :transitions_to => :active do self.last_reply_at = Time.now self.posted_at = Time.now end end - state :locked do - event :unlock, :transitions_to => :active - end state :deleted end + def lock + raise "cannot lock before due date" if self.assignment.try(:due_at) && self.assignment.due_at > Time.now + self.locked = true + save! + end + alias_method :lock!, :lock + + def unlock + self.locked = false + self.workflow_state = 'active' if self.workflow_state == 'locked' + save! + end + alias_method :unlock!, :unlock + + def locked? + return workflow_state == 'locked' if locked.nil? + locked + end + def should_send_to_stream if self.delayed_post_at && self.delayed_post_at > Time.now false @@ -557,13 +555,13 @@ class DiscussionTopic < ActiveRecord::Base given { |user| self.user && self.user == user and self.discussion_entries.active.empty? && !self.locked? && !self.root_topic_id && context.user_can_manage_own_discussion_posts?(user) } can :delete - given { |user, session| (self.active? || self.locked?) && self.cached_context_grants_right?(user, session, :read_forum) }# + given { |user, session| self.active? && self.cached_context_grants_right?(user, session, :read_forum) }# can :read - given { |user, session| self.active? && self.cached_context_grants_right?(user, session, :post_to_forum) }#students.include?(user) } + given { |user, session| self.active? && !self.locked? && self.cached_context_grants_right?(user, session, :post_to_forum) }#students.include?(user) } can :reply and can :read - given { |user, session| (self.active? || self.locked?) && self.cached_context_grants_right?(user, session, :post_to_forum) }#students.include?(user) } + given { |user, session| self.active? && self.cached_context_grants_right?(user, session, :post_to_forum) }#students.include?(user) } can :read given { |user, session| diff --git a/db/migrate/20130702104734_add_locked_flag_to_discussion_topics.rb b/db/migrate/20130702104734_add_locked_flag_to_discussion_topics.rb new file mode 100644 index 00000000000..acd18fe409b --- /dev/null +++ b/db/migrate/20130702104734_add_locked_flag_to_discussion_topics.rb @@ -0,0 +1,11 @@ +class AddLockedFlagToDiscussionTopics < ActiveRecord::Migration + tag :predeploy + + def self.up + add_column 'discussion_topics', 'locked', :boolean + end + + def self.down + remove_column 'discussion_topics', 'locked' + end +end diff --git a/spec/apis/v1/discussion_topics_api_spec.rb b/spec/apis/v1/discussion_topics_api_spec.rb index 346d16bb46d..fefd34d867d 100644 --- a/spec/apis/v1/discussion_topics_api_spec.rb +++ b/spec/apis/v1/discussion_topics_api_spec.rb @@ -390,7 +390,8 @@ describe DiscussionTopicsController, :type => :integration do it "should not unlock topic if lock_at changes but is still in the past" do lock_at = 1.month.ago new_lock_at = 1.week.ago - @topic.workflow_state = 'locked' + @topic.workflow_state = 'active' + @topic.locked = true @topic.lock_at = lock_at @topic.save! @@ -404,7 +405,8 @@ describe DiscussionTopicsController, :type => :integration do it "should update workflow_state if delayed_post_at changed to future" do post_at = 1.month.from_now - @topic.workflow_state = 'locked' + @topic.workflow_state = 'active' + @topic.locked = true @topic.save! api_call(:put, "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}", @@ -434,7 +436,8 @@ describe DiscussionTopicsController, :type => :integration do old_lock_at = 1.month.ago new_lock_at = 1.month.from_now @topic.lock_at = old_lock_at - @topic.workflow_state = 'locked' + @topic.workflow_state = 'active' + @topic.locked = true @topic.save! api_call(:put, "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}", @@ -444,6 +447,7 @@ describe DiscussionTopicsController, :type => :integration do @topic.reload @topic.lock_at.to_i.should == new_lock_at.to_i @topic.should be_active + @topic.should_not be_locked end it "should lock the topic if lock_at is changed to the past" do @@ -474,6 +478,7 @@ describe DiscussionTopicsController, :type => :integration do @topic.reload @topic.lock_at.should be_nil @topic.should be_active + @topic.should_not be_locked end it 'should process html content in message on update' do diff --git a/spec/models/discussion_topic_spec.rb b/spec/models/discussion_topic_spec.rb index 144d202a5c4..8b5a679c262 100644 --- a/spec/models/discussion_topic_spec.rb +++ b/spec/models/discussion_topic_spec.rb @@ -236,7 +236,7 @@ describe DiscussionTopic do topic.stream_item.should_not be_nil end - describe "#auto_update_workflow" do + describe "#update_based_on_date" do before do course_with_student(:active_all => true) @user.register @@ -247,8 +247,9 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => Time.now - 1.day, :lock_at => nil) - topic.auto_update_workflow - topic.workflow_state.should eql 'active' + topic.update_based_on_date + topic.workflow_state.should eql 'active' + topic.locked?.should be_false end it "should be post_delayed when delayed_post_at is in the future" do @@ -256,8 +257,9 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => Time.now + 1.day, :lock_at => nil) - topic.auto_update_workflow + topic.update_based_on_date topic.workflow_state.should eql 'post_delayed' + topic.locked?.should be_false end it "should be locked when lock_at is in the past" do @@ -265,8 +267,8 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => nil, :lock_at => Time.now - 1.day) - topic.auto_update_workflow - topic.workflow_state.should eql 'locked' + topic.update_based_on_date + topic.locked?.should be_true end it "should be active when lock_at is in the future" do @@ -274,8 +276,9 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => nil, :lock_at => Time.now + 1.day) - topic.auto_update_workflow + topic.update_based_on_date topic.workflow_state.should eql 'active' + topic.locked?.should be_false end it "should be active when now is between delayed_post_at and lock_at" do @@ -283,8 +286,9 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => Time.now - 1.day, :lock_at => Time.now + 1.day) - topic.auto_update_workflow + topic.update_based_on_date topic.workflow_state.should eql 'active' + topic.locked?.should be_false end it "should be post_delayed when delayed_post_at and lock_at are in the future" do @@ -292,8 +296,9 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => Time.now + 1.day, :lock_at => Time.now + 3.days) - topic.auto_update_workflow + topic.update_based_on_date topic.workflow_state.should eql 'post_delayed' + topic.locked?.should be_false end it "should be locked when delayed_post_at and lock_at are in the past" do @@ -301,18 +306,20 @@ describe DiscussionTopic do :message => "content here", :delayed_post_at => Time.now - 3.days, :lock_at => Time.now - 1.day) - topic.auto_update_workflow - topic.workflow_state.should eql 'locked' + topic.update_based_on_date + topic.workflow_state.should eql 'active' + topic.locked?.should be_true end it "should not unlock a topic even if the lock date is in the future" do topic = discussion_topic(:title => "title", :message => "content here", :workflow_state => 'locked', + :locked => true, :delayed_post_at => nil, :lock_at => Time.now + 1.day) - topic.auto_update_workflow - topic.workflow_state.should eql 'locked' + topic.update_based_on_date + topic.locked?.should be_true end it "should not mark a topic with post_delayed even if delayed_post_at even is in the future" do @@ -321,8 +328,9 @@ describe DiscussionTopic do :workflow_state => 'active', :delayed_post_at => Time.now + 1.day, :lock_at => nil) - topic.auto_update_workflow + topic.update_based_on_date topic.workflow_state.should eql 'active' + topic.locked?.should be_false end end end @@ -1079,4 +1087,35 @@ describe DiscussionTopic do end + describe "locked flag" do + before :each do + discussion_topic_model + end + + it "should ignore workflow_state if the flag is set" do + @topic.locked = true + @topic.workflow_state = 'active' + @topic.locked?.should be_true + @topic.locked = false + @topic.workflow_state = 'locked' + @topic.locked?.should be_false + end + + it "should fall back to the workflow_state if the flag is nil" do + @topic.locked = nil + @topic.workflow_state = 'active' + @topic.locked?.should be_false + @topic.workflow_state = 'locked' + @topic.locked?.should be_true + end + + it "should fix up a 'locked' workflow_state" do + @topic.workflow_state = 'locked' + @topic.locked = nil + @topic.save! + @topic.unlock! + @topic.workflow_state.should eql 'active' + @topic.locked?.should be_false + end + end end diff --git a/spec/selenium/announcements_spec.rb b/spec/selenium/announcements_spec.rb index 0396fc38270..480d3a3eed1 100644 --- a/spec/selenium/announcements_spec.rb +++ b/spec/selenium/announcements_spec.rb @@ -148,7 +148,7 @@ describe "announcements" do f('#lock').click wait_for_ajax_requests #TODO: check the UI to make sure the topics have a locked symbol - what_to_create.where(:workflow_state => 'locked').count.should == 5 + what_to_create.where(:locked => true).count.should == 5 end it "should search by title" do diff --git a/spec/selenium/discussions_spec.rb b/spec/selenium/discussions_spec.rb index 10fd4a43ff8..12d7e91a2b4 100644 --- a/spec/selenium/discussions_spec.rb +++ b/spec/selenium/discussions_spec.rb @@ -167,7 +167,7 @@ describe "discussions" do expect_new_page_load { f(".discussion_locked_toggler").click } f('.discussion-fyi').text.should == 'This topic is closed for comments' ff('.discussion-reply-action').should be_empty - DiscussionTopic.last.workflow_state.should == 'locked' + DiscussionTopic.last.locked?.should be_true end it "should validate reopening the discussion for comments" do @@ -176,6 +176,7 @@ describe "discussions" do expect_new_page_load { f(".discussion_locked_toggler").click } ff('.discussion-reply-action').should_not be_empty DiscussionTopic.last.workflow_state.should == 'active' + DiscussionTopic.last.locked?.should be_false end it "should escape correctly when posting an attachment" do @@ -652,7 +653,7 @@ describe "discussions" do it "should set as active when removing existing delayed_post_at and lock_at dates" do @topic.delayed_post_at = 10.days.ago @topic.lock_at = 5.days.ago - @topic.workflow_state = 'locked' + @topic.locked = true @topic.save! get "/courses/#{@course.id}/discussion_topics/#{@topic.id}/edit" @@ -668,12 +669,13 @@ describe "discussions" do @topic.delayed_post_at.should be_nil @topic.lock_at.should be_nil @topic.active?.should be_true + @topic.locked?.should be_false end it "should clear the delayed_post_at and lock_at when manually triggering unlock" do @topic.delayed_post_at = 10.days.ago @topic.lock_at = 5.days.ago - @topic.workflow_state = 'locked' + @topic.locked = true @topic.save! get "/courses/#{@course.id}/discussion_topics/#{@topic.id}" @@ -686,6 +688,7 @@ describe "discussions" do @topic.delayed_post_at.should be_nil @topic.lock_at.should be_nil @topic.active?.should be_true + @topic.locked?.should be_false end it "should set workflow to locked when delayed_post_at and lock_at are in past" do @@ -738,7 +741,8 @@ describe "discussions" do it "should set workflow to active when delayed_post_at in past and lock_at in future" do @topic.delayed_post_at = 5.days.from_now @topic.lock_at = 10.days.from_now - @topic.workflow_state = 'locked' + @topic.workflow_state = 'active' + @topic.locked = nil @topic.save! get "/courses/#{@course.id}/discussion_topics/#{@topic.id}/edit" @@ -756,6 +760,7 @@ describe "discussions" do @topic.reload @topic.delayed_post_at.strftime(date_format).should == delayed_post_at.strftime(date_format) @topic.active?.should be_true + @topic.locked?.should be_false end end end diff --git a/spec/selenium/helpers/discussions_common.rb b/spec/selenium/helpers/discussions_common.rb index fd65e6e947d..ad17e2809c2 100644 --- a/spec/selenium/helpers/discussions_common.rb +++ b/spec/selenium/helpers/discussions_common.rb @@ -9,8 +9,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../common') def create_and_go_to_topic(title = 'new topic', discussion_type = 'side_comment', is_locked = false) @topic = @course.discussion_topics.create!(:title => title, :discussion_type => discussion_type) if is_locked - @topic.workflow_state = 'locked' - @topic.save! + @topic.lock @topic.reload end go_to_topic