From 409f78de9ac8dc6c98a576502c857aac6313178f Mon Sep 17 00:00:00 2001 From: Brian Palmer Date: Tue, 27 Mar 2012 19:10:47 -0600 Subject: [PATCH] return discussion_type in topics api response We changed the threaded boolean to a discussion_type string, with currently two types: 'side_comment' and 'threaded' test plan: get the list of topics in the api, verify threaded is returned appropriately. Change-Id: Id5bfb867329d93fe8be6131e2e7c401f70ced290 Reviewed-on: https://gerrit.instructure.com/9647 Reviewed-by: Zach Wily Tested-by: Hudson --- .../discussion_topics_controller.rb | 2 ++ app/models/discussion_topic.rb | 24 +++++++++++++++++-- app/views/shared/_topic.html.erb | 2 +- ...0120328162105_add_discussion_topic_type.rb | 13 ++++++++++ lib/api/v1/discussion_topics.rb | 1 + spec/apis/v1/assignments_api_spec.rb | 1 + spec/apis/v1/discussion_topics_api_spec.rb | 8 +++++++ spec/factories/discussion_topic_factory.rb | 2 +- spec/models/discussion_topic_spec.rb | 17 ++++++++++--- 9 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20120328162105_add_discussion_topic_type.rb diff --git a/app/controllers/discussion_topics_controller.rb b/app/controllers/discussion_topics_controller.rb index 77541d13efe..6946d7962f0 100644 --- a/app/controllers/discussion_topics_controller.rb +++ b/app/controllers/discussion_topics_controller.rb @@ -48,6 +48,7 @@ class DiscussionTopicsController < ApplicationController # @response_field topic_children An array of topic_ids for the group discussions the user is a part of # @response_field user_name The username of the creator # @response_field url The URL to the discussion topic in canvas + # @response_field discussion_type The type of discussion. Values are 'side_comment', for discussions that only allow one level of nested comments, and 'threaded' for fully threaded discussions. # @response_field permissions[attach] If true, the calling user can attach files to this discussion's entries. # # @example_response @@ -68,6 +69,7 @@ class DiscussionTopicsController < ApplicationController # "topic_children":[], # "root_topic_id":null, # "podcast_url":"/feeds/topics/1/enrollment_1XAcepje4u228rt4mi7Z1oFbRpn3RAkTzuXIGOPe.rss", + # "discussion_type":"side_comment", # "attachments":[ # { # "content-type":"unknown/unknown", diff --git a/app/models/discussion_topic.rb b/app/models/discussion_topic.rb index 6df78620046..a2ca7d1c5fa 100644 --- a/app/models/discussion_topic.rb +++ b/app/models/discussion_topic.rb @@ -26,7 +26,13 @@ class DiscussionTopic < ActiveRecord::Base attr_accessible :title, :message, :user, :delayed_post_at, :assignment, :plaintext_message, :podcast_enabled, :podcast_has_student_posts, - :require_initial_post, :threaded + :require_initial_post, :threaded, :discussion_type + + module DiscussionTypes + SIDE_COMMENT = 'side_comment' + THREADED = 'threaded' + TYPES = DiscussionTypes.constants.map { |c| DiscussionTypes.const_get(c) } + end attr_readonly :context_id, :context_type, :user_id @@ -48,6 +54,7 @@ class DiscussionTopic < ActiveRecord::Base belongs_to :user validates_presence_of :context_id validates_presence_of :context_type + validates_presence_of :discussion_type, :in => DiscussionTypes::TYPES validates_length_of :message, :maximum => maximum_long_text_length, :allow_nil => true, :allow_blank => true validates_length_of :title, :maximum => maximum_string_length, :allow_nil => true @@ -65,9 +72,22 @@ class DiscussionTopic < ActiveRecord::Base after_create :create_participant after_create :create_materialized_view + def threaded=(v) + self.discussion_type = Canvas::Plugin.value_to_boolean(v) ? DiscussionTypes::THREADED : DiscussionTypes::SIDE_COMMENT + end + + def threaded? + self.discussion_type == DiscussionTypes::THREADED + end + + def discussion_type + read_attribute(:discussion_type) || DiscussionTypes::SIDE_COMMENT + end + def default_values self.context_code = "#{self.context_type.underscore}_#{self.context_id}" self.title ||= t '#discussion_topic.default_title', "No Title" + self.discussion_type = DiscussionTypes::SIDE_COMMENT if !read_attribute(:discussion_type) @content_changed = self.message_changed? || self.title_changed? if self.assignment_id != self.assignment_id_was @old_assignment_id = self.assignment_id_was @@ -106,7 +126,7 @@ class DiscussionTopic < ActiveRecord::Base topic.title = "#{self.title} - #{group.name}" topic.assignment_id = self.assignment_id topic.user_id = self.user_id - topic.threaded = self.threaded + topic.discussion_type = self.discussion_type topic.save if topic.changed? topic end diff --git a/app/views/shared/_topic.html.erb b/app/views/shared/_topic.html.erb index 9e2e545ad0f..3f979648455 100644 --- a/app/views/shared/_topic.html.erb +++ b/app/views/shared/_topic.html.erb @@ -119,7 +119,7 @@ <%= (topic && topic.podcast_enabled) ? "1" : "0" %> <%= (topic && topic.podcast_has_student_posts) ? "1" : "0" %> <%= (topic && topic.require_initial_post) ? "1" : "0" %> - <%= (topic && topic.threaded) ? "1" : "0" %> + <%= topic.try(:threaded?) ? "1" : "0" %> diff --git a/db/migrate/20120328162105_add_discussion_topic_type.rb b/db/migrate/20120328162105_add_discussion_topic_type.rb new file mode 100644 index 00000000000..30cfb59451d --- /dev/null +++ b/db/migrate/20120328162105_add_discussion_topic_type.rb @@ -0,0 +1,13 @@ +class AddDiscussionTopicType < ActiveRecord::Migration + tag :predeploy + + def self.up + remove_column :discussion_topics, :threaded + add_column :discussion_topics, :discussion_type, :string + end + + def self.down + remove_column :discussion_topics, :discussion_type + add_column :discussion_topics, :threaded, :boolean + end +end diff --git a/lib/api/v1/discussion_topics.rb b/lib/api/v1/discussion_topics.rb index 20516a088c5..52f96e15162 100644 --- a/lib/api/v1/discussion_topics.rb +++ b/lib/api/v1/discussion_topics.rb @@ -45,6 +45,7 @@ module Api::V1::DiscussionTopics :methods => [:user_name, :discussion_subentry_count], }, [:attach] ).tap do |json| json.merge! :message => api_user_content(topic.message, context), + :discussion_type => topic.discussion_type, :podcast_url => url, :read_state => topic.read_state(user), :unread_count => topic.unread_count(user), diff --git a/spec/apis/v1/assignments_api_spec.rb b/spec/apis/v1/assignments_api_spec.rb index 960ad61e89b..7ecd8938dcb 100644 --- a/spec/apis/v1/assignments_api_spec.rb +++ b/spec/apis/v1/assignments_api_spec.rb @@ -243,6 +243,7 @@ describe AssignmentsApiController, :type => :integration do 'url' => "http://www.example.com/courses/#{@course.id}/discussion_topics/#{@topic.id}", 'attachments' => [], 'permissions' => { 'attach' => true }, + 'discussion_type' => 'side_comment', } end diff --git a/spec/apis/v1/discussion_topics_api_spec.rb b/spec/apis/v1/discussion_topics_api_spec.rb index d3a223b7955..c853fc39269 100644 --- a/spec/apis/v1/discussion_topics_api_spec.rb +++ b/spec/apis/v1/discussion_topics_api_spec.rb @@ -95,6 +95,7 @@ describe DiscussionTopicsController, :type => :integration do "size"=>attachment.size, }], "topic_children"=>[sub.id], + "discussion_type" => 'side_comment', "permissions" => { "attach" => true }} end @@ -165,6 +166,7 @@ describe DiscussionTopicsController, :type => :integration do "posted_at"=>gtopic.posted_at.as_json, "root_topic_id"=>nil, "topic_children"=>[], + "discussion_type" => 'side_comment', "permissions" => { "attach" => true }} end @@ -826,6 +828,12 @@ describe DiscussionTopicsController, :type => :integration do end context "in the original API" do + it "should respond with information on the threaded discussion" do + 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]['discussion_type'].should == 'threaded' + end + it "should return nested discussions in a flattened format" do json = api_call(:get, "/api/v1/courses/#{@course.id}/discussion_topics/#{@topic.id}/entries", { :controller => "discussion_topics_api", :action => "entries", :format => "json", :course_id => @course.id.to_s, :topic_id => @topic.id.to_s }) diff --git a/spec/factories/discussion_topic_factory.rb b/spec/factories/discussion_topic_factory.rb index e986d4febaf..83295b027b6 100644 --- a/spec/factories/discussion_topic_factory.rb +++ b/spec/factories/discussion_topic_factory.rb @@ -40,7 +40,7 @@ end def topic_with_nested_replies(opts = {}) course_with_teacher(:active_all => true) student_in_course(:course => @course, :active_all => true) - @topic = @course.discussion_topics.create!(:title => "title", :message => "message", :user => @teacher, :threaded => true) + @topic = @course.discussion_topics.create!(:title => "title", :message => "message", :user => @teacher, :discussion_type => 'threaded') @root1 = @topic.reply_from(:user => @student, :html => "root1") @root2 = @topic.reply_from(:user => @student, :html => "root2") @reply1 = @root1.reply_from(:user => @teacher, :html => "reply1") diff --git a/spec/models/discussion_topic_spec.rb b/spec/models/discussion_topic_spec.rb index cbc7fab1b12..e0a1292cf92 100644 --- a/spec/models/discussion_topic_spec.rb +++ b/spec/models/discussion_topic_spec.rb @@ -25,6 +25,17 @@ describe DiscussionTopic do @course.discussion_topics.first.message.should eql("only this should stay") end + it "should default to side_comment type" do + d = DiscussionTopic.new + d.discussion_type.should == 'side_comment' + + d.threaded = '1' + d.discussion_type.should == 'threaded' + + d.threaded = '' + d.discussion_type.should == 'side_comment' + end + it "should update the assignment it is associated with" do course_model a = @course.assignments.create!(:title => "some assignment", :points_possible => 5) @@ -248,10 +259,10 @@ describe DiscussionTopic do it "should copy appropriate attributes from the parent topic to subtopics on updates to the parent" do topic_for_group_assignment subtopics = @topic.refresh_subtopics - subtopics.each {|st| st.threaded.should be_nil } - @topic.threaded = true + subtopics.each {|st| st.discussion_type.should == 'side_comment' } + @topic.discussion_type = 'threaded' @topic.save - subtopics.each {|st| st.reload.threaded.should == true } + subtopics.each {|st| st.reload.discussion_type.should == 'threaded' } end end