ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

do discussion topic locking with a flag 

fixes CNVS-6592

test plan
- ensure discussion topic locking works as before
- delete and undelete a topic
- ensure that undeleted topic remembers whether it was locked

Change-Id: I389dc68ac181283b77fd23e5ca9b71b8bf6a0b55
Reviewed-on: https://gerrit.instructure.com/21923
Reviewed-by: Mark Ericksen <marke@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Cam Theriault <cam@instructure.com>
Product-Review: Joel Hough <joel@instructure.com>
This commit is contained in:
Joel Hough 2013-07-01 16:27:46 -06:00
parent afa7c06be8
commit 7a6ccef2b9
10 changed files with 126 additions and 69 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/courses_controller.rb
View File

@ -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

4
app/controllers/discussion_topics_controller.rb
View File

@ -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)

2
app/models/announcement.rb
View File

@ -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

80
app/models/discussion_topic.rb
View File

@ -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|

11
db/migrate/20130702104734_add_locked_flag_to_discussion_topics.rb Normal file
View File

@ -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

11
spec/apis/v1/discussion_topics_api_spec.rb
View File

@ -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

67
spec/models/discussion_topic_spec.rb
View File

@ -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

2
spec/selenium/announcements_spec.rb
View File

@ -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

13
spec/selenium/discussions_spec.rb
View File

@ -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

3
spec/selenium/helpers/discussions_common.rb
View File

@ -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