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

discussion topic subscription flag and api 

fixes CNVS-6238

test plan
using the api:
- view a discussion topic as a user that did not author it
- ensure that subscribed: false

- view a discussion topic as the authoring user
- ensure that subscribed: true

- unsubscribe from a discussion topic that the user authored
- ensure that subscribed: false

- subscribe to a discussion topic
- ensure subscribed: true

- subscribe to a topic that requires an initial post with a user
 that hasn't posted yet
- ensure that it is an invalid request

- subscribe to a topic that requires an initial post with a user
 that has posted
- ensure subscribed: true

Change-Id: Ie76a046bb6e9b5253088c371ffdc4dc6ddf08231
Reviewed-on: https://gerrit.instructure.com/21402
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Mark Ericksen <marke@instructure.com>
QA-Review: Cam Theriault <cam@instructure.com>
Product-Review: Joel Hough <joel@instructure.com>
This commit is contained in:
Joel Hough 2013-06-11 13:55:33 -06:00
parent 0d612a2784
commit 65986cfaac
11 changed files with 279 additions and 45 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

95
app/controllers/discussion_topics_api_controller.rb
View File

@ -25,7 +25,7 @@ class DiscussionTopicsApiController < ApplicationController
before_filter :require_context
before_filter :require_topic
before_filter :require_initial_post, :except => [:add_entry, :mark_topic_read, :mark_topic_unread]
before_filter :require_initial_post, :except => [:add_entry, :mark_topic_read, :mark_topic_unread, :unsubscribe_topic]
# @API Get a single topic
#
@ -36,7 +36,6 @@ class DiscussionTopicsApiController < ApplicationController
# curl https://<canvas>/api/v1/courses/<course_id>/discussion_topics/<topic_id> \
# -H 'Authorization: Bearer <token>'
def show
return unless authorized_action(@topic, @current_user, :read)
render :json => discussion_topics_api_json([@topic], @context, @current_user, session).first
end
@ -93,7 +92,6 @@ class DiscussionTopicsApiController < ApplicationController
# ]
# }
def view
return unless authorized_action(@topic, @current_user, :read)
structure, participant_ids, entry_ids, new_entries_structure = @topic.materialized_view(:include_new_entries => params[:include_new_entries] == '1')
if structure
@ -222,10 +220,8 @@ class DiscussionTopicsApiController < ApplicationController
# } ],
# "has_more_replies": false } ]
def entries
if authorized_action(@topic, @current_user, :read)
@entries = Api.paginate(root_entries(@topic).newest_first, self, entry_pagination_url(@topic))
render :json => discussion_entry_api_json(@entries, @context, @current_user, session)
end
@entries = Api.paginate(root_entries(@topic).newest_first, self, entry_pagination_url(@topic))
render :json => discussion_entry_api_json(@entries, @context, @current_user, session)
end
# @API Post a reply
@ -252,7 +248,7 @@ class DiscussionTopicsApiController < ApplicationController
def add_reply
@parent = all_entries(@topic).find(params[:entry_id])
@entry = build_entry(@parent.discussion_subentries)
if authorized_action(@topic, @current_user, :read) && authorized_action(@entry, @current_user, :create)
if authorized_action(@entry, @current_user, :create)
save_entry
end
end
@ -303,10 +299,8 @@ class DiscussionTopicsApiController < ApplicationController
# "created_at": "2011-11-03T21:26:44Z" } ]
def replies
@parent = root_entries(@topic).find(params[:entry_id])
if authorized_action(@topic, @current_user, :read)
@replies = Api.paginate(reply_entries(@parent).newest_first, self, reply_pagination_url(@parent))
render :json => discussion_entry_api_json(@replies, @context, @current_user, session)
end
@replies = Api.paginate(reply_entries(@parent).newest_first, self, reply_pagination_url(@parent))
render :json => discussion_entry_api_json(@replies, @context, @current_user, session)
end
# @API List entries
@ -348,12 +342,10 @@ class DiscussionTopicsApiController < ApplicationController
# { ... entry 3 ... },
# ]
def entry_list
if authorized_action(@topic, @current_user, :read)
ids = Array(params[:ids])
entries = @topic.discussion_entries.find(ids, :order => :id)
@entries = Api.paginate(entries, self, entry_pagination_url(@topic))
render :json => discussion_entry_api_json(@entries, @context, @current_user, session, [])
end
ids = Array(params[:ids])
entries = @topic.discussion_entries.find(ids, :order => :id)
@entries = Api.paginate(entries, self, entry_pagination_url(@topic))
render :json => discussion_entry_api_json(@entries, @context, @current_user, session, [])
end
# @API Mark topic as read
@ -463,6 +455,33 @@ class DiscussionTopicsApiController < ApplicationController
change_entry_read_state("unread")
end
# @API Subscribe to a topic
# Subscribe to a topic to receive notifications about new entries
#
# On success, the response will be 204 No Content with an empty body
#
# @example_request
# curl 'http://<canvas>/api/v1/courses/<course_id>/discussion_topics/<topic_id>/subscribed.json' \
# -X PUT \
# -H "Authorization: Bearer <token>" \
# -H "Content-Length: 0"
def subscribe_topic
render_state_change_result @topic.subscribe(@current_user)
end
# @API Unsubscribe from a topic
# Unsubscribe from a topic to stop receiving notifications about new entries
#
# On success, the response will be 204 No Content with an empty body
#
# @example_request
# curl 'http://<canvas>/api/v1/courses/<course_id>/discussion_topics/<topic_id>/subscribed.json' \
# -X DELETE \
# -H "Authorization: Bearer <token>"
def unsubscribe_topic
render_state_change_result @topic.unsubscribe(@current_user)
end
protected
def require_topic
if params[:topic_id] == "self" && @context.is_a?(CollectionItem)
@ -534,15 +553,7 @@ class DiscussionTopicsApiController < ApplicationController
end
def change_topic_read_state(new_state)
if authorized_action(@topic, @current_user, :read)
topic_participant = @topic.change_read_state(new_state, @current_user)
if topic_participant.present? && (topic_participant == true || topic_participant.errors.blank?)
render :nothing => true, :status => :no_content
else
error_json = topic_participant.errors.to_json rescue {}
render :json => error_json, :status => :bad_request
end
end
render_state_change_result @topic.change_read_state(new_state, @current_user)
end
def get_forced_option()
@ -554,10 +565,8 @@ class DiscussionTopicsApiController < ApplicationController
def change_topic_all_read_state(new_state)
opts = get_forced_option
if authorized_action(@topic, @current_user, :read)
@topic.change_all_read_state(new_state, @current_user, opts)
render :json => {}, :status => :no_content
end
@topic.change_all_read_state(new_state, @current_user, opts)
render :json => {}, :status => :no_content
end
def change_entry_read_state(new_state)
@ -565,13 +574,23 @@ class DiscussionTopicsApiController < ApplicationController
opts = get_forced_option
if authorized_action(@entry, @current_user, :read)
entry_participant = @entry.change_read_state(new_state, @current_user, opts)
if entry_participant.present? && (entry_participant == true || entry_participant.errors.blank?)
render :nothing => true, :status => :no_content
else
error_json = entry_participant.errors.to_json rescue {}
render :json => error_json, :status => :bad_request
end
render_state_change_result @entry.change_read_state(new_state, @current_user, opts)
end
end
# the result of several state change functions are the following:
# nil - no current user
# true - state is already set to the requested state
# participant with errors - something went wrong with the participant
# participant with no errors - the change went through
# this function renders a 204 No Content for a success, or a Bad Request
# for failure with participant errors if there are any
def render_state_change_result(result)
if result == true || result.try(:errors).blank?
render :nothing => true, :status => :no_content
else
error_json = result.try(:errors).try(:to_json) || {}
render :json => error_json, :status => :bad_request
end
end
end

