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

restore "speed up dashcards by only loading tabs shown" & fix bug 

Fixes: CORE-1964

This reverts commit 72d558f095

this restores the commit (aka, reverts that ^ revert) that was causing
us bugs on caturday. the root cause of the bug was that
["active_record_types", [], self].cache_key
and
["active_record_types", nil, self].cache_key
return the same string.

how to replay the buggy scenerio:
(as a student, in a course with annnouncements):
* right after having "touched" the course (so there's nothing cached),
* have one person go to  /courses/:course_id/assignments/syllabus
* in the controller action for that, it will do:
  `return unless tab_enabled?(@context.class::TAB_SYLLABUS)`
  that will call Course::uncached_tabs_available(only_check: @context.class::TAB_SYLLABUS)
  which would have called @course.active_record_types(only_check: [])
  (because @context.class::TAB_SYLLABUS is not one of the `tabs_that_can_be_marked_hidden_unused`)
  which woud have written `{}` to redis at ['active_record_types', [], self].cache_key
* now, as a different student, go to /courses/:course_id/annnouncements
  it will call `tab_enabled?(@context.class::TAB_ANNOUNCEMENTS)`
  that will call Course::uncached_tabs_available(only_check: @context.class::TAB_ANNOUNCEMENTS)
  which will call @course.active_record_types(only_check: [:announcements])
  it will do a cache read for ['active_record_types', [:announcements], self].cache_key
  since it it a fresh cach, that will not be found
  then it would have done a cache read for the "everything" cache at
  ['active_record_types', nil, self].cache_key
  THAT WOULD HAVE RETURNED THE CACHED `{}` SINCE `nil.cache_key` and `[].cache_key` ARE THE SAME!
* the user would be told the announcement page is not enabled for that course

the fix is to explicitly not allow Context::active_record_types to ever 
be called with `only_check: []`

and for good measure, we don't allow the implicit conversion of 
nil.cache_key to "" and instead use "everything" for the cache cache_key

I added specs to spefically catch this bug so that it doesn't happen again.

To see the difference, compare the latest patchset of this commit against
patchset 1. patchset 1 is the original version of this code without this
fix.

Change-Id: I513104b90dd94227a04c151ee02a22f4a4ac2832
Reviewed-on: https://gerrit.instructure.com/167400
Tested-by: Jenkins
Reviewed-by: Jeremy Stanley <jeremy@instructure.com>
QA-Review: Jeremy Stanley <jeremy@instructure.com>
Product-Review: Ryan Shaw <ryan@instructure.com>
This commit is contained in:
Ryan Shaw 2018-10-08 09:36:54 -06:00
parent e9b241bf05
commit 957ffeaa85
19 changed files with 304 additions and 191 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
app/controllers/application_controller.rb
View File

@ -535,21 +535,16 @@ class ApplicationController < ActionController::Base
end
def tab_enabled?(id, opts = {})
return true unless @context && @context.respond_to?(:tabs_available)
tabs = Rails.cache.fetch(['tabs_available2', @context, @current_user, @domain_root_account,
session[:enrollment_uuid]].cache_key, expires_in: 1.hour) do
precalculated_permissions = @context.is_a?(Course) && @current_user &&
@current_user.precalculate_permissions_for_courses([@context], SectionTabHelper::PERMISSIONS_TO_PRECALCULATE, [@domain_root_account])&.values&.first
return true unless @context&.respond_to?(:tabs_available)
valid = Rails.cache.fetch(['tab_enabled3', id, @context, @current_user, @domain_root_account, session[:enrollment_uuid]].cache_key) do
@context.tabs_available(@current_user,
:session => session,
:include_hidden_unused => true,
:root_account => @domain_root_account,
:precalculated_permissions => precalculated_permissions
)
session: session,
include_hidden_unused: true,
root_account: @domain_root_account,
only_check: [id]
).any?{|t| t[:id] == id }
end
valid = tabs.any?{|t| t[:id] == id }
render_tab_disabled unless valid || opts[:no_render]
return valid
end

9
app/controllers/users_controller.rb
View File

@ -553,10 +553,13 @@ class UsersController < ApplicationController
end
end
def dashboard_cards
cancel_cache_buster
DASHBOARD_CARD_TABS = [
Course::TAB_DISCUSSIONS, Course::TAB_ASSIGNMENTS,
Course::TAB_ANNOUNCEMENTS, Course::TAB_FILES
].freeze
dashboard_courses = map_courses_for_menu(@current_user.menu_courses, :include_section_tabs => true)
def dashboard_cards
dashboard_courses = map_courses_for_menu(@current_user.menu_courses, tabs: DASHBOARD_CARD_TABS)
Rails.cache.write(['last_known_dashboard_cards_count', @current_user].cache_key, dashboard_courses.count)
render json: dashboard_courses
end

