diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index f202128393d..acd2d74558c 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -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 diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 1085211e64a..bf1e9d21506 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -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 diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 70408f40920..688e496336c 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -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| diff --git a/app/helpers/dashboard_helper.rb b/app/helpers/dashboard_helper.rb index 52a5b79a572..ba9e1c3daa6 100644 --- a/app/helpers/dashboard_helper.rb +++ b/app/helpers/dashboard_helper.rb @@ -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 diff --git a/app/models/context.rb b/app/models/context.rb index 983d518d115..0b81364f17a 100644 --- a/app/models/context.rb +++ b/app/models/context.rb @@ -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 = {} - 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? - end - 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 + value_to_cache = types_to_check.each_with_object({}) do |(key, type_to_check), memo| + memo[key] = type_to_check.call + end + end + Rails.cache.write(cache_key, value_to_cache) + @active_record_types[only_check] = value_to_cache end def allow_wiki_comments diff --git a/app/models/course.rb b/app/models/course.rb index f6a331602ae..c5250cef2cf 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -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) - 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 + delete_unless.call([TAB_ASSIGNMENTS, TAB_QUIZZES], :read, :manage_content, :manage_assignments) + delete_unless.call([TAB_SYLLABUS], :read, :read_syllabus, :manage_content, :manage_assignments) - # 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] } - end + 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) + + # remove outcomes tab for logged-out users or non-students + 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 + 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 - # 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 diff --git a/app/presenters/course_for_menu_presenter.rb b/app/presenters/course_for_menu_presenter.rb index f5f16588ae0..43e1175fb46 100644 --- a/app/presenters/course_for_menu_presenter.rb +++ b/app/presenters/course_for_menu_presenter.rb @@ -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]) - end + @session = session + @opts = opts 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| - presenter = SectionTabPresenter.new(tab, course) - presenter.to_h + 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 diff --git a/spec/apis/v1/courses_api_spec.rb b/spec/apis/v1/courses_api_spec.rb index a8a7cbc6203..ad3f131147d 100644 --- a/spec/apis/v1/courses_api_spec.rb +++ b/spec/apis/v1/courses_api_spec.rb @@ -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 diff --git a/spec/apis/v1/tabs_api_spec.rb b/spec/apis/v1/tabs_api_spec.rb index aeae44dbd73..b35e6f4d4f0 100644 --- a/spec/apis/v1/tabs_api_spec.rb +++ b/spec/apis/v1/tabs_api_spec.rb @@ -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 diff --git a/spec/controllers/announcements_controller_spec.rb b/spec/controllers/announcements_controller_spec.rb index 5e813d447c2..f83142c9a46 100644 --- a/spec/controllers/announcements_controller_spec.rb +++ b/spec/controllers/announcements_controller_spec.rb @@ -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 diff --git a/spec/controllers/context_modules_controller_spec.rb b/spec/controllers/context_modules_controller_spec.rb index c638717902f..a328e47f8f8 100644 --- a/spec/controllers/context_modules_controller_spec.rb +++ b/spec/controllers/context_modules_controller_spec.rb @@ -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 diff --git a/spec/controllers/files_controller_spec.rb b/spec/controllers/files_controller_spec.rb index d517bff0f0f..890d8a5f834 100644 --- a/spec/controllers/files_controller_spec.rb +++ b/spec/controllers/files_controller_spec.rb @@ -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} diff --git a/spec/controllers/outcomes_controller_spec.rb b/spec/controllers/outcomes_controller_spec.rb index d42f9dd7aef..5a69d2a79c9 100644 --- a/spec/controllers/outcomes_controller_spec.rb +++ b/spec/controllers/outcomes_controller_spec.rb @@ -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 diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 534111fca5e..d72aace4e24 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -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 diff --git a/spec/helpers/dashboard_helper_spec.rb b/spec/helpers/dashboard_helper_spec.rb index 0de31ab2e09..22830a9f193 100644 --- a/spec/helpers/dashboard_helper_spec.rb +++ b/spec/helpers/dashboard_helper_spec.rb @@ -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 diff --git a/spec/models/context_spec.rb b/spec/models/context_spec.rb index ca8211dd64b..8460dd05883 100644 --- a/spec/models/context_spec.rb +++ b/spec/models/context_spec.rb @@ -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) diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index 8de99f3335c..fad11167f78 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -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 diff --git a/spec/presenters/course_for_menu_presenter_spec.rb b/spec/presenters/course_for_menu_presenter_spec.rb index d1daddafafb..88f89dd89b3 100644 --- a/spec/presenters/course_for_menu_presenter_spec.rb +++ b/spec/presenters/course_for_menu_presenter_spec.rb @@ -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 diff --git a/spec/selenium/new_files_spec.rb b/spec/selenium/new_files_spec.rb index eaf8611e5b4..1abcf7f19d9 100644 --- a/spec/selenium/new_files_spec.rb +++ b/spec/selenium/new_files_spec.rb @@ -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