3
app/controllers/discussion_topics_controller.rb
View File

@ -54,6 +54,9 @@
# // The count of unread entries of this topic for the current user.
# "unread_count":0,
#
# // Whether or not the current user is subscribed to this topic.
# "subscribed":true,
#
# // The unique identifier of the assignment if the topic is for grading, otherwise null.
# "assignment_id":null,
#

11
app/models/discussion_entry.rb
View File

@ -175,6 +175,7 @@ class DiscussionEntry < ActiveRecord::Base
save!
update_topic_submission
decrement_unread_counts_for_this_entry
update_topic_subscription
end
def update_discussion
@ -222,6 +223,13 @@ class DiscussionEntry < ActiveRecord::Base
end
end
def update_topic_subscription
discussion_topic.user_ids_who_have_posted_and_admins(true) # pesky memoization
unless discussion_topic.user_can_see_posts?(user)
discussion_topic.unsubscribe(user)
end
end
scope :active, where("discussion_entries.workflow_state<>'deleted'")
scope :deleted, where(:workflow_state => 'deleted')
@ -377,7 +385,8 @@ class DiscussionEntry < ActiveRecord::Base
new_count = self.discussion_topic.unread_count(self.user) - 1
topic_participant = self.discussion_topic.discussion_topic_participants.create(:user => self.user,
:unread_entry_count => new_count,
:workflow_state => "unread")
:workflow_state => "unread",
:subscribed => self.discussion_topic.subscribed?(self.user))
end
end
end