18
app/helpers/application_helper.rb
View File

@ -562,24 +562,6 @@ module ApplicationHelper
super
end
def map_courses_for_menu(courses, opts={})
precalculated_tab_permissions = opts[:include_section_tabs] && @current_user &&
Rails.cache.fetch(['precalculated_permissions_for_menu', @current_user, collection_cache_key(courses)]) do
@current_user.precalculate_permissions_for_courses(courses, SectionTabHelper::PERMISSIONS_TO_PRECALCULATE)
end
mapped = courses.map do |course|
tabs = opts[:include_section_tabs] && available_section_tabs(course, precalculated_tab_permissions&.dig(course.global_id))
presenter = CourseForMenuPresenter.new(course, tabs, @current_user, @domain_root_account)
presenter.to_h
end
if @domain_root_account.feature_enabled?(:dashcard_reordering)
mapped = mapped.sort_by {|h| h[:position] || ::CanvasSort::Last}
end
mapped
end
# return enough group data for the planner to display items associated with groups
def map_groups_for_planner(groups)
mapped = groups.map do |g|

12
app/helpers/dashboard_helper.rb
View File

@ -149,4 +149,16 @@ module DashboardHelper
todo_ignore_dropdown_type?(activity_type) ? 'al-trigger disable_item_link' : 'disable_item_link disable-todo-item-link'
end
def map_courses_for_menu(courses, opts={})
mapped = courses.map do |course|
presenter = CourseForMenuPresenter.new(course, @current_user, @domain_root_account, session, opts)
presenter.to_h
end
if @domain_root_account.feature_enabled?(:dashcard_reordering)
mapped = mapped.sort_by {|h| h[:position] || ::CanvasSort::Last}
end
mapped
end
end

57
app/models/context.rb
View File

@ -108,22 +108,53 @@ module Context
Canvas::ICU.collate_by(contexts) { |r| r[:name] }
end
def active_record_types
@active_record_types ||= Rails.cache.fetch(['active_record_types2', self].cache_key) do
res = {}
def active_record_types(only_check: nil)
only_check = only_check.sort if only_check.present? # so that we always have consistent cache keys
@active_record_types ||= {}
return @active_record_types[only_check] if @active_record_types[only_check]
possible_types = {
files: -> { self.respond_to?(:attachments) && self.attachments.active.exists? },
modules: -> { self.respond_to?(:context_modules) && self.context_modules.active.exists? },
quizzes: -> { self.respond_to?(:quizzes) && self.quizzes.active.exists? },
assignments: -> { self.respond_to?(:assignments) && self.assignments.active.exists? },
pages: -> { self.respond_to?(:wiki_pages) && self.wiki_pages.active.exists? },
conferences: -> { self.respond_to?(:web_conferences) && self.web_conferences.active.exists? },
announcements: -> { self.respond_to?(:announcements) && self.announcements.active.exists? },
outcomes: -> { self.respond_to?(:has_outcomes?) && self.has_outcomes? },
discussions: -> { self.respond_to?(:discussion_topics) && self.discussion_topics.only_discussion_topics.except(:preload).exists? }
}
types_to_check = if only_check
possible_types.select { |k| only_check.include?(k) }
else
possible_types
end
raise ArgumentError, "only_check is either an empty array or you are aking for invalid types" if types_to_check.empty?
base_cache_key = 'active_record_types3'
cache_key = [base_cache_key, (only_check.present? ? only_check : 'everything'), self].cache_key
# if it exists in redis, return that
if (cached = Rails.cache.read(cache_key))
return @active_record_types[only_check] = cached
end
# if we're only asking for a subset but the full set is cached return that, but filtered with just what we want
if only_check.present? && (cache_with_everything = Rails.cache.read([base_cache_key, 'everything', self].cache_key))
return @active_record_types[only_check] = cache_with_everything.select { |k,_v| only_check.include?(k) }
end
# otherwise compute it and store it in the cache
value_to_cache = nil
ActiveRecord::Base.uncached do
res[:files] = self.respond_to?(:attachments) && self.attachments.active.exists?
res[:modules] = self.respond_to?(:context_modules) && self.context_modules.active.exists?
res[:quizzes] = self.respond_to?(:quizzes) && self.quizzes.active.exists?
res[:assignments] = self.respond_to?(:assignments) && self.assignments.active.exists?
res[:pages] = self.respond_to?(:wiki_pages) && self.wiki_pages.active.exists?
res[:conferences] = self.respond_to?(:web_conferences) && self.web_conferences.active.exists?
res[:announcements] = self.respond_to?(:announcements) && self.announcements.active.exists?
res[:outcomes] = self.respond_to?(:has_outcomes?) && self.has_outcomes?
res[:discussions] = self.respond_to?(:discussion_topics) && self.discussion_topics.only_discussion_topics.except(:preload).exists?
value_to_cache = types_to_check.each_with_object({}) do |(key, type_to_check), memo|
memo[key] = type_to_check.call
end
res
end
Rails.cache.write(cache_key, value_to_cache)
@active_record_types[only_check] = value_to_cache
end
def allow_wiki_comments

