From 53920a96ce575e494bb14da5a3f059998f700a53 Mon Sep 17 00:00:00 2001 From: Josh Simpson Date: Fri, 6 Jun 2014 16:24:54 -0600 Subject: [PATCH] add explicit poll choice ordering and poll_session deletion fixes CNVS-13521,CNVS-13489 This commit adds 3 things. One is a fix for deleting a poll_session when there are poll_submissions created already for the poll_session. The second is the addition of a new attribute on poll_choices called 'position'. The 'position' attribute specifies the order a poll choice should be returned in. The third is the visibility of poll sessions to students: as long as a student is enrolled in the course (and course section, if it exists on the poll session) associated with the poll session, they can access it, whether or not it's open or closed. Test plans 1. Deleting a poll session As a teacher - Create a poll and related poll session - Open the poll session for submissions As a student - Create a submission for the newly created poll session As a teacher - You should be able to delete the poll session 2. Position ordering attribute As a teacher - Create a poll - Create poll choices, passing along the 'position' attribute to specify ordering (try to mix these up, e.g. create a poll choice with a position of 2, then a poll choice with an position of 4, then a poll choice with an position of 3, then a poll choice with an position of 1) - Retrieve the poll choices for the poll with the poll choices index endpoint - The poll should be returned in the order they were specified using the 'position' attribute 3. Poll session visibility to students As a teacher - Create a poll - Create an associated poll session with a valid course As a student enrolled in the given course - You should be able to view the poll session whether or not it's open or closed. As a student not enrolled in the given course - You should not be able to view the poll session, whether or not it's open or closed. Change-Id: Ifabd64540d843c83f3a22fb6668c4fcac8f728b0 Reviewed-on: https://gerrit.instructure.com/36087 Reviewed-by: Derek DeVries QA-Review: Trevor deHaan Product-Review: Bryan Madsen Tested-by: Jenkins Product-Review: Josh Simpson --- .../polling/poll_choices_controller.rb | 11 ++++++++ .../polling/poll_sessions_controller.rb | 2 +- app/models/polling/poll.rb | 2 +- app/models/polling/poll_choice.rb | 4 ++- app/models/polling/poll_session.rb | 18 +++++++++---- .../polling/poll_choice_serializer.rb | 4 +-- ...920_add_position_column_to_poll_choices.rb | 11 ++++++++ spec/apis/v1/polling/poll_choices_api_spec.rb | 25 ++++++++++++++----- .../apis/v1/polling/poll_sessions_api_spec.rb | 14 +++++++++++ spec/apis/v1/polling/polls_api_spec.rb | 6 ++--- 10 files changed, 77 insertions(+), 20 deletions(-) create mode 100644 db/migrate/20140606220920_add_position_column_to_poll_choices.rb diff --git a/app/controllers/polling/poll_choices_controller.rb b/app/controllers/polling/poll_choices_controller.rb index 969e23c09d8..d3cc10c47d7 100644 --- a/app/controllers/polling/poll_choices_controller.rb +++ b/app/controllers/polling/poll_choices_controller.rb @@ -44,6 +44,11 @@ module Polling # "description": "The text of the poll choice.", # "type": "string", # "example": "Choice A" + # }, + # "position": { + # "description": "The order of the poll choice in relation to it's sibling poll choices.", + # "type": "integer", + # "example": 1 # } # } # } @@ -101,6 +106,9 @@ module Polling # @argument poll_choices[][is_correct] [Optional, Boolean] # Whether this poll choice is considered correct or not. Defaults to false. # + # @argument poll_choices[][position] [Optional, Integer] + # The order this poll choice should be returned in the context it's sibling poll choices. + # # @example_response # { # "poll_choices": [PollChoice] @@ -131,6 +139,9 @@ module Polling # @argument poll_choices[][is_correct] [Optional, Boolean] # Whether this poll choice is considered correct or not. Defaults to false. # + # @argument poll_choices[][position] [Optional, Integer] + # The order this poll choice should be returned in the context it's sibling poll choices. + # # @example_response # { # "poll_choices": [PollChoice] diff --git a/app/controllers/polling/poll_sessions_controller.rb b/app/controllers/polling/poll_sessions_controller.rb index 091ffe9810d..e5a89153150 100644 --- a/app/controllers/polling/poll_sessions_controller.rb +++ b/app/controllers/polling/poll_sessions_controller.rb @@ -23,7 +23,7 @@ module Polling # @model PollSession # { # "id": "PollSession", - # "required": ["id", "poll_id", "course_id", "course_section_id"], + # "required": ["id", "poll_id", "course_id"], # "properties": { # "id": { # "description": "The unique identifier for the poll session.", diff --git a/app/models/polling/poll.rb b/app/models/polling/poll.rb index 4d23be64acc..98a785ea8da 100644 --- a/app/models/polling/poll.rb +++ b/app/models/polling/poll.rb @@ -21,7 +21,7 @@ module Polling attr_accessible :user, :question, :description belongs_to :user - has_many :poll_choices, class_name: 'Polling::PollChoice', dependent: :destroy + has_many :poll_choices, class_name: 'Polling::PollChoice', dependent: :destroy, order: :position has_many :poll_submissions, class_name: 'Polling::PollSubmission', dependent: :destroy has_many :poll_sessions, class_name: 'Polling::PollSession', dependent: :destroy diff --git a/app/models/polling/poll_choice.rb b/app/models/polling/poll_choice.rb index b39e19a8d37..669fdfecb69 100644 --- a/app/models/polling/poll_choice.rb +++ b/app/models/polling/poll_choice.rb @@ -18,7 +18,9 @@ module Polling class PollChoice < ActiveRecord::Base - attr_accessible :text, :poll, :is_correct + set_table_name 'polling_poll_choices' + + attr_accessible :text, :poll, :is_correct, :position belongs_to :poll, class_name: 'Polling::Poll' has_many :poll_submissions, class_name: 'Polling::PollSubmission', dependent: :destroy diff --git a/app/models/polling/poll_session.rb b/app/models/polling/poll_session.rb index 4c09b372fa3..870bb933c3d 100644 --- a/app/models/polling/poll_session.rb +++ b/app/models/polling/poll_session.rb @@ -23,7 +23,7 @@ module Polling belongs_to :course belongs_to :course_section belongs_to :poll, class_name: 'Polling::Poll' - has_many :poll_submissions, class_name: 'Polling::PollSubmission' + has_many :poll_submissions, class_name: 'Polling::PollSubmission', dependent: :destroy validates_presence_of :poll, :course validate :section_belongs_to_course @@ -34,11 +34,14 @@ module Polling can :read and can :create and can :delete and can :publish given do |user, session| - self.cached_context_grants_right?(user, session, :read) && - self.is_published? && - (self.course_section ? self.course_section.grants_right?(user, session, :read) : true) + self.visible_to?(user, session) end - can :read and can :submit + can :read + + given do |user, session| + self.visible_to?(user, session) && self.is_published? + end + can :submit end def self.available_for(user) @@ -65,6 +68,11 @@ module Polling save! end + def visible_to?(user, session) + self.cached_context_grants_right?(user, session, :read) && + (self.course_section ? self.course_section.grants_right?(user, session, :read) : true) + end + private def section_belongs_to_course if self.course && self.course_section diff --git a/app/serializers/polling/poll_choice_serializer.rb b/app/serializers/polling/poll_choice_serializer.rb index 38323831070..ed43b4a460e 100644 --- a/app/serializers/polling/poll_choice_serializer.rb +++ b/app/serializers/polling/poll_choice_serializer.rb @@ -2,7 +2,7 @@ module Polling class PollChoiceSerializer < Canvas::APISerializer root :poll_choice - attributes :id, :text, :is_correct + attributes :id, :text, :is_correct, :position has_one :poll, embed: :id @@ -31,7 +31,7 @@ module Polling end def student_keys - keys = [:id, :text] + keys = [:id, :text, :position] keys << :is_correct if poll.closed_and_viewable_for?(current_user) keys end diff --git a/db/migrate/20140606220920_add_position_column_to_poll_choices.rb b/db/migrate/20140606220920_add_position_column_to_poll_choices.rb new file mode 100644 index 00000000000..3cb4f44f528 --- /dev/null +++ b/db/migrate/20140606220920_add_position_column_to_poll_choices.rb @@ -0,0 +1,11 @@ +class AddPositionColumnToPollChoices < ActiveRecord::Migration + tag :predeploy + + def self.up + add_column :polling_poll_choices, :position, :integer + end + + def self.down + remove_column :polling_poll_choices, :position + end +end diff --git a/spec/apis/v1/polling/poll_choices_api_spec.rb b/spec/apis/v1/polling/poll_choices_api_spec.rb index 86142f885d2..20cb81d4555 100644 --- a/spec/apis/v1/polling/poll_choices_api_spec.rb +++ b/spec/apis/v1/polling/poll_choices_api_spec.rb @@ -26,9 +26,11 @@ describe Polling::PollChoicesController, type: :request do describe 'GET index' do before(:each) do @poll = @teacher.polls.create!(question: "Example Poll") - 5.times do |n| - @poll.poll_choices.create!(text: "Poll Choice #{n+1}", is_correct: false) - end + @poll.poll_choices.create!(text: "Poll Choice 1", is_correct:false, position: 1) + @poll.poll_choices.create!(text: "Poll Choice 3", is_correct:false, position: 3) + @poll.poll_choices.create!(text: "Poll Choice 4", is_correct:false, position: 4) + @poll.poll_choices.create!(text: "Poll Choice 2", is_correct:false, position: 2) + @poll.poll_choices.create!(text: "Poll Choice 5", is_correct:false, position: 5) end def get_index(raw = false, data = {}) @@ -46,10 +48,20 @@ describe Polling::PollChoicesController, type: :request do poll_choices_json.size.should == 5 poll_choices_json.each_with_index do |pc, i| - pc['text'].should == "Poll Choice #{5-i}" + pc['text'].should == "Poll Choice #{i+1}" end end + it "returns the poll choices in the correct order" do + json = get_index + poll_choices_json = json['poll_choices'] + + poll_choices_json.each_with_index do |pc, i| + pc['position'].should == i+1 + end + end + + context "as a student" do before(:each) do student_in_course(:active_all => true, :course => @course) @@ -159,12 +171,13 @@ describe Polling::PollChoicesController, type: :request do context "as a teacher" do it "creates a poll choice successfully" do - post_create(text: 'Poll Choice 1', is_correct: false) + post_create(text: 'Poll Choice 1', is_correct: false, position: 1) @poll.poll_choices.first.text.should == 'Poll Choice 1' + @poll.poll_choices.first.position.should == 1 end it "sets is_correct to false if is_correct is provided but blank" do - post_create(text: 'is correct poll choice', is_correct: '') + post_create(text: 'is correct poll choice', is_correct: '', position: 1) @poll.poll_choices.first.text.should == 'is correct poll choice' @poll.poll_choices.first.is_correct.should be_false end diff --git a/spec/apis/v1/polling/poll_sessions_api_spec.rb b/spec/apis/v1/polling/poll_sessions_api_spec.rb index e1550e9867e..f2e19e12453 100644 --- a/spec/apis/v1/polling/poll_sessions_api_spec.rb +++ b/spec/apis/v1/polling/poll_sessions_api_spec.rb @@ -136,6 +136,20 @@ describe Polling::PollSessionsController, type: :request do end context "as a student" do + it "doesn't display if the student isn't enrolled in the associated course or course section" do + section = @course.course_sections.create!(name: 'Some Course Section') + @poll_session.course_section = section + @poll_session.save + + student_in_course(active_all: true, course: @course) + + get_show(true) + + response.code.should == '401' + @poll_session.reload + @poll_session.poll_submissions.size.should be_zero + end + it "returns has_submitted as true if the student has made a submission" do choice = @poll.poll_choices.create!(text: 'Choice A', is_correct: true) submission = create_submission(choice) diff --git a/spec/apis/v1/polling/polls_api_spec.rb b/spec/apis/v1/polling/polls_api_spec.rb index 5d4b5d40557..7b9d2ec59a8 100644 --- a/spec/apis/v1/polling/polls_api_spec.rb +++ b/spec/apis/v1/polling/polls_api_spec.rb @@ -66,9 +66,6 @@ describe Polling::PollsController, type: :request do describe 'GET show' do before(:each) do @poll = @teacher.polls.create!(question: 'An Example Poll') - 5.times do |n| - @poll.poll_choices.create!(text: "Poll Choice #{n+1}", is_correct: false) - end end def get_show(raw = false, data = {}) @@ -114,7 +111,8 @@ describe Polling::PollsController, type: :request do it "shouldn't return the id of the user that created the poll" do student_in_course(:active_all => true, :course => @course) - @poll.poll_sessions.create!(course: @course) + session = @poll.poll_sessions.create!(course: @course) + session.publish! json = get_show poll_json = json['polls'].first