Include an option to allow crosslisted courses in api results.
This also adds a new brakeman override about a SQL injection false
positive. `#{Course.best_unicode_collation_key("courses.name")}` is
safe, and any input in `opts[:order]` is generated from
app/controllers/accounts_controller.rb:sort_order, which does not allow
user input to be passed in.
Closes COMMS-574
Test Plan:
* Have an account with two sub-accounts, call them A and B
* Create a course in each sub account, and put a section in
the course in account B.
* Publish these courses, and add them to the public index (in
the settings for each course).
* Crosslist the section in the course in account B into the
course in account A.
* Go to account B's page and ensure that the course in account
A is *not* there.
* Also, go to /api/v1/accounts/#/courses for each account and
check that only courses that *originated* in each account
show up in the JSON response.
* HOWEVER, if you add in the param "include_crosslisted_courses=true"
in the request for the account that the crosslist *originated from*
then both coruses should show.
Change-Id: I3170bd062f8cf3e60c668e6b4bb494e5b0f2991b
Reviewed-on: https://gerrit.instructure.com/134690
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Reviewed-by: Felix Milea-Ciobanu <fmileaciobanu@instructure.com>
Tested-by: Jenkins
QA-Review: Aaron Kc Hsu <ahsu@instructure.com>
Product-Review: Matt Goodwin <mattg@instructure.com>
This commit is contained in:
Venk Natarajan
Landon Gilbert-Bland
parent
9a18b7faa4
commit
ca929f15e6
|
|
@ -182,11 +182,13 @@ class AccountsController < ApplicationController
|
|||
end
|
||||
return redirect_to account_settings_url(@account) if @account.site_admin? || !@account.grants_right?(@current_user, :read_course_list)
|
||||
js_env(:ACCOUNT_COURSES_PATH => account_courses_path(@account, :format => :json))
|
||||
load_course_right_side
|
||||
include_crosslisted_courses = value_to_boolean(params[:include_crosslisted_courses])
|
||||
load_course_right_side(:include_crosslisted_courses => include_crosslisted_courses)
|
||||
@courses = @account.fast_all_courses(:term => @term, :limit => @maximum_courses_im_gonna_show,
|
||||
:hide_enrollmentless_courses => @hide_enrollmentless_courses,
|
||||
:only_master_courses => @only_master_courses,
|
||||
:order => sort_order)
|
||||
:order => sort_order,
|
||||
:include_crosslisted_courses => include_crosslisted_courses)
|
||||
ActiveRecord::Associations::Preloader.new.preload(@courses, :enrollment_term)
|
||||
build_course_stats
|
||||
end
|
||||
|
|
@ -359,7 +361,8 @@ class AccountsController < ApplicationController
|
|||
order += (params[:order] == "desc" ? " DESC, id DESC" : ", id")
|
||||
end
|
||||
|
||||
@courses = @account.associated_courses.order(order).where(:workflow_state => params[:state])
|
||||
opts = { :include_crosslisted_courses => value_to_boolean(params[:include_crosslisted_courses]) }
|
||||
@courses = @account.associated_courses(opts).order(order).where(:workflow_state => params[:state])
|
||||
|
||||
if params[:hide_enrollmentless_courses] || value_to_boolean(params[:with_enrollments])
|
||||
@courses = @courses.with_enrollments
|
||||
|
|
@ -870,7 +873,7 @@ class AccountsController < ApplicationController
|
|||
end
|
||||
end
|
||||
|
||||
def load_course_right_side
|
||||
def load_course_right_side(opts = {})
|
||||
@root_account = @account.root_account
|
||||
@maximum_courses_im_gonna_show = 50
|
||||
@term = nil
|
||||
|
|
@ -878,7 +881,7 @@ class AccountsController < ApplicationController
|
|||
@term = @root_account.enrollment_terms.active.find(params[:enrollment_term_id]) rescue nil
|
||||
@term ||= @root_account.enrollment_terms.active[-1]
|
||||
end
|
||||
associated_courses = @account.associated_courses.active
|
||||
associated_courses = @account.associated_courses(opts).active
|
||||
associated_courses = associated_courses.for_term(@term) if @term
|
||||
@associated_courses_count = associated_courses.count
|
||||
@hide_enrollmentless_courses = params[:hide_enrollmentless_courses] == "1"
|
||||
|
|
@ -1192,7 +1195,7 @@ class AccountsController < ApplicationController
|
|||
:app_center_access_token].freeze
|
||||
|
||||
def permitted_account_attributes
|
||||
[:name, :turnitin_account_id, :turnitin_shared_secret,
|
||||
[:name, :turnitin_account_id, :turnitin_shared_secret, :include_crosslisted_courses,
|
||||
:turnitin_host, :turnitin_comments, :turnitin_pledge, :turnitin_originality,
|
||||
:default_time_zone, :parent_account, :default_storage_quota,
|
||||
:default_storage_quota_mb, :storage_quota, :default_locale,
|
||||
|
|
|
|||
|
|
@ -459,13 +459,18 @@ class Account < ActiveRecord::Base
|
|||
@cached_users_name_like[query] ||= self.fast_all_users.name_like(query)
|
||||
end
|
||||
|
||||
def associated_courses
|
||||
def associated_courses(opts = {})
|
||||
if root_account?
|
||||
all_courses
|
||||
else
|
||||
shard.activate do
|
||||
Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self, course_section_id: nil)
|
||||
.where("course_id=courses.id"))
|
||||
if opts[:include_crosslisted_courses]
|
||||
Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self)
|
||||
.where("course_id=courses.id"))
|
||||
else
|
||||
Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self, course_section_id: nil)
|
||||
.where("course_id=courses.id"))
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
@ -477,7 +482,10 @@ class Account < ActiveRecord::Base
|
|||
def fast_course_base(opts = {})
|
||||
opts[:order] ||= "#{Course.best_unicode_collation_key("courses.name")} ASC"
|
||||
columns = "courses.id, courses.name, courses.workflow_state, courses.course_code, courses.sis_source_id, courses.enrollment_term_id"
|
||||
associated_courses = self.associated_courses.active.order(opts[:order])
|
||||
associated_courses = self.associated_courses(
|
||||
:include_crosslisted_courses => opts[:include_crosslisted_courses]
|
||||
)
|
||||
associated_courses = associated_courses.active.order(opts[:order])
|
||||
associated_courses = associated_courses.with_enrollments if opts[:hide_enrollmentless_courses]
|
||||
associated_courses = associated_courses.master_courses if opts[:only_master_courses]
|
||||
associated_courses = associated_courses.for_term(opts[:term]) if opts[:term].present?
|
||||
|
|
|
|||
|
|
@ -75,6 +75,28 @@
|
|||
},
|
||||
"user_input": "Enrollment.active_student_conditions",
|
||||
"confidence": "High"
|
||||
},
|
||||
{
|
||||
"warning_type": "SQL Injection",
|
||||
"warning_code": 0,
|
||||
"fingerprint": "d8e0d77e896b3da629e04233f458668fff1d130a6b969287822c292af1daeaad",
|
||||
"check_name": "SQL",
|
||||
"message": "Possible SQL injection",
|
||||
"file": "app/models/account.rb",
|
||||
"line": 486,
|
||||
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
|
||||
"code": "self.associated_courses(:include_crosslisted_courses => opts[:include_crosslisted_courses]).active.order(\"#{Course.best_unicode_collation_key(\"courses.name\")} ASC\")",
|
||||
"render_path": null,
|
||||
"location": {
|
||||
"type": "method",
|
||||
"class": "Account",
|
||||
"method": "fast_course_base"
|
||||
},
|
||||
"user_input": "Course.best_unicode_collation_key(\"courses.name\")",
|
||||
"confidence": "High",
|
||||
"note": "No user input is passed in to via opts['order'] in this function (see accounts_controller:sort_order)"
|
||||
}
|
||||
]
|
||||
],
|
||||
"updated": "2017-12-19 10:36:25 -0700",
|
||||
"brakeman_version": "4.1.0"
|
||||
}
|
||||
|
|
|
|||
|
|
@ -696,8 +696,28 @@ describe "Accounts API", type: :request do
|
|||
@account1.account_users.create!(user: @user)
|
||||
json = api_call(:get, "/api/v1/accounts/#{@account1.id}/courses",
|
||||
{ :controller => 'accounts', :action => 'courses_api',
|
||||
:account_id => @account1.to_param, :format => 'json',
|
||||
})
|
||||
:account_id => @account1.to_param, :format => 'json' })
|
||||
expect(json.length).to eq 1
|
||||
expect(json.first["name"]).to eq "course1"
|
||||
end
|
||||
|
||||
it "include crosslisted course when querying account section was crosslisted from if requested" do
|
||||
@account2.account_users.create!(user: @user)
|
||||
json = api_call(:get, "/api/v1/accounts/#{@account2.id}/courses?include_crosslisted_courses=true",
|
||||
{ :controller => 'accounts', :action => 'courses_api',
|
||||
:include_crosslisted_courses => true,
|
||||
:account_id => @account2.to_param, :format => 'json' })
|
||||
expect(json.length).to eq 2
|
||||
names = json.pluck("name")
|
||||
expect(names.include?("course1")).to be_truthy
|
||||
expect(names.include?("course2")).to be_truthy
|
||||
end
|
||||
|
||||
it "don't include crosslisted course when querying account section was crosslisted to even if requested" do
|
||||
@account1.account_users.create!(user: @user)
|
||||
json = api_call(:get, "/api/v1/accounts/#{@account1.id}/courses",
|
||||
{ :controller => 'accounts', :action => 'courses_api',
|
||||
:account_id => @account1.to_param, :format => 'json' })
|
||||
expect(json.length).to eq 1
|
||||
expect(json.first["name"]).to eq "course1"
|
||||
end
|
||||
|
|
|
|||
|
|
@ -228,6 +228,18 @@ describe AccountsController do
|
|||
get 'show', params: {:id => @account1.id }, :format => 'html'
|
||||
expect(assigns[:associated_courses_count]).to eq 1
|
||||
end
|
||||
|
||||
it "if crosslisted a section to another account, do show other if that param is not set" do
|
||||
account_with_admin_logged_in(account: @account2)
|
||||
get 'show', params: {:id => @account2.id, :include_crosslisted_courses => true}, :format => 'html'
|
||||
expect(assigns[:associated_courses_count]).to eq 2
|
||||
end
|
||||
|
||||
it "if crosslisted a section to this account, do *not* show other account's course even if param is not set" do
|
||||
account_with_admin_logged_in(account: @account1)
|
||||
get 'show', params: {:id => @account1.id, :include_crosslisted_courses => true}, :format => 'html'
|
||||
expect(assigns[:associated_courses_count]).to eq 1
|
||||
end
|
||||
end
|
||||
|
||||
# Check that both published and un-published courses have the correct count
|
||||
|
|
|
|||
|
|
@ -141,9 +141,9 @@ describe Account do
|
|||
expect(@account.fast_all_courses({:term => EnrollmentTerm.where(sis_source_id: "T003").first}).map(&:sis_source_id).sort).to eq ["C005", "C006", "C007", "C008", "C009", "C005S", "C006S", "C007S", "C008S", "C009S"].sort
|
||||
end
|
||||
|
||||
it "don't double-count cross-listed courses" do
|
||||
def check_account(account, expected_length, expected_course_names)
|
||||
actual_courses = account.fast_all_courses
|
||||
it "counting cross-listed courses only if requested" do
|
||||
def check_account(account, include_crosslisted_courses, expected_length, expected_course_names)
|
||||
actual_courses = account.fast_all_courses({ :include_crosslisted_courses => include_crosslisted_courses })
|
||||
expect(actual_courses.length).to eq expected_length
|
||||
actual_course_names = actual_courses.pluck("name").sort!
|
||||
expect(actual_course_names).to eq(expected_course_names.sort!)
|
||||
|
|
@ -156,8 +156,10 @@ describe Account do
|
|||
course_b = course_factory({ :account => account_b, :course_name => "course_b" })
|
||||
course_b.course_sections.create!({ :name => "section_b" })
|
||||
course_b.course_sections.first.crosslist_to_course(course_a)
|
||||
check_account(account_a, 1, ["course_a"])
|
||||
check_account(account_b, 1, ["course_b"])
|
||||
check_account(account_a, false, 1, ["course_a"])
|
||||
check_account(account_a, true, 1, ["course_a"])
|
||||
check_account(account_b, false, 1, ["course_b"])
|
||||
check_account(account_b, true, 2, ["course_a", "course_b"])
|
||||
end
|
||||
|
||||
it "should list associated nonenrollmentless courses" do
|
||||
|
|
|
|||
Loading…
Reference in New Issue