130
app/models/course.rb
View File

@ -2615,7 +2615,7 @@ class Course < ActiveRecord::Base
end
def tabs_available(user=nil, opts={})
opts.reverse_merge!(:include_external => true)
opts.reverse_merge!(:include_external => true, include_hidden_unused: true)
cache_key = [user, opts].cache_key
@tabs_available ||= {}
@tabs_available[cache_key] ||= uncached_tabs_available(user, opts)
@ -2657,6 +2657,10 @@ class Course < ActiveRecord::Base
tabs.delete_if {|t| t[:id] == TAB_SETTINGS }
tabs << settings_tab
if opts[:only_check]
tabs = tabs.select { |t| opts[:only_check].include?(t[:id]) }
end
check_for_permission = lambda do |*permissions|
permissions.any? do |permission|
if opts[:precalculated_permissions]&.has_key?(permission)
@ -2667,75 +2671,83 @@ class Course < ActiveRecord::Base
end
end
tabs.each do |tab|
tab[:hidden_unused] = true if tab[:id] == TAB_MODULES && !active_record_types[:modules]
tab[:hidden_unused] = true if tab[:id] == TAB_FILES && !active_record_types[:files]
tab[:hidden_unused] = true if tab[:id] == TAB_QUIZZES && !active_record_types[:quizzes]
tab[:hidden_unused] = true if tab[:id] == TAB_ASSIGNMENTS && !active_record_types[:assignments]
tab[:hidden_unused] = true if tab[:id] == TAB_PAGES && !active_record_types[:pages] && !allow_student_wiki_edits
tab[:hidden_unused] = true if tab[:id] == TAB_CONFERENCES && !active_record_types[:conferences] && !check_for_permission.call(:create_conferences)
tab[:hidden_unused] = true if tab[:id] == TAB_ANNOUNCEMENTS && !active_record_types[:announcements]
tab[:hidden_unused] = true if tab[:id] == TAB_OUTCOMES && !active_record_types[:outcomes]
tab[:hidden_unused] = true if tab[:id] == TAB_DISCUSSIONS && !active_record_types[:discussions] && !allow_student_discussion_topics
delete_unless = lambda do |tabs_to_check, *permissions|
matched_tabs = tabs.select { |t| tabs_to_check.include?(t[:id]) }
tabs -= matched_tabs if matched_tabs.present? && !check_for_permission.call(*permissions)
end
tabs_that_can_be_marked_hidden_unused = [
{id: TAB_MODULES, relation: :modules},
{id: TAB_FILES, relation: :files},
{id: TAB_QUIZZES, relation: :quizzes},
{id: TAB_ASSIGNMENTS, relation: :assignments},
{id: TAB_ANNOUNCEMENTS, relation: :announcements},
{id: TAB_OUTCOMES, relation: :outcomes},
{id: TAB_PAGES, relation: :pages, additional_check: -> { allow_student_wiki_edits }},
{id: TAB_CONFERENCES, relation: :conferences, additional_check: -> { check_for_permission.call(:create_conferences) }},
{id: TAB_DISCUSSIONS, relation: :discussions, additional_check: -> { allow_student_discussion_topics }}
].select{ |hidable_tab| tabs.any?{ |t| t[:id] == hidable_tab[:id] } }
if tabs_that_can_be_marked_hidden_unused.present?
ar_types = active_record_types(only_check: tabs_that_can_be_marked_hidden_unused.map{|t| t[:relation]})
tabs_that_can_be_marked_hidden_unused.each do |t|
if !ar_types[t[:relation]] && (!t[:additional_check] || !t[:additional_check].call)
# that means there are none of this type of thing in the DB
if opts[:include_hidden_unused] || opts[:for_reordering] || opts[:api]
tabs.detect{ |tab| tab[:id] == t[:id] }[:hidden_unused] = true
else
tabs.delete_if{ |tab| tab[:id] == t[:id] }
end
end
end
end
# remove tabs that the user doesn't have access to
unless opts[:for_reordering]
unless check_for_permission.call(:read, :manage_content)
tabs.delete_if { |t| t[:id] == TAB_HOME }
tabs.delete_if { |t| t[:id] == TAB_ANNOUNCEMENTS }
tabs.delete_if { |t| t[:id] == TAB_PAGES }
tabs.delete_if { |t| t[:id] == TAB_OUTCOMES }
tabs.delete_if { |t| t[:id] == TAB_CONFERENCES }
tabs.delete_if { |t| t[:id] == TAB_COLLABORATIONS }
tabs.delete_if { |t| t[:id] == TAB_MODULES }
end
unless check_for_permission.call(:participate_as_student, :read_as_admin)
tabs.delete_if{ |t| t[:visibility] == 'members' }
end
unless check_for_permission.call(:read, :manage_content, :manage_assignments)
tabs.delete_if { |t| t[:id] == TAB_ASSIGNMENTS }
tabs.delete_if { |t| t[:id] == TAB_QUIZZES }
end
unless check_for_permission.call(:read, :read_syllabus, :manage_content, :manage_assignments)
tabs.delete_if { |t| t[:id] == TAB_SYLLABUS }
end
tabs.delete_if{ |t| t[:visibility] == 'admins' } unless check_for_permission.call(:read_as_admin)
if check_for_permission.call(:manage_content, :manage_assignments)
tabs.detect { |t| t[:id] == TAB_ASSIGNMENTS }[:manageable] = true
tabs.detect { |t| t[:id] == TAB_SYLLABUS }[:manageable] = true
tabs.detect { |t| t[:id] == TAB_QUIZZES }[:manageable] = true
end
tabs.delete_if { |t| t[:hidden] && t[:external] } unless opts[:api] && check_for_permission.call(:read_as_admin)
tabs.delete_if { |t| t[:id] == TAB_GRADES } unless check_for_permission.call(:read_grades, :view_all_grades, :manage_grades)
tabs.detect { |t| t[:id] == TAB_GRADES }[:manageable] = true if check_for_permission.call(:view_all_grades, :manage_grades)
tabs.delete_if { |t| t[:id] == TAB_PEOPLE } unless check_for_permission.call(:read_roster, :manage_students, :manage_admin_users)
tabs.detect { |t| t[:id] == TAB_PEOPLE }[:manageable] = true if check_for_permission.call(:manage_students, :manage_admin_users)
tabs.delete_if { |t| t[:id] == TAB_FILES } unless check_for_permission.call(:read, :manage_files)
tabs.detect { |t| t[:id] == TAB_FILES }[:manageable] = true if check_for_permission.call(:manage_files)
tabs.delete_if { |t| t[:id] == TAB_DISCUSSIONS } unless check_for_permission.call(:read_forum, :post_to_forum, :create_forum, :moderate_forum)
tabs.detect { |t| t[:id] == TAB_DISCUSSIONS }[:manageable] = true if check_for_permission.call(:moderate_forum)
tabs.delete_if { |t| t[:id] == TAB_SETTINGS } unless check_for_permission.call(:read_as_admin)
delete_unless.call([TAB_HOME, TAB_ANNOUNCEMENTS, TAB_PAGES, TAB_OUTCOMES, TAB_CONFERENCES, TAB_COLLABORATIONS, TAB_MODULES], :read, :manage_content)
unless check_for_permission.call(:read_announcements)
tabs.delete_if { |t| t[:id] == TAB_ANNOUNCEMENTS }
end
member_only_tabs = tabs.select{ |t| t[:visibility] == 'members' }
tabs -= member_only_tabs if member_only_tabs.present? && !check_for_permission.call(:participate_as_student, :read_as_admin)
delete_unless.call([TAB_ASSIGNMENTS, TAB_QUIZZES], :read, :manage_content, :manage_assignments)
delete_unless.call([TAB_SYLLABUS], :read, :read_syllabus, :manage_content, :manage_assignments)
admin_only_tabs = tabs.select{ |t| t[:visibility] == 'admins' }
tabs -= admin_only_tabs if admin_only_tabs.present? && !check_for_permission.call(:read_as_admin)
hidden_exteral_tabs = tabs.select{ |t| t[:hidden] && t[:external] }
tabs -= hidden_exteral_tabs if hidden_exteral_tabs.present? && !(opts[:api] && check_for_permission.call(:read_as_admin))
delete_unless.call([TAB_GRADES], :read_grades, :view_all_grades, :manage_grades)
delete_unless.call([TAB_PEOPLE], :read_roster, :manage_students, :manage_admin_users)
delete_unless.call([TAB_FILES], :read, :manage_files)
delete_unless.call([TAB_DISCUSSIONS], :read_forum, :post_to_forum, :create_forum, :moderate_forum)
delete_unless.call([TAB_SETTINGS], :read_as_admin)
delete_unless.call([TAB_ANNOUNCEMENTS], :read_announcements)
if !user || !check_for_permission.call(:manage_content)
# remove outcomes tab for logged-out users or non-students
unless check_for_permission.call(:participate_as_student, :read_as_admin)
tabs.delete_if { |t| t[:id] == TAB_OUTCOMES }
end
outcome_tab = tabs.detect { |t| t[:id] == TAB_OUTCOMES }
tabs.delete(outcome_tab) if outcome_tab && (!user || !check_for_permission.call(:manage_content, :participate_as_student, :read_as_admin))
# remove hidden tabs from students
unless check_for_permission.call(:read_as_admin)
tabs.delete_if {|t| (t[:hidden] || (t[:hidden_unused] && !opts[:include_hidden_unused])) && !t[:manageable] }
additional_checks = {
TAB_ASSIGNMENTS => [:manage_content, :manage_assignments],
TAB_SYLLABUS => [:manage_content, :manage_assignments],
TAB_QUIZZES => [:manage_content, :manage_assignments],
TAB_GRADES => [:view_all_grades, :manage_grades],
TAB_PEOPLE => [:manage_students, :manage_admin_users],
TAB_FILES => [:manage_files],
TAB_DISCUSSIONS => [:moderate_forum]
}
tabs.reject! do |t|
# tab shouldn't be shown to non-admins
(t[:hidden] || t[:hidden_unused]) &&
# not an admin user
(!user || !check_for_permission.call(:manage_content, :read_as_admin)) &&
# can't do any of the additional things required
(!additional_checks[t[:id]] || !check_for_permission.call(*additional_checks[t[:id]]))
end
end
end
# Uncommenting these lines will always put hidden links after visible links
# tabs.each_with_index{|t, i| t[:sort_index] = i }
# tabs = tabs.sort_by{|t| [t[:hidden_unused] || t[:hidden] ? 1 : 0, t[:sort_index]] } if !self.tab_configuration || self.tab_configuration.empty?
tabs
end
end

