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

API tweaks to support querying upcoming enrollments/courses 

fixes CAT-147

add support for a user_id param when querying course/section enrollments

ensure course and enrollment endpoints allow users to see their own
*active* enrollments in unpublished course (i.e. self-enrollments), even
if they are not admins.

slightly tighten up User#current_and_future_enrollments (only return non-
admin enrollments in unpublished courses if they are active). this method
is only used currently around self-enrollment functionality, and that sets
the enrollment state to :active, so there should be no changes in
functionality

test plan:
1. set up self-enrollment in an unpublished course
2. self-enroll as a student (via link from course settings page)
3. it should work (though you can't get into the course yet)
4. as the student, hit /api/v1/courses
   1. with no params, you shouldn't see the course
   2. with state[]=unpublished you should see the course
5. as the student, hit /api/v1/courses/:id/enrollments?user_id=self&state[]=current_and_future
   1. you should get back your enrollment
6. as a teacher, publish the course
7. as the student, hit /api/v1/courses
   1. you should get back the course
8. as the student, hit /api/v1/courses/:id/enrollments?user_id=self
   1. you should get back your enrollment

Change-Id: I75a4f77b0298729088c7f8e3dbc46d551f36f241
Reviewed-on: https://gerrit.instructure.com/35085
Reviewed-by: Jeff Belser <jbelser@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
Product-Review: Marc LeGendre <marc@instructure.com>
QA-Review: Marc LeGendre <marc@instructure.com>
This commit is contained in:
Jon Jensen 2014-05-16 16:36:00 -06:00
parent 0cd74b7214
commit e95b14f196
7 changed files with 156 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
app/controllers/courses_controller.rb
View File

@ -335,11 +335,15 @@ class CoursesController < ApplicationController
format.json {
if params[:state]
params[:state] += %w(created claimed) if params[:state].include? 'unpublished'
enrollments = @current_user.enrollments
enrollments = enrollments.reject { |e| !params[:state].include?(e.course.workflow_state) || (%w(StudentEnrollment ObserverEnrollment).include?(e.type) && %w(created claimed).include?(e.course.workflow_state))}
states = Array(params[:state])
states += %w(created claimed) if states.include?('unpublished')
conditions = states.map{ |state|
Enrollment::QueryBuilder.new(nil, course_workflow_state: state, enforce_course_workflow_state: true).conditions
}.compact.join(" OR ")
enrollments = @current_user.enrollments.joins(:course).includes(:course).where(conditions)
else
enrollments = @current_user.cached_current_enrollments
Enrollment.send(:preload_associations, enrollments, [:course])
end
if params[:enrollment_role]

21
app/controllers/enrollments_api_controller.rb
View File

@ -209,7 +209,16 @@ class EnrollmentsApiController < ApplicationController
#
# @argument state[] [String, "active"|"invited"|"creation_pending"|"deleted"|"rejected"|"completed"|"inactive"]
# Filter by enrollment state. If omitted, 'active' and 'invited' enrollments
# are returned.
# are returned. When querying a user's enrollments (either via user_id
# argument or via user enrollments endpoint), the following additional
# synthetic states are supported: "current_and_invited"|"current_and_future"|"current_and_concluded"
#
# @argument user_id [String]
# Filter by user_id (only valid for course or section enrollment
# queries). If set to the current user's id, this is a way to
# determine if the user has any enrollments in the course or section,
# independent of whether the user has permission to view other people
# on the roster.
#
# @returns [Enrollment]
def index
@ -420,6 +429,13 @@ class EnrollmentsApiController < ApplicationController
#
# Returns an ActiveRecord scope of enrollments on success, false on failure.
def course_index_enrollments
if params[:user_id]
# if you pass in your own id, you can see if you are enrolled in the
# course, regardless of whether you have read_roster
scope = user_index_enrollments
return scope && scope.where(course_id: @context.id)
end
if authorized_action(@context, @current_user, [:read_roster, :view_all_grades, :manage_grades])
scope = @context.enrollments_visible_to(@current_user, :type => :all, :include_priors => true).where(enrollment_index_conditions)
unless params[:state].present?
@ -490,7 +506,8 @@ class EnrollmentsApiController < ApplicationController
if state.present?
if use_course_state
clauses << "(#{state.map{|s| "(#{Enrollment::QueryBuilder.new(s.to_sym).conditions})" }.join(' OR ')})"
conditions = state.map{ |s| Enrollment::QueryBuilder.new(s.to_sym).conditions }.compact
clauses << "(#{conditions.join(' OR ')})"
else
clauses << 'enrollments.workflow_state IN (:workflow_state)'
replacements[:workflow_state] = Array(state)

40
app/models/enrollment/query_builder.rb
View File

@ -11,13 +11,18 @@ class Enrollment
# these conditions to search users in the course (rather than as an
# association on a particular user)
#
# the course_workflow_state option can be used to simplify the
# The course_workflow_state option can be used to simplify the
# query when the enrollments are all known to come from one course
# whose workflow state is already known. when provided, the method may
# whose workflow state is already known. When provided, the method may
# return nil, in which case the condition should be treated as 'always
# false'
#
# The enforce_course_workflow_state option takes things a step
# further; if you are querying enrollments in *multiple* courses and
# want to ensure you only get ones from courses with a given
# workflow_state, set this to true
def initialize(state, options = {})
@state = state
@state = state || COURSE_ENROLLMENT_STATE_MAP[options[:course_workflow_state]]
@options = options.reverse_merge(strict_checks: true)
@builders = infer_sub_builders
end
@ -29,6 +34,11 @@ class Enrollment
QueryBuilder.new(:active, @options),
QueryBuilder.new(:invited, @options)
]
when :current_and_future
[
QueryBuilder.new(:active, @options.merge(strict_checks: false)),
QueryBuilder.new(:invited, @options)
]
when :current_and_concluded
[
QueryBuilder.new(:active, @options),
@ -43,7 +53,7 @@ class Enrollment
def conditions
return @builders.map(&:conditions).join(" OR ") if @builders
case @state
conditions = case @state
when :active
if @options[:strict_checks]
case @options[:course_workflow_state]
@ -117,6 +127,28 @@ class Enrollment
when :creation_pending; "enrollments.workflow_state = 'creation_pending'"
when :inactive; "enrollments.workflow_state = 'inactive'"
end
if conditions && @options[:course_workflow_state] && @options[:enforce_course_workflow_state]
conditions << sanitize_sql(
" AND courses.workflow_state = ?",
@options[:course_workflow_state]
)
end
conditions
end
def sanitize_sql(sql, *args)
ActiveRecord::Base.send :sanitize_sql_array, [sql, *args]
end
# a map of Course#workflow_state <-> compatible #state arguments,
# i.e. infer a compatible value for the latter from the former
COURSE_ENROLLMENT_STATE_MAP = {
available: :current_and_invited,
completed: :completed,
deleted: :deleted,
created: :current_and_future,
claimed: :current_and_future
}.with_indifferent_access.freeze
end
end

8
app/models/user.rb
View File

@ -53,8 +53,8 @@ class User < ActiveRecord::Base
time_zone_attribute :time_zone
include Workflow
def self.enrollment_conditions(state, options = {})
Enrollment::QueryBuilder.new(state, options).conditions
def self.enrollment_conditions(state)
Enrollment::QueryBuilder.new(state).conditions or raise "invalid enrollment conditions"
end
has_many :communication_channels, :order => 'communication_channels.position ASC', :dependent => :destroy
@ -68,7 +68,7 @@ class User < ActiveRecord::Base
has_many :current_and_invited_enrollments, :class_name => 'Enrollment', :include => [:course, :course_section], :order => 'enrollments.created_at',
:conditions => enrollment_conditions(:current_and_invited)
has_many :current_and_future_enrollments, :class_name => 'Enrollment', :include => [:course, :course_section], :order => 'enrollments.created_at',
:conditions => enrollment_conditions(:current_and_invited, strict_checks: false)
:conditions => enrollment_conditions(:current_and_future)
has_many :concluded_enrollments, :class_name => 'Enrollment', :include => [:course, :course_section], :conditions => enrollment_conditions(:completed), :order => 'enrollments.created_at'
has_many :current_and_concluded_enrollments, :class_name => 'Enrollment', :include => [:course, :course_section],
:conditions => enrollment_conditions(:current_and_concluded), :order => 'enrollments.created_at'
@ -78,7 +78,7 @@ class User < ActiveRecord::Base
has_many :current_and_invited_enrollments, :class_name => 'Enrollment', :joins => [:course], :order => 'enrollments.created_at',
:conditions => enrollment_conditions(:current_and_invited), :readonly => false
has_many :current_and_future_enrollments, :class_name => 'Enrollment', :joins => [:course], :order => 'enrollments.created_at',
:conditions => enrollment_conditions(:current_and_invited, strict_checks: false), :readonly => false
:conditions => enrollment_conditions(:current_and_future), :readonly => false
has_many :concluded_enrollments, :class_name => 'Enrollment', :joins => [:course], :conditions => enrollment_conditions(:completed), :order => 'enrollments.created_at', :readonly => false
has_many :current_and_concluded_enrollments, :class_name => 'Enrollment', :joins => [:course],
:conditions => enrollment_conditions(:current_and_concluded), :order => 'enrollments.created_at', :readonly => false

23
spec/apis/v1/courses_api_spec.rb
View File

@ -1179,7 +1179,7 @@ describe CoursesController, type: :request do
end
end
it "should not return courses with StudentEnrollment or ObserverEnrollment when state[] param" do
it "should not return courses with invited StudentEnrollment or ObserverEnrollment when state[]=unpublished" do
@course4.enrollments.each do |e|
e.type = 'StudentEnrollment'
e.save
@ -1187,7 +1187,7 @@ describe CoursesController, type: :request do
json = api_call(:get, "/api/v1/courses.json",
{ :controller => 'courses', :action => 'index', :format => 'json' },
{ :state => ['unpublished'] })
json.collect{ |c| c['id'].to_i }.sort.should ==[@course3.id]
json.collect{ |c| c['id'].to_i }.sort.should == [@course3.id]
@course3.enrollments.each do |e|
e.type = 'ObserverEnrollment'
@ -1196,7 +1196,24 @@ describe CoursesController, type: :request do
json = api_call(:get, "/api/v1/courses.json",
{ :controller => 'courses', :action => 'index', :format => 'json' },
{ :state => ['unpublished'] })
json.collect{ |c| c['id'].to_i }.should ==[]
json.collect{ |c| c['id'].to_i }.should == []
end
it "should return courses with active StudentEnrollment or ObserverEnrollment when state[]=unpublished" do
@course3.enrollments.each do |e|
e.type = 'ObserverEnrollment'
e.workflow_state = "active"
e.save
end
@course4.enrollments.each do |e|
e.type = 'StudentEnrollment'
e.workflow_state = "active"
e.save
end
json = api_call(:get, "/api/v1/courses.json",
{ :controller => 'courses', :action => 'index', :format => 'json' },
{ :state => ['unpublished'] })
json.collect{ |c| c['id'].to_i }.sort.should == [@course3.id, @course4.id]
end
end

17
spec/apis/v1/enrollments_api_spec.rb
View File

@ -856,8 +856,6 @@ describe EnrollmentsApiController, type: :request do
end
it "should not show enrollments for courses that aren't published" do
# Setup test with an unpublished course and an active enrollment in
# that course.
course
@course.claim
enrollment = course.enroll_student(@user)
@ -868,18 +866,29 @@ describe EnrollmentsApiController, type: :request do
json.map { |e| e['id'] }.should_not include enrollment.id
# Request w/ a state[] filter.
json = api_call(:get, "#{@user_path}?state[]=active&type[]=StudentEnrollment",
json = api_call(:get, @user_path,
@user_params.merge(:state => %w{active}, :type => %w{StudentEnrollment}))
json.map { |e| e['id'] }.should_not include enrollment.id
end
it "should show enrollments for courses that aren't published if state[]=current_and_future" do
course
@course.claim
enrollment = course.enroll_student(@user)
enrollment.update_attribute(:workflow_state, 'active')
json = api_call(:get, @user_path,
@user_params.merge(:state => %w{current_and_future}, :type => %w{StudentEnrollment}))
json.map { |e| e['id'] }.should include enrollment.id
end
it "should accept multiple state[] filters" do
course
@course.offer!
enrollment = course.enroll_student(@user)
enrollment.update_attribute(:workflow_state, 'completed')
json = api_call(:get, "#{@user_path}?state[]=active&state[]=completed",
json = api_call(:get, @user_path,
@user_params.merge(:state => %w{active completed}))
json.map { |e| e['id'].to_i }.sort.should == @user.enrollments.map(&:id).sort
end

61
spec/models/enrollment/query_builder_spec.rb
View File

@ -57,6 +57,23 @@ describe "Enrollment::QueryBuilder" do
scope
end
shared_examples_for "enforce_course_workflow_state" do
let(:options){ {strict_checks: false} }
context "with :enforce_course_workflow_state=true" do
it "should reject enrollments in courses with a different workflow_state" do
create_enrollments(
[state.to_s, "available", "StudentEnrollment"]
)
options[:course_workflow_state] = 'unknown'
options[:enforce_course_workflow_state] = true
result = enrollments.where(conditions)
result.should be_empty
end
end
end
context "with :active" do
let(:state){ :active }
@ -70,7 +87,7 @@ describe "Enrollment::QueryBuilder" do
)
end
context "with strict_checks=true" do
context "with strict_checks:true" do
let(:options){ {strict_checks: true} }
it "should return sensible defaults" do
@ -105,7 +122,7 @@ describe "Enrollment::QueryBuilder" do
end
end
context "with strict_checks=false" do
context "with strict_checks:false" do
let(:options){ {strict_checks: false} }
it "should return sensible defaults" do
@ -134,6 +151,8 @@ describe "Enrollment::QueryBuilder" do
]
end
end
it_should_behave_like "enforce_course_workflow_state"
end
context "with :invited" do
@ -153,7 +172,7 @@ describe "Enrollment::QueryBuilder" do
)
end
context "with strict_checks=true" do
context "with strict_checks:true" do
let(:options){ {strict_checks: true} }
it "should return sensible defaults" do
@ -188,7 +207,7 @@ describe "Enrollment::QueryBuilder" do
end
end
context "with strict_checks=false" do
context "with strict_checks:false" do
let(:options){ {strict_checks: false} }
it "should return sensible defaults" do
@ -226,11 +245,14 @@ describe "Enrollment::QueryBuilder" do
]
end
end
it_should_behave_like "enforce_course_workflow_state"
end
[:deleted, :rejected, :completed, :creation_pending, :inactive].each do |state|
context "with #{state.inspect}" do
let(:state){ state }
it "should only return #{state} enrollments" do
create_enrollments(
%w{active available StudentEnrollment},
@ -243,6 +265,8 @@ describe "Enrollment::QueryBuilder" do
[state.to_s, "available", "StudentEnrollment"]
]
end
it_should_behave_like "enforce_course_workflow_state"
end
end
@ -274,6 +298,35 @@ describe "Enrollment::QueryBuilder" do
end
end
context "with :current_and_future" do
let(:state) { :current_and_future }
it "should return sensible defaults" do
create_enrollments(
%w{active available StudentEnrollment},
%w{active available TeacherEnrollment},
%w{active claimed StudentEnrollment},
%w{active claimed TeacherEnrollment},
%w{invited available StudentEnrollment},
%w{invited available TeacherEnrollment},
%w{invited claimed StudentEnrollment},
%w{invited claimed TeacherEnrollment},
%w{creation_pending available StudentEnrollment}
)
result = enrollments.where(conditions)
matches_for(result).should == [
%w{active available StudentEnrollment},
%w{active available TeacherEnrollment},
%w{active claimed StudentEnrollment}, # students can see that they have an active enrollment in an unpublished course
%w{active claimed TeacherEnrollment},
%w{invited available StudentEnrollment},
%w{invited available TeacherEnrollment},
%w{invited claimed TeacherEnrollment}
]
end
end
context "with :current_and_concluded" do
let(:state) { :current_and_concluded }