44
app/models/discussion_topic.rb
View File

@ -212,7 +212,7 @@ class DiscussionTopic < ActiveRecord::Base
end
def create_participant
self.discussion_topic_participants.create(:user => self.user, :workflow_state => "read", :unread_entry_count => 0) if self.user
self.discussion_topic_participants.create(:user => self.user, :workflow_state => "read", :unread_entry_count => 0, :subscribed => true) if self.user
end
def update_materialized_view
@ -296,6 +296,31 @@ class DiscussionTopic < ActiveRecord::Base
topic_participant.try(:unread_entry_count) || self.default_unread_count
end
def subscribed?(current_user = nil)
current_user ||= self.current_user
return false unless current_user # default for logged out user
participant = discussion_topic_participants.where(:user_id => current_user.id).first
# if there is no explicit subscription, assume the author is subscribed
# assume non-authors are not subscribed
return current_user == user if participant.try(:subscribed).nil?
participant.subscribed
end
def subscribe(current_user = nil)
change_subscribed_state(true, current_user)
end
def unsubscribe(current_user = nil)
change_subscribed_state(false, current_user)
end
def change_subscribed_state(new_state, current_user = nil)
current_user ||= self.current_user
return unless current_user
return true if subscribed?(current_user) == new_state
update_or_create_participant(:current_user => current_user, :subscribed => new_state)
end
def update_or_create_participant(opts={})
current_user = opts[:current_user] || self.current_user
return nil unless current_user
@ -306,10 +331,12 @@ class DiscussionTopic < ActiveRecord::Base
topic_participant = self.discussion_topic_participants.where(:user_id => current_user).lock.first
topic_participant ||= self.discussion_topic_participants.build(:user => current_user,
:unread_entry_count => self.unread_count(current_user),
:workflow_state => "unread")
:workflow_state => "unread",
:subscribed => current_user == user)
topic_participant.workflow_state = opts[:new_state] if opts[:new_state]
topic_participant.unread_entry_count += opts[:offset] if opts[:offset] && opts[:offset] != 0
topic_participant.unread_entry_count = opts[:new_count] if opts[:new_count]
topic_participant.subscribed = opts[:subscribed] if !opts[:subscribed].nil?
topic_participant.save
end
end
@ -653,9 +680,20 @@ class DiscussionTopic < ActiveRecord::Base
end
end
def participating_users(user_ids)
context.respond_to?(:participating_users) ? context.participating_users(user_ids) : User.find(user_ids)
end
def subscribers
user_ids = discussion_topic_participants.where(:subscribed => true).pluck(:user_id)
user_ids.push(user_id) if subscribed?(user)
user_ids.uniq!
participating_users(user_ids)
end
def posters
user_ids = discussion_entries.map(&:user_id).push(self.user_id).uniq
context.respond_to?(:participating_users) ? context.participating_users(user_ids) : User.find(user_ids)
participating_users(user_ids)
end
def user_name

2
app/models/discussion_topic_participant.rb
View File

@ -20,7 +20,7 @@ class DiscussionTopicParticipant < ActiveRecord::Base
include Workflow
# Be more restrictive if this is ever updatable from user params
attr_accessible :discussion_topic, :user, :workflow_state, :unread_entry_count
attr_accessible :discussion_topic, :user, :workflow_state, :unread_entry_count, :subscribed
belongs_to :discussion_topic
belongs_to :user

2
config/routes.rb
View File