34
app/presenters/course_for_menu_presenter.rb
View File

@ -19,20 +19,14 @@ class CourseForMenuPresenter
include I18nUtilities
include Rails.application.routes.url_helpers
DASHBOARD_CARD_TABS = [
Course::TAB_DISCUSSIONS, Course::TAB_ASSIGNMENTS,
Course::TAB_ANNOUNCEMENTS, Course::TAB_FILES
].freeze
def initialize(course, available_section_tabs, user = nil, context = nil)
def initialize(course, user = nil, context = nil, session = nil, opts={})
@course = course
@user = user
@context = context
@available_section_tabs = (available_section_tabs || []).select do |tab|
DASHBOARD_CARD_TABS.include?(tab[:id])
@session = session
@opts = opts
end
end
attr_reader :course, :available_section_tabs
attr_reader :course
def to_h
{
@ -47,13 +41,25 @@ class CourseForMenuPresenter
enrollmentType: course.primary_enrollment_type,
id: course.id,
image: course.feature_enabled?(:course_card_images) ? course.image : nil,
position: (@context && @context.feature_enabled?(:dashcard_reordering)) ? @user.dashboard_positions[course.asset_string] : nil,
links: available_section_tabs.map do |tab|
position: @context&.feature_enabled?(:dashcard_reordering) ? @user.dashboard_positions[course.asset_string] : nil,
}.tap do |hash|
if @opts[:tabs]
tabs = course.tabs_available(@user, {
session: @session,
only_check: @opts[:tabs],
precalculated_permissions: {
# we can assume they can read the course at this point
read: true,
},
include_external: false,
include_hidden_unused: false,
})
hash[:links] = tabs.map do |tab|
presenter = SectionTabPresenter.new(tab, course)
presenter.to_h
end
}
end
end
end
private

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

