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

disable non-members' subscription controls for group discussions 

fixes CNVS-6853

test plan
- create a graded group discussion
- ensure that a teacher cannot subscribe to the topic, either from
  the discussion show page, the course discussion index, or the
  group discussion index
- ensure that a student in one of the groups in the discussion's
  group set CAN subscribe from all those places
- ensure that a student NOT in one of the groups can NOT subscribe
- ensure that the tooltips over the subscription icons give reasons
  for not allowing subscriptions
- ensure that the subscribe button can not be seen on the show page
  when the user can't subscribe

Change-Id: I9ee79f8a13f48de3fa0d109580e6280d7681ea41
Reviewed-on: https://gerrit.instructure.com/22495
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Zach Pendleton <zachp@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-18 16:57:10 -06:00
parent 95662eb9a4
commit c36b85593f
11 changed files with 131 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
app/coffeescripts/models/DiscussionTopic.coffee
View File

@ -19,6 +19,7 @@ define [
is_announcement: false
subscribed: false
user_can_see_posts: true
subscription_hold: null
dateAttributes: [
'last_reply_at'

2
app/coffeescripts/views/DiscussionTopic/TopicView.coffee
View File

@ -123,7 +123,7 @@ define [
@reply.on 'edit', => @$addRootReply?.hide()
@reply.on 'hide', => @$addRootReply?.show()
@reply.on 'save', (entry) =>
@setSubscribed(true)
@setSubscribed(true) unless @topic.subscription_hold
@trigger 'addReply', entry
@model.set 'notification', ''
@reply.edit()

47
app/coffeescripts/views/ToggleableSubscriptionIconView.coffee
View File

@ -16,7 +16,9 @@ define [
subscribed: I18n.t('subscribed', 'Subscribed')
unsubscribe: I18n.t('unsubscribe', 'Unsubscribe from this topic')
unsubscribed: I18n.t('unsubscribed', 'Unsubscribed')
initial_post_required_to_subscribe: I18n.t('initial_post_required_to_subscribe', 'You must post a reply before subscribing')
initial_post_required: I18n.t('initial_post_required_to_subscribe', 'You must post a reply before subscribing')
not_in_group_set: I18n.t('cant_subscribe_not_in_group_set', 'You must be in an associated group to subscribe')
not_in_group: I18n.t('cant_subscribe_not_in_group', 'You must be in this group to subscribe')
tooltipOptions:
items: '*'
@ -28,6 +30,7 @@ define [
initialize: ->
@model.on('change:subscribed', @render)
@model.on('change:user_can_see_posts', @render)
@model.on('change:subscription_hold', @render)
super
hover: ({type}) ->
@ -45,29 +48,28 @@ define [
@displayStateDuringHover = true
@render()
subscribed: -> @model.get('subscribed')
canSubscribe: -> @model.get('user_can_see_posts')
render: ->
super
subscribed: -> @model.get('subscribed') && @canSubscribe()
# hovering: h, subscribed: s, requires initial post: r, just changed: j
canSubscribe: -> !@subscriptionHold()
subscriptionHold: -> @model.get('subscription_hold')
classAndTextForTooltip: ->
# hovering: v, subscribed: s, subscription hold: h, display state during hover: d
# true: 1, false: 0, don't care: x
# j implies h
#
# hsrj | class | tooltip
#
# vshd | class | tooltip
# -----+-----------------------+---------
# 0xx1 | <should never happen> | <should never happen>
# 00xx | icon-discussion | x
# 01xx | icon-discussion-check | x
# 1000 | icon-discussion-check | Subscribe
# 1010 | icon-discussion-x | Initial post required
# 1010 | icon-discussion-x | <subscription hold message>
# 11x0 | icon-discussion-x | Unsubscribe
# 10x1 | icon-discussion | Unsubscribed
# 11x1 | icon-discussion-check | Subscribed
if @hovering
[newClass, tooltipText] = if @subscribed()
if @subscribed()
if @displayStateDuringHover
['icon-discussion-check', @messages['subscribed']]
else
@ -78,13 +80,20 @@ define [
else if @canSubscribe()
['icon-discussion-check', @messages['subscribe']]
else
['icon-discussion-x', @messages['initial_post_required_to_subscribe']]
@$el.tooltip(@tooltipOptions)
@$el.tooltip('close')
@$el.tooltip('option', content: -> tooltipText)
@$el.tooltip('open')
['icon-discussion-x', @messages[@subscriptionHold()]]
else
newClass = if @subscribed() then 'icon-discussion-check' else 'icon-discussion'
[(if @subscribed() then 'icon-discussion-check' else 'icon-discussion'), '']
resetTooltipText: (tooltipText) ->
@$el.tooltip(@tooltipOptions)
# cycle the tooltip to recenter, also blinks if the text doesn't change which is good here
@$el.tooltip('close')
@$el.tooltip('option', content: -> tooltipText)
@$el.tooltip('open')
afterRender: ->
[newClass, tooltipText] = @classAndTextForTooltip()
@resetTooltipText(tooltipText)
@$el.removeClass('icon-discussion icon-discussion-x icon-discussion-check')
@$el.addClass(newClass)
this

10
app/controllers/discussion_topics_controller.rb
View File

@ -60,6 +60,14 @@
# // Whether or not the current user is subscribed to this topic.
# "subscribed":true,
#
# // (Optional) Why the user cannot subscribe to this topic. Only one reason
# // will be returned even if multiple apply. Can be one of:
# // 'initial_post_required': The user must post a reply first
# // 'not_in_group_set': The user is not in the group set for this graded group discussion
# // 'not_in_group': The user is not in this topic's group
# // 'topic_is_announcement': This topic is an announcement
# "subscription_hold":"not_in_group_set",
#
# // The unique identifier of the assignment if the topic is for grading, otherwise null.
# "assignment_id":null,
#
@ -325,7 +333,7 @@ class DiscussionTopicsController < ApplicationController
:MARK_ALL_READ_URL => named_context_url(@context, :api_v1_context_discussion_topic_mark_all_read_url, @topic),
:MARK_ALL_UNREAD_URL => named_context_url(@context, :api_v1_context_discussion_topic_mark_all_unread_url, @topic),
:MANUAL_MARK_AS_READ => @current_user.try(:manual_mark_as_read?),
:CAN_SUBSCRIBE => !@initial_post_required && !@topic.is_a?(Announcement), # can subscribe when no initial post required for user and don't show for announcements
:CAN_SUBSCRIBE => !@topic.subscription_hold(@current_user, @context_enrollment, session),
:CURRENT_USER => user_display_json(@current_user),
:INITIAL_POST_REQUIRED => @initial_post_required,
:THREADED => @topic.threaded?

3
app/models/announcement.rb
View File

@ -83,4 +83,7 @@ class Announcement < DiscussionTopic
[]
end
def subscription_hold(user, context_enrollment, session)
:topic_is_announcement
end
end

2
app/models/discussion_entry.rb
View File

@ -392,7 +392,7 @@ class DiscussionEntry < ActiveRecord::Base
:workflow_state => "unread",
:subscribed => self.discussion_topic.subscribed?(self.user))
end
self.discussion_topic.subscribe(self.user)
self.discussion_topic.subscribe(self.user) unless self.discussion_topic.subscription_hold(self.user, nil, nil)
end
end
end

28
app/models/discussion_topic.rb
View File

@ -296,6 +296,22 @@ class DiscussionTopic < ActiveRecord::Base
topic_participant.try(:unread_entry_count) || self.default_unread_count
end
# Cases where you CAN'T subscribe:
# - initial post is required and you haven't made one
# - it's an announcement
# - this is a root level graded group discussion and you aren't in any of the groups
# - this is group level discussion and you aren't in the group
def subscription_hold(user, context_enrollment, session)
case
when initial_post_required?(user, context_enrollment, session)
:initial_post_required
when root_topic? && !child_topic_for(user)
:not_in_group_set
when context.is_a?(Group) && !context.has_member?(user)
:not_in_group
end
end
def subscribed?(current_user = nil)
current_user ||= self.current_user
return false unless current_user # default for logged out user
@ -315,7 +331,7 @@ class DiscussionTopic < ActiveRecord::Base
participant.subscribed
end
else
current_user == user
current_user == user && !why_cant_user_subscribe?(current_user, nil, nil)
end
end
@ -339,10 +355,14 @@ class DiscussionTopic < ActiveRecord::Base
end
end
def change_child_topic_subscribed_state(new_state, current_user)
group_ids = current_user.group_memberships.active.pluck(:group_id) &
def child_topic_for(user)
group_ids = user.group_memberships.active.pluck(:group_id) &
context.groups.active.pluck(:id)
topic = child_topics.where(context_id: group_ids, context_type: 'Group').first
child_topics.where(context_id: group_ids, context_type: 'Group').first
end
def change_child_topic_subscribed_state(new_state, current_user)
topic = child_topic_for(current_user)
topic.update_or_create_participant(current_user: current_user, subscribed: new_state)
end
protected :change_child_topic_subscribed_state

2
app/stylesheets/discussions_list.scss
View File

@ -217,7 +217,7 @@
}
.icon-discussion-x {
color: #ee5f5b;
color: $btnDangerBackground;
}
}

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

@ -69,6 +69,8 @@ module Api::V1::DiscussionTopics
:context_discussion_topic_url,
topic,
:include_host => true)
subscription_hold = topic.subscription_hold(user, @context_enrollment, session)
json[:subscription_hold] = subscription_hold if subscription_hold
locked_json(json, topic, user, session)
json[:url] = json[:html_url] # deprecated
if include_assignment && topic.assignment

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

@ -772,6 +772,7 @@ describe DiscussionTopicsController, :type => :integration do
it "should work with groups" do
group_category = @course.group_categories.create(:name => 'watup')
group = group_category.groups.create!(:name => "group1", :context => @course)
group.add_user(@user)
attachment = create_attachment(group)
gtopic = create_topic(group, :title => "Group Topic 1", :message => "<p>content here</p>", :attachment => attachment)
@ -1590,14 +1591,14 @@ describe DiscussionTopicsController, :type => :integration do
{ :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
@ -1620,7 +1621,7 @@ describe DiscussionTopicsController, :type => :integration do
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
@ -1642,7 +1643,35 @@ describe DiscussionTopicsController, :type => :integration do
end
end
end
context "subscription holds" do
it "should hold when an initial post is required" do
@topic = create_topic(@course, :require_initial_post => true)
student_in_course(:active_all => true)
json = api_call(:get, "/api/v1/courses/#{@course.id}/discussion_topics",
{ :controller => "discussion_topics", :action => "index", :format => "json", :course_id => @course.id.to_s })
json[0]['subscription_hold'].should eql('initial_post_required')
end
it "should hold when the user isn't in a group set" do
teacher_in_course(:active_all => true)
group_discussion_assignment
json = api_call(:get, "/api/v1/courses/#{@course.id}/discussion_topics",
{ :controller => "discussion_topics", :action => "index", :format => "json", :course_id => @course.id.to_s })
json[0]['subscription_hold'].should eql('not_in_group_set')
end
it "should hold when the user isn't in a group" do
teacher_in_course(:active_all => true)
group_discussion_assignment
child = @topic.child_topics.first
group = child.context
json = api_call(:get, "/api/v1/groups/#{group.id}/discussion_topics",
{ :controller => "discussion_topics", :action => "index", :format => "json", :group_id => group.id.to_s })
json[0]['subscription_hold'].should eql('not_in_group')
end
end
describe "threaded discussions" do
before do
student_in_course(:active_all => true)

32
spec/models/discussion_topic_spec.rb
View File

@ -674,7 +674,7 @@ describe DiscussionTopic do
@topic2.subscribers.should_not include(@student)
end
end
context "posters" do
before :each do
@teacher = course_with_teacher(:active_all => true).user
@ -977,7 +977,7 @@ describe DiscussionTopic do
@context = @course
discussion_topic_model(:user => @teacher)
end
it "should allow subscription" do
@topic.subscribed?(@student).should be_false
@topic.subscribe(@student)
@ -989,13 +989,13 @@ describe DiscussionTopic do
@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
@ -1017,6 +1017,30 @@ describe DiscussionTopic do
end
end
context "subscription holds" do
before :each do
course_with_student(:active_all => true)
@context = @course
end
it "should hold when requiring an initial post" do
discussion_topic_model(:user => @teacher, :require_initial_post => true)
@topic.subscription_hold(@student, nil, nil).should eql(:initial_post_required)
end
it "should hold when the user is not in a group set" do
# i.e. when you check holds on a root topic and no child topics are for groups
# the user is in
group_discussion_assignment
@topic.subscription_hold(@student, nil, nil).should eql(:not_in_group_set)
end
it "should hold when the user is not in a group" do
group_discussion_assignment
@topic.child_topics.first.subscription_hold(@student, nil, nil).should eql(:not_in_group)
end
end
context "a group topic subscription" do
before(:each) do