@ -807,6 +807,8 @@ ActionController::Routing::Routes.draw do |map|
topics.delete "#{context.pluralize}/:#{context}_id/discussion_topics/:topic_id/read_all", :action => :mark_all_unread, :path_name => "#{context}_discussion_topic_mark_all_unread"
topics.put "#{context.pluralize}/:#{context}_id/discussion_topics/:topic_id/entries/:entry_id/read", :action => :mark_entry_read, :path_name => "#{context}_discussion_topic_discussion_entry_mark_read"
topics.delete "#{context.pluralize}/:#{context}_id/discussion_topics/:topic_id/entries/:entry_id/read", :action => :mark_entry_unread, :path_name => "#{context}_discussion_topic_discussion_entry_mark_unread"
topics.put "#{context.pluralize}/:#{context}_id/discussion_topics/:topic_id/subscribed", :action => :subscribe_topic, :path_name => "#{context}_discussion_topic_subscribe"
topics.delete "#{context.pluralize}/:#{context}_id/discussion_topics/:topic_id/subscribed", :action => :unsubscribe_topic, :path_name => "#{context}_discussion_topic_unsubscribe"
end
topic_routes(topics, "course")
topic_routes(topics, "group")

11
db/migrate/20130610204053_add_subscribed_to_discussion_topic_participants.rb Normal file
View File

@ -0,0 +1,11 @@
class AddSubscribedToDiscussionTopicParticipants < ActiveRecord::Migration
tag :predeploy
def self.up
add_column :discussion_topic_participants, :subscribed, :boolean
end
def self.down
remove_column :discussion_topic_participants, :subscribed
end
end

1
lib/api/v1/discussion_topics.rb
View File

@ -57,6 +57,7 @@ module Api::V1::DiscussionTopics
:position => topic.position,
:read_state => topic.read_state(user),
:unread_count => topic.unread_count(user),
:subscribed => topic.subscribed?(user),
:topic_children => children,
:attachments => attachments,
:locked => topic.locked?,

1
spec/apis/v1/assignments_api_spec.rb
View File

@ -1015,6 +1015,7 @@ describe AssignmentsApiController, :type => :integration do
'podcast_has_student_posts' => nil,
'read_state' => 'unread',
'unread_count' => 0,
'subscribed' => @topic.subscribed?(@user),
'url' =>
"http://www.example.com/courses/#{@course.id}/discussion_topics/#{@topic.id}",
'html_url' =>

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