@ -590,7 +590,13 @@ describe CoursesController, type: :request do
c1 = course_with_student(course_name: 'def', active_all: true).course
json = api_call(:get, "/api/v1/courses.json?include[]=tabs", controller: 'courses', action: 'index', format: 'json', include: ['tabs'])
expect(json.first['tabs']).to be_present
expect(json.first['tabs']).to match_array([
a_hash_including({"id" => "home"}),
a_hash_including({"id" => "discussions"}),
a_hash_including({"id" => "grades"}),
a_hash_including({"id" => "people"}),
a_hash_including({"id" => "syllabus"}),
])
end
describe "user index" do

26
spec/apis/v1/tabs_api_spec.rb
View File

@ -275,8 +275,23 @@ describe TabsController, type: :request do
it "doesn't include hidden tabs for student" do
course_with_student(active_all: true)
tab_ids = [0, 1, 3, 8, 5, 6, 14, 2, 11, 15, 4, 10, 13]
hidden_tabs = [3, 8, 5]
tab_ids = [
Course::TAB_HOME,
Course::TAB_SYLLABUS,
Course::TAB_ASSIGNMENTS,
Course::TAB_DISCUSSIONS,
Course::TAB_GRADES,
Course::TAB_PEOPLE,
Course::TAB_ANNOUNCEMENTS,
Course::TAB_PAGES,
Course::TAB_FILES,
Course::TAB_OUTCOMES,
Course::TAB_QUIZZES,
Course::TAB_MODULES,
Course::TAB_OUTCOMES
]
hidden_tabs = [Course::TAB_ASSIGNMENTS, Course::TAB_DISCUSSIONS, Course::TAB_GRADES]
@course.tab_configuration = tab_ids.map do |n|
hash = {'id' => n}
hash['hidden'] = true if hidden_tabs.include?(n)
@ -285,8 +300,11 @@ describe TabsController, type: :request do
@course.save
json = api_call(:get, "/api/v1/courses/#{@course.id}/tabs", { :controller => 'tabs', :action => 'index',
:course_id => @course.to_param, :format => 'json'})
expect(json.count).to eq 3
json.each {|t| expect(%w{home syllabus people}).to include(t['id'])}
expect(json).to match_array([
a_hash_including({"id" => "home"}),
a_hash_including({"id" => "syllabus"}),
a_hash_including({"id" => "people"}),
])
end
describe "teacher in a course" do

