From 74e1b6ce67aecdc664ada120612f7e194ab67a0c Mon Sep 17 00:00:00 2001 From: Keith Garner Date: Thu, 28 Jul 2016 09:08:22 -0500 Subject: [PATCH] honor gradebook inactive/concluded settings in export Gradebook export now honors the settings from gradebook related to showing active and inactive enrollments. fixes CNVS-30640 test plan: - Create a course with an assignment, and three students, one active, one inactive, and one concluded. - In gradebook turn off show inactive and show concluded. - Export the gradebook and note that only the active student shows. - In gradebook turn on show inactive. - Export the gradebook and note that both the active and inactive students show. - In gradebook turn off show inactive and turn on show concluded. - Export the gradebook and note that both the active and concluded students show. - In gradebook turn on show inactive and turn on show concluded. - Export the gradebook and note that all the students show - Edit the export and change the grade for all students. - Upload the export and note that only the active and inactive students are graded Change-Id: I40dfe08991049f0689e05588f189b33a59cf9813 Reviewed-on: https://gerrit.instructure.com/86570 Reviewed-by: Derek Bender Reviewed-by: Jeremy Neander Tested-by: Jenkins QA-Review: KC Naegle Product-Review: Christi Wruck --- app/coffeescripts/gradebook2/Gradebook.coffee | 2 - app/controllers/gradebook_csvs_controller.rb | 3 +- app/models/assignment/speed_grader.rb | 28 +++---------- app/models/course.rb | 2 + lib/gradebook_exporter.rb | 7 ++-- lib/gradebook_importer.rb | 2 +- lib/gradebook_settings_helpers.rb | 41 +++++++++++++++++++ .../controllers/gradebooks_controller_spec.rb | 2 +- spec/lib/gradebook_exporter_spec.rb | 12 +++++- spec/lib/gradebook_importer_spec.rb | 20 ++++----- spec/models/course_spec.rb | 11 ++++- 11 files changed, 86 insertions(+), 44 deletions(-) create mode 100644 lib/gradebook_settings_helpers.rb diff --git a/app/coffeescripts/gradebook2/Gradebook.coffee b/app/coffeescripts/gradebook2/Gradebook.coffee index 76635fb21cf..0696c2e93ff 100644 --- a/app/coffeescripts/gradebook2/Gradebook.coffee +++ b/app/coffeescripts/gradebook2/Gradebook.coffee @@ -1165,10 +1165,8 @@ define [ $('#download_csv').prop('disabled', true) $('.icon-import').parent().focus() loading_interval = self.exportingGradebookStatus() - include_priors = $('#show_concluded_enrollments').prop('checked') params = - include_priors: include_priors grading_period_id: @getGradingPeriodToShow() $.ajaxJSON( diff --git a/app/controllers/gradebook_csvs_controller.rb b/app/controllers/gradebook_csvs_controller.rb index 035f177a0d8..6ea67f58bca 100644 --- a/app/controllers/gradebook_csvs_controller.rb +++ b/app/controllers/gradebook_csvs_controller.rb @@ -28,8 +28,7 @@ class GradebookCsvsController < ApplicationController csv_options = { include_sis_id: @context.grants_any_right?(@current_user, session, :read_sis, :manage_sis), - grading_period_id: params[:grading_period_id], - include_priors: Canvas::Plugin.value_to_boolean(params[:include_priors]) + grading_period_id: params[:grading_period_id] } attachment_progress = @context.gradebook_to_csv_in_background(filename, @current_user, csv_options) diff --git a/app/models/assignment/speed_grader.rb b/app/models/assignment/speed_grader.rb index e7489269fb9..b28c3e2979d 100644 --- a/app/models/assignment/speed_grader.rb +++ b/app/models/assignment/speed_grader.rb @@ -2,8 +2,11 @@ require_relative '../assignment' class Assignment class SpeedGrader + include GradebookSettingsHelpers + def initialize(assignment, user, avatars: false, grading_role: :grader) @assignment = assignment + @course = @assignment.context @user = user @avatars = avatars @grading_role = grading_role @@ -44,9 +47,8 @@ class Assignment others.each { |s| res[:context][:rep_for_student][s.id] = rep.id } end - enrollments = - @assignment.context.apply_enrollment_visibility(enrollment_scope(gradebook_includes), - @user, nil, include: gradebook_includes) + enrollments = @course.apply_enrollment_visibility(gradebook_enrollment_scope, @user, nil, + include: gradebook_includes) is_provisional = @grading_role == :provisional_grader || @grading_role == :moderator rubric_assmnts = @assignment.visible_rubric_assessments_for(@user, :provisional_grader => is_provisional) || [] @@ -240,25 +242,5 @@ class Assignment Attachment.skip_thumbnails = nil end - private - - def gradebook_includes - @gradebook_includes ||= begin - context_id = @assignment.context.id - gb_settings = @user.preferences.fetch(:gradebook_settings, {}).fetch(context_id, {}) - - includes = [] - includes << :inactive if gb_settings.fetch('show_inactive_enrollments', "false") == "true" - includes << :completed if gb_settings.fetch('show_concluded_enrollments', "false") == "true" - includes - end - end - - def enrollment_scope(includes) - scope = @assignment.context.all_accepted_student_enrollments - scope = scope.where("enrollments.workflow_state <> 'inactive'") unless includes.include?(:inactive) - scope = scope.where("enrollments.workflow_state <> 'completed'") unless includes.include?(:completed) - scope - end end end diff --git a/app/models/course.rb b/app/models/course.rb index fcb668c4e59..eebd19c82ca 100644 --- a/app/models/course.rb +++ b/app/models/course.rb @@ -106,6 +106,7 @@ class Course < ActiveRecord::Base has_many :prior_enrollments, -> { preload(:user, :course).where(workflow_state: 'completed') }, class_name: 'Enrollment' has_many :prior_users, :through => :prior_enrollments, :source => :user has_many :students, :through => :student_enrollments, :source => :user + has_many :gradable_students, through: :gradable_student_enrollments, source: :user has_many :admin_visible_students, :through => :admin_visible_student_enrollments, :source => :user has_many :self_enrolled_students, -> { where("self_enrolled") }, through: :student_enrollments, source: :user @@ -114,6 +115,7 @@ class Course < ActiveRecord::Base has_many :participating_students, -> { where(enrollments: { type: ['StudentEnrollment', 'StudentViewEnrollment'], workflow_state: 'active' }) }, through: :enrollments, source: :user has_many :student_enrollments, -> { where("enrollments.workflow_state NOT IN ('rejected', 'completed', 'deleted', 'inactive') AND enrollments.type IN ('StudentEnrollment', 'StudentViewEnrollment')").preload(:user) }, class_name: 'Enrollment' has_many :admin_visible_student_enrollments, -> { where("enrollments.workflow_state NOT IN ('rejected', 'completed', 'deleted') AND enrollments.type IN ('StudentEnrollment', 'StudentViewEnrollment')").preload(:user) }, class_name: 'Enrollment' + has_many :gradable_student_enrollments, -> { where(enrollments: { workflow_state: ['active', 'inactive'], type: ['StudentEnrollment', 'StudentViewEnrollment'] }).preload(:user) }, class_name: 'Enrollment' has_many :all_student_enrollments, -> { where("enrollments.workflow_state<>'deleted' AND enrollments.type IN ('StudentEnrollment', 'StudentViewEnrollment')").preload(:user) }, class_name: 'Enrollment' has_many :all_accepted_student_enrollments, -> { where("enrollments.workflow_state NOT IN ('rejected', 'deleted') AND enrollments.type IN ('StudentEnrollment', 'StudentViewEnrollment')").preload(:user) }, class_name: 'Enrollment' has_many :all_real_users, :through => :all_real_enrollments, :source => :user diff --git a/lib/gradebook_exporter.rb b/lib/gradebook_exporter.rb index 645b8e54f84..dd64bc2e6b0 100644 --- a/lib/gradebook_exporter.rb +++ b/lib/gradebook_exporter.rb @@ -18,6 +18,7 @@ class GradebookExporter include GradebookTransformer + include GradebookSettingsHelpers def initialize(course, user, options = {}) @course = course @@ -26,9 +27,9 @@ class GradebookExporter end def to_csv - collection = @options[:include_priors] ? @course.all_student_enrollments : @course.admin_visible_student_enrollments - enrollments_scope = @course.apply_enrollment_visibility(collection, @user) - student_enrollments = enrollments_for_csv(enrollments_scope, @options) + enrollment_scope = @course.apply_enrollment_visibility(gradebook_enrollment_scope, @user, nil, + include: gradebook_includes) + student_enrollments = enrollments_for_csv(enrollment_scope, @options) student_section_names = {} student_enrollments.each do |enrollment| diff --git a/lib/gradebook_importer.rb b/lib/gradebook_importer.rb index 48b6007630c..5b2dc0915ab 100644 --- a/lib/gradebook_importer.rb +++ b/lib/gradebook_importer.rb @@ -170,7 +170,7 @@ class GradebookImporter # remove concluded enrollments prior_enrollment_ids = ( - @all_students.keys - @context.students.pluck(:user_id).map(&:to_i) + @all_students.keys - @context.gradable_students.pluck(:user_id).map(&:to_i) ).to_set @students.delete_if { |s| prior_enrollment_ids.include? s.id } diff --git a/lib/gradebook_settings_helpers.rb b/lib/gradebook_settings_helpers.rb new file mode 100644 index 00000000000..2481aa99487 --- /dev/null +++ b/lib/gradebook_settings_helpers.rb @@ -0,0 +1,41 @@ +# +# Copyright (C) 2016 Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +module GradebookSettingsHelpers + private + + def gradebook_includes + @gradebook_includes ||= begin + course_id = @course.id + gb_settings = @user.preferences.fetch(:gradebook_settings, {}).fetch(course_id, {}) + + includes = [] + includes << :inactive if gb_settings.fetch('show_inactive_enrollments', "false") == "true" + includes << :completed if gb_settings.fetch('show_concluded_enrollments', "false") == "true" + includes + end + end + + def gradebook_enrollment_scope(course = @course) + scope = course.all_accepted_student_enrollments + scope = scope.where("enrollments.workflow_state <> 'inactive'") unless gradebook_includes.include?(:inactive) + scope = scope.where("enrollments.workflow_state <> 'completed'") unless gradebook_includes.include?(:completed) + scope + end + +end diff --git a/spec/controllers/gradebooks_controller_spec.rb b/spec/controllers/gradebooks_controller_spec.rb index 80012cadbf9..b0c39623e15 100644 --- a/spec/controllers/gradebooks_controller_spec.rb +++ b/spec/controllers/gradebooks_controller_spec.rb @@ -454,7 +454,7 @@ describe GradebooksController do exporter = GradebookExporter.new( @course, @teacher, - { include_priors: false, include_sis_id: true } + { include_sis_id: true } ) raw_csv = exporter.to_csv expect(raw_csv).to include("Déjà vu") diff --git a/spec/lib/gradebook_exporter_spec.rb b/spec/lib/gradebook_exporter_spec.rb index cbd5c3bb759..bdcaa152bd1 100644 --- a/spec/lib/gradebook_exporter_spec.rb +++ b/spec/lib/gradebook_exporter_spec.rb @@ -26,7 +26,8 @@ describe GradebookExporter do end describe "#to_csv" do - let(:course) { @course } + let(:course) { @course } + let(:teacher) { @teacher } def exporter(opts = {}) GradebookExporter.new(course, @teacher, opts) @@ -194,6 +195,15 @@ describe GradebookExporter do student1_enrollment.deactivate student2_enrollment.deactivate + teacher.preferences[:gradebook_settings] = + { course.id => + { + 'show_inactive_enrollments' => 'true', + 'show_concluded_enrollments' => 'false' + } + } + teacher.save! + csv = exporter.to_csv rows = CSV.parse(csv, headers: true) diff --git a/spec/lib/gradebook_importer_spec.rb b/spec/lib/gradebook_importer_spec.rb index 47663060887..aa3b768fe2d 100644 --- a/spec/lib/gradebook_importer_spec.rb +++ b/spec/lib/gradebook_importer_spec.rb @@ -121,28 +121,28 @@ describe GradebookImporter do it "should Lookup with either Student Name, ID, SIS User ID, or SIS Login ID" do course_model - student_in_course(:name => "Some Name") + student_in_course(:name => "Some Name", active_all: true) @u1 = @user user_with_pseudonym(:active_all => true) @user.pseudonym.sis_user_id = "SISUSERID" @user.pseudonym.save! - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u2 = @user user_with_pseudonym(:active_all => true, :username => "something_that_has_not_been_taken") - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u3 = @user user_with_pseudonym(:active_all => true, :username => "inactive_login") @user.pseudonym.destroy - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u4 = @user user_with_pseudonym(:active_all => true, :username => "inactive_login") @user.pseudonym.destroy @user.pseudonyms.create!(:unique_id => 'active_login', :account => Account.default) - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u5 = @user uploaded_csv = CSV.generate do |csv| @@ -183,7 +183,7 @@ describe GradebookImporter do it "should Lookup by root account" do course_model - student_in_course(:name => "Some Name") + student_in_course(name: "Some Name", active_all: true) @u1 = @user account2 = Account.create! @@ -212,14 +212,14 @@ describe GradebookImporter do user_with_pseudonym(:active_all => true) @user.pseudonym.sis_user_id = "0123456" @user.pseudonym.save! - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u0 = @user # user with an sis-id that is a number user_with_pseudonym(:active_all => true, :username => "octal_ud") @user.pseudonym.destroy @user.pseudonyms.create!(:unique_id => '0231163', :account => Account.default) - student_in_course(:user => @user) + student_in_course(user: @user, active_all: true) @u1 = @user uploaded_csv = CSV.generate do |csv| @@ -327,7 +327,7 @@ describe GradebookImporter do end it "should parse new and existing users" do - course_with_student + course_with_student(active_all: true) @student1 = @student e = student_in_course e.update_attribute :workflow_state, 'completed' @@ -359,7 +359,7 @@ describe GradebookImporter do end it "should include assignments that the grade changed for an existing user" do - course_with_student + course_with_student(active_all: true) @assignment1 = @course.assignments.create!(:name => 'Assignment 1', :points_possible => 10) @assignment1.grade_student(@student, :grade => 8) importer_with_rows( diff --git a/spec/models/course_spec.rb b/spec/models/course_spec.rb index 9716f9ef0d2..875414d33b0 100644 --- a/spec/models/course_spec.rb +++ b/spec/models/course_spec.rb @@ -1275,7 +1275,16 @@ describe Course, "gradebook_to_csv" do e.update_attribute :workflow_state, 'completed' expect(GradebookExporter.new(@course, @teacher).to_csv).not_to include @student.name - expect(GradebookExporter.new(@course, @teacher, include_priors: true).to_csv).to include @student.name + + @teacher.preferences[:gradebook_settings] = + { @course.id => + { + 'show_inactive_enrollments' => 'false', + 'show_concluded_enrollments' => 'true' + } + } + @teacher.save! + expect(GradebookExporter.new(@course, @teacher).to_csv).to include @student.name end context "accumulated points" do