@ -209,6 +209,7 @@ describe DiscussionTopicsController, :type => :integration do
{"read_state"=>"read",
"unread_count"=>0,
"podcast_url"=>"/feeds/topics/#{@topic.id}/enrollment_randomness.rss",
"subscribed"=>@topic.subscribed?(@user),
"require_initial_post"=>nil,
"title"=>"Topic 1",
"discussion_subentry_count"=>0,
@ -258,7 +259,7 @@ describe DiscussionTopicsController, :type => :integration do
json.size.should == 2
# get rid of random characters in podcast url
json.last["podcast_url"].gsub!(/_[^.]*/, '_randomness')
json.last.should == @response_json
json.last.should == @response_json.merge("subscribed" => @sub.subscribed?(@user))
end
it "should order topics by descending position by default" do
@ -350,7 +351,7 @@ describe DiscussionTopicsController, :type => :integration do
# get rid of random characters in podcast url
json["podcast_url"].gsub!(/_[^.]*/, '_randomness')
json.should == @response_json
json.should == @response_json.merge("subscribed" => @topic.subscribed?(@user))
end
end
@ -677,6 +678,7 @@ describe DiscussionTopicsController, :type => :integration do
expected = {
"read_state"=>"read",
"unread_count"=>0,
"subscribed"=>true,
"podcast_url"=>nil,
"podcast_has_student_posts"=>nil,
"require_initial_post"=>nil,
@ -1471,6 +1473,72 @@ describe DiscussionTopicsController, :type => :integration do
end
end
context "subscribing" do
before do
student_in_course(:active_all => true)
@topic1 = create_topic(@course, :user => @student)
@topic2 = create_topic(@course, :user => @teacher, :require_initial_post => true)
end
def call_subscribe(topic, user, course=@course)
@user = user
raw_api_call(:put, "/api/v1/courses/#{course.id}/discussion_topics/#{topic.id}/subscribed",
{ :controller => "discussion_topics_api", :action => "subscribe_topic", :format => "json", :course_id => course.id.to_s, :topic_id => topic.id.to_s})
response.status.to_i
end
def call_unsubscribe(topic, user, course=@course)
@user = user
raw_api_call(:delete, "/api/v1/courses/#{course.id}/discussion_topics/#{topic.id}/subscribed",
{ :controller => "discussion_topics_api", :action => "unsubscribe_topic", :format => "json", :course_id => course.id.to_s, :topic_id => topic.id.to_s})
response.status.to_i
end
it "should allow subscription" do
call_subscribe(@topic1, @teacher).should == 204
@topic1.subscribed?(@teacher).should be_true
end
it "should allow unsubscription" do
call_unsubscribe(@topic2, @teacher).should == 204
@topic2.subscribed?(@teacher).should be_false
end
it "should be idempotent" do
call_unsubscribe(@topic1, @teacher).should == 204
call_subscribe(@topic1, @student).should == 204
end
context "when initial_post_required" do
it "should allow subscription with an initial post" do
@user = @student
create_reply(@topic2, :message => 'first post!')
call_subscribe(@topic2, @student).should == 204
@topic2.subscribed?(@student).should be_true
end
it "should not allow subscription without an initial post" do
call_subscribe(@topic2, @student).should == 403
end
it "should allow unsubscription even without an initial post" do
@topic2.subscribe(@student)
@topic2.subscribed?(@student).should be_true
call_unsubscribe(@topic2, @student).should == 204
@topic2.subscribed?(@student).should be_false
end
it "should unsubscribe a user if all their posts get deleted" do
@user = @student
@entry = create_reply(@topic2, :message => 'first post!')
call_subscribe(@topic2, @student).should == 204
@topic2.subscribed?(@student).should be_true
@entry.destroy
@topic2.subscribed?(@student).should be_false
end
end
end
describe "threaded discussions" do
before do
student_in_course(:active_all => true)

82
spec/models/discussion_topic_spec.rb
View File

@ -600,6 +600,41 @@ describe DiscussionTopic do
end
context "subscribers" do
before :each do
course_with_student(:active_all => true)
@context = @course
discussion_topic_model(:user => @teacher)
end
it "should automatically include the author" do
@topic.subscribers.should include(@teacher)
end
it "should not include the author if they unsubscribe" do
@topic.unsubscribe(@teacher)
@topic.subscribers.should_not include(@teacher)
end
it "should not automatically include posters" do
@topic.reply_from(:user => @student, :text => "entry")
@topic.subscribers.should_not include(@student)
end
it "should include users who subscribe" do
@topic.subscribe(@student)
@topic.subscribers.should include(@student)
end
it "should not include anyone no longer in the course" do
@topic.subscribe(@student)
@topic2 = @course.discussion_topics.create!(:title => "student topic", :message => "I'm outta here", :user => @student)
@student.enrollments.first.destroy
@topic.subscribers.should_not include(@student)
@topic2.subscribers.should_not include(@student)
end
end
context "posters" do
before :each do
@teacher = course_with_teacher(:active_all => true).user
@ -896,6 +931,53 @@ describe DiscussionTopic do
end
end
context "subscribing" do
before :each do
course_with_student(:active_all => true)
@context = @course
discussion_topic_model(:user => @teacher)
end
it "should allow subscription" do
@topic.subscribed?(@student).should be_false
@topic.subscribe(@student)
@topic.subscribed?(@student).should be_true
end
it "should allow unsubscription" do
@topic.subscribed?(@teacher).should be_true
@topic.unsubscribe(@teacher)
@topic.subscribed?(@teacher).should be_false
end
it "should be idempotent" do
@topic.subscribed?(@student).should be_false
@topic.unsubscribe(@student)
@topic.subscribed?(@student).should be_false
end
it "should assume the author is subscribed" do
@topic.subscribed?(@teacher).should be_true
end
it "should not assume posters are subscribed" do
@topic.reply_from(:user => @student, :text => 'first post!')
@topic.subscribed?(@student).should be_false
end
context "when initial_post_required" do
it "should unsubscribe a user when all of their posts are deleted" do
@topic.require_initial_post = true
@topic.save!
@entry = @topic.reply_from(:user => @student, :text => 'first post!')
@topic.subscribe(@student)
@topic.subscribed?(@student).should be_true
@entry.destroy
@topic.subscribed?(@student).should be_false
end
end
end
context "materialized view" do
before do
topic_with_nested_replies