1
spec/controllers/announcements_controller_spec.rb
View File

@ -59,6 +59,7 @@ describe AnnouncementsController do
it "returns new bundle for course announcements if section specific enabled" do
user_session(@user)
@course.announcements.create!(message: 'hello!')
get 'index', params: { :course_id => @course.id }
expect(response).to be_successful
expect(assigns[:js_bundles].length).to eq 1

2
spec/controllers/context_modules_controller_spec.rb
View File

@ -39,7 +39,7 @@ describe ContextModulesController do
end
it "should assign variables" do
user_session(@student)
user_session(@teacher)
get 'index', params: {:course_id => @course.id}
expect(response).to be_successful
end

1
spec/controllers/files_controller_spec.rb
View File

@ -212,6 +212,7 @@ describe FilesController do
end
it "should not show restricted external tools to students" do
course_file
@course.enroll_student(@user).accept!
get 'index', params: {:course_id => @course.id}

2
spec/controllers/outcomes_controller_spec.rb
View File

@ -55,7 +55,7 @@ describe OutcomesController do
end
it "should assign variables" do
user_session(@student)
user_session(@teacher)
get 'index', params: {:course_id => @course.id}
expect(response).to be_successful
end

28
spec/helpers/application_helper_spec.rb
View File

@ -694,34 +694,6 @@ describe ApplicationHelper do
end
end
describe "map_courses_for_menu" do
context "with Dashcard Reordering feature enabled" do
before(:each) do
@account = Account.default
@account.enable_feature! :dashcard_reordering
@domain_root_account = @account
end
it "returns the list of courses sorted by position" do
course1 = @account.courses.create!
course2 = @account.courses.create!
course3 = @account.courses.create!
user = user_model
course1.enroll_student(user)
course2.enroll_student(user)
course3.enroll_student(user)
courses = [course1, course2, course3]
user.dashboard_positions[course1.asset_string] = 3
user.dashboard_positions[course2.asset_string] = 2
user.dashboard_positions[course3.asset_string] = 1
user.save!
@current_user = user
mapped_courses = map_courses_for_menu(courses)
expect(mapped_courses.map {|h| h[:id]}).to eq [course3.id, course2.id, course1.id]
end
end
end
describe "map_groups_for_planner" do
context "with planner enabled" do
before(:each) do

28
spec/helpers/dashboard_helper_spec.rb
View File

@ -72,4 +72,32 @@ describe DashboardHelper do
expect(user_dashboard_view).to eq 'activity'
end
end
describe "map_courses_for_menu" do
context "with Dashcard Reordering feature enabled" do
before(:each) do
@account = Account.default
@account.enable_feature! :dashcard_reordering
@domain_root_account = @account
end
it "returns the list of courses sorted by position" do
course1 = @account.courses.create!
course2 = @account.courses.create!
course3 = @account.courses.create!
user = user_model
course1.enroll_student(user)
course2.enroll_student(user)
course3.enroll_student(user)
courses = [course1, course2, course3]
user.dashboard_positions[course1.asset_string] = 3
user.dashboard_positions[course2.asset_string] = 2
user.dashboard_positions[course3.asset_string] = 1
user.save!
@current_user = user
mapped_courses = map_courses_for_menu(courses)
expect(mapped_courses.map {|h| h[:id]}).to eq [course3.id, course2.id, course1.id]
end
end
end
end

36
spec/models/context_spec.rb
View File

@ -221,6 +221,42 @@ describe Context do
end
end
describe "#active_record_types" do
let(:course) { Course.create! }
it "looks at the 'everything' cache if asking for just one thing and doesn't have a cache for that" do
# it should look first for the cache for just the thing we are asking for
expect(Rails.cache).to receive(:read).
with(['active_record_types3', [:assignments], course].cache_key).
and_return(nil)
# if that ^ returns nil, it should then look for for the "everything" cache
expect(Rails.cache).to receive(:read).
with(['active_record_types3', 'everything', course].cache_key).
and_return({
other_thing_we_are_not_asking_for: true,
assignments: "the cached value for :assignments from the 'everything' cache"
})
expect(course.active_record_types(only_check: [:assignments])).to eq({
assignments: "the cached value for :assignments from the 'everything' cache"
})
end
it "raises an ArgumentError if you pass (only_check: [])" do
expect{
course.active_record_types(only_check: [])
}.to raise_exception ArgumentError
end
it "raises an ArgumentError if you pass bogus values as only_check" do
expect{
course.active_record_types(only_check: [:bogus_type, :other_bogus_tab])
}.to raise_exception ArgumentError
end
end
describe "last_updated_at" do
before :once do
@course1 = Course.create!(name: "course1", updated_at: 1.year.ago)

8
spec/models/course_spec.rb
View File

@ -2144,18 +2144,18 @@ describe Course, "tabs_available" do
end
it "should handle hidden_unused correctly for discussions" do
tabs = @course.uncached_tabs_available(@teacher, {})
tabs = @course.uncached_tabs_available(@teacher, include_hidden_unused: true)
dtab = tabs.detect{|t| t[:id] == Course::TAB_DISCUSSIONS}
expect(dtab[:hidden_unused]).to be_falsey
@course.allow_student_discussion_topics = false
tabs = @course.uncached_tabs_available(@teacher, {})
tabs = @course.uncached_tabs_available(@teacher, include_hidden_unused: true)
dtab = tabs.detect{|t| t[:id] == Course::TAB_DISCUSSIONS}
expect(dtab[:hidden_unused]).to be_truthy
@course.allow_student_discussion_topics = true
discussion_topic_model
tabs = @course.uncached_tabs_available(@teacher, {})
tabs = @course.uncached_tabs_available(@teacher, include_hidden_unused: true)
dtab = tabs.detect{|t| t[:id] == Course::TAB_DISCUSSIONS}
expect(dtab[:hidden_unused]).to be_falsey
end
@ -2168,7 +2168,7 @@ describe Course, "tabs_available" do
it "should not include Announcements without read_announcements rights" do
@course.account.role_overrides.create!(:role => teacher_role, :permission => 'read_announcements', :enabled => false)
tab_ids = @course.uncached_tabs_available(@teacher, {}).map{|t| t[:id] }
tab_ids = @course.uncached_tabs_available(@teacher, include_hidden_unused: true).map{|t| t[:id] }
expect(tab_ids).to_not include(Course::TAB_ANNOUNCEMENTS)
end
end

63
spec/presenters/course_for_menu_presenter_spec.rb
View File

@ -20,25 +20,11 @@ require File.expand_path(File.dirname(__FILE__) + '/../sharding_spec_helper')
describe CourseForMenuPresenter do
let_once(:course) { course_model }
let_once(:available_section_tabs) do
Course.default_tabs.select do |tab|
[ Course::TAB_ASSIGNMENTS, Course::TAB_HOME ].include?(tab[:id])
end
end
let_once(:presenter) do
CourseForMenuPresenter.new(
course, available_section_tabs
)
end
let_once(:user) { user_model }
let(:dashboard_card_tabs) { UsersController::DASHBOARD_CARD_TABS }
describe '#initialize' do
it 'should limit available_section_tabs to be those for dashboard' do
available_section_tab_ids = presenter.available_section_tabs.map do |tab|
tab[:id]
end
expect(available_section_tab_ids).to include(Course::TAB_ASSIGNMENTS)
expect(available_section_tab_ids).not_to include(Course::TAB_HOME)
end
let_once(:presenter) do
CourseForMenuPresenter.new(course, user, nil, nil, {tabs: dashboard_card_tabs})
end
describe '#to_h' do
@ -46,15 +32,40 @@ describe CourseForMenuPresenter do
expect(presenter.to_h).to be_a Hash
end
it 'should include available_section_tabs as link element of hash' do
expect(presenter.to_h[:links].length).to eq presenter.available_section_tabs.length
it 'shouldnt include tab links to unenrolled users' do
expect(presenter.to_h[:links]).to be_empty
end
it 'should show all the tab links to a teacher' do
course.enroll_teacher(user).accept
course.assignments.create!
course.discussion_topics.create!
course.announcements.create! title: 'hear ye!', message: 'wat'
course.attachments.create! filename: 'blah', uploaded_data: StringIO.new('blah')
expect(presenter.to_h[:links]).to match_array([
a_hash_including({css_class: "announcements", icon: "icon-announcement", label: "Announcements"}),
a_hash_including({css_class: "discussions", icon: "icon-discussion", label: "Discussions"}),
a_hash_including({css_class: "assignments", icon: "icon-assignment", label: "Assignments"}),
a_hash_including({css_class: "files", icon: "icon-folder", label: "Files"})
])
end
it 'should only show the tabs a student has access to to students' do
course.enroll_student(user).accept
course.assignments.create!
course.attachments.create! filename: 'blah', uploaded_data: StringIO.new('blah')
expect(presenter.to_h[:links]).to match_array([
a_hash_including({css_class: "assignments", icon: "icon-assignment", label: "Assignments"}),
a_hash_including({css_class: "files", icon: "icon-folder", label: "Files"})
])
end
it 'returns the course nickname if one is set' do
user = user_model
user.course_nicknames[course.id] = 'nickname'
user.save!
cs_presenter = CourseForMenuPresenter.new(course, nil, user)
cs_presenter = CourseForMenuPresenter.new(course, user)
h = cs_presenter.to_h
expect(h[:originalName]).to eq course.name
expect(h[:shortName]).to eq 'nickname'
@ -67,17 +78,15 @@ describe CourseForMenuPresenter do
end
it 'returns a position if one is set' do
user = user_model
user.dashboard_positions[course.asset_string] = 3
user.save!
cs_presenter = CourseForMenuPresenter.new(course, nil, user, @account)
cs_presenter = CourseForMenuPresenter.new(course, user, @account)
h = cs_presenter.to_h
expect(h[:position]).to eq 3
end
it 'returns nil when no position is set' do
user = user_model
cs_presenter = CourseForMenuPresenter.new(course, nil, user, @account)
cs_presenter = CourseForMenuPresenter.new(course, user, @account)
h = cs_presenter.to_h
expect(h[:position]).to eq nil
end
@ -97,7 +106,7 @@ describe CourseForMenuPresenter do
@cs_course = account.courses.create!
@cs_course.primary_enrollment_role_id = @role.local_id
end
cs_presenter = CourseForMenuPresenter.new(@cs_course, available_section_tabs)
cs_presenter = CourseForMenuPresenter.new(@cs_course)
expect(cs_presenter.send(:role)).to eq @role
end
end

1
spec/selenium/new_files_spec.rb
View File

@ -170,6 +170,7 @@ describe "better_file_browsing" do
include_context "public course as a logged out user"
it "should display course files", priority: "1", test_id: 270032 do
public_course.attachments.create!(:filename => "somefile.doc", :uploaded_data => StringIO.new('test'))
get "/courses/#{public_course.id}/files"
expect(f('.ef-main')).to be_displayed
end