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

DA - more refactoring 

- cleans up assignment model
- eliminates unnecessary feature flag checks
- eliminates unnecessary teacher permission checks
- moves duplicated methods from assignment and quiz
  to new module

Change-Id: I8d1e8f99bf5c8f6a63bd670b432aeef7e8f0e86b
Reviewed-on: https://gerrit.instructure.com/41654
Reviewed-by: Cameron Sutter <csutter@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Sean Lewis <slewis@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
This commit is contained in:
Michael Nomitch 2014-09-23 15:23:30 -05:00 committed by Mike Nomitch
parent 8c1b04e912
commit 4e791b941b
17 changed files with 88 additions and 123 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/assignments_api_controller.rb
View File

@ -527,7 +527,7 @@ class AssignmentsApiController < ApplicationController
end
if da_enabled = @context.feature_enabled?(:differentiated_assignments)
scope = AssignmentStudentVisibility.filter_for_differentiated_assignments(scope, @current_user, @context) do |scope, user_ids|
scope = DifferentiableAssignment.filter(scope, @current_user, @context) do |scope, user_ids|
scope.visible_to_student_in_course_with_da(user_ids, @context.id)
end
end

4
app/controllers/assignments_controller.rb
View File

@ -115,7 +115,9 @@ class AssignmentsController < ApplicationController
end
if authorized_action(@assignment, @current_user, :read)
if @context.feature_enabled?(:differentiated_assignments) && @current_user && @assignment && !@assignment.visible_to_user?(@current_user)
if (da_on = @context.feature_enabled?(:differentiated_assignments)) &&
@current_user && @assignment &&
!@assignment.visible_to_user?(@current_user, differentiated_assignments: da_on)
respond_to do |format|
flash[:error] = t 'notices.assignment_not_available', "The assignment you requested is not available to your course section."
format.html { redirect_to named_context_url(@context, :context_assignments_url) }

4
app/controllers/discussion_topics_api_controller.rb
View File

@ -606,8 +606,8 @@ class DiscussionTopicsApiController < ApplicationController
end
def check_differentiated_assignments
return true unless @context.feature_enabled?(:differentiated_assignments)
return render_unauthorized_action if @topic.for_assignment? && !@topic.assignment.visible_to_user?(@current_user)
return true unless da_on = @context.feature_enabled?(:differentiated_assignments)
return render_unauthorized_action if @topic.for_assignment? && !@topic.assignment.visible_to_user?(@current_user, differentiated_assignments: da_on)
end
# the result of several state change functions are the following:

2
app/controllers/discussion_topics_controller.rb
View File

@ -276,7 +276,7 @@ class DiscussionTopicsController < ApplicationController
end
if @context.feature_enabled?(:differentiated_assignments)
scope = AssignmentStudentVisibility.filter_for_differentiated_assignments(scope, @current_user, @context) do |scope, user_ids|
scope = DifferentiableAssignment.filter(scope, @current_user, @context) do |scope, user_ids|
scope.visible_to_students_with_da_enabled(user_ids)
end
end

8
app/controllers/quizzes/quizzes_api_controller.rb
View File

@ -292,8 +292,8 @@ class Quizzes::QuizzesApiController < ApplicationController
api_route = api_v1_course_quizzes_url(@context)
scope = Quizzes::Quiz.search_by_attribute(@context.quizzes.active, :title, params[:search_term])
if @context.feature_enabled?(:differentiated_assignments) && !@context.grants_any_right?(@current_user, :read_as_admin, :manage_grades, :manage_assignments)
scope = AssignmentStudentVisibility.filter_for_differentiated_assignments(scope, @current_user, @context) do |scope, user_ids|
if @context.feature_enabled?(:differentiated_assignments)
scope = DifferentiableAssignment.filter(scope, @current_user, @context) do |scope, user_ids|
scope.visible_to_students_in_course_with_da(user_ids, @context.id)
end
end
@ -520,7 +520,7 @@ class Quizzes::QuizzesApiController < ApplicationController
end
def check_differentiated_assignments
return true unless @context.feature_enabled?(:differentiated_assignments)
return render_unauthorized_action if @current_user && !@quiz.visible_to_user?(@current_user)
return true unless da_on = @context.feature_enabled?(:differentiated_assignments)
return render_unauthorized_action if @current_user && !@quiz.visible_to_user?(@current_user, differentiated_assignments: da_on)
end
end

2
app/controllers/quizzes/quizzes_controller.rb
View File

@ -57,7 +57,7 @@ class Quizzes::QuizzesController < ApplicationController
scope = scope.available unless can_manage
if @context.feature_enabled?(:differentiated_assignments) && !@context.grants_any_right?(@current_user, :read_as_admin, :manage_grades, :manage_assignments)
scope = AssignmentStudentVisibility.filter_for_differentiated_assignments(scope, @current_user, @context) do |scope, user_ids|
scope = DifferentiableAssignment.filter(scope, @current_user, @context) do |scope, user_ids|
scope.visible_to_students_in_course_with_da(user_ids,@context.id)
end
end

4
app/controllers/submissions_api_controller.rb
View File

@ -260,9 +260,9 @@ class SubmissionsApiController < ApplicationController
bulk_load_attachments_and_previews([@submission])
if authorized_action(@submission, @current_user, :read)
if !@context.feature_enabled?(:differentiated_assignments) ||
if !(da_on = @context.feature_enabled?(:differentiated_assignments)) ||
@context.grants_any_right?(@current_user, :read_as_admin, :manage_grades, :manage_assignments) ||
@submission.assignment_visible_to_user?(@current_user)
@submission.assignment_visible_to_user?(@current_user, differentiated_assignments: da_on)
includes = Array(params[:include])
render :json => submission_json(@submission, @assignment, @current_user, session, @context, includes)
else

46
app/models/assignment.rb
View File

@ -584,37 +584,9 @@ class Assignment < ActiveRecord::Base
overridden_users
end
def students_with_visibility
if self.differentiated_assignments_applies?
context.all_students.able_to_see_assignment_in_course_with_da(self.id, context.id)
else
context.all_students
end
end
def visible_to_user?(user)
return true if context.grants_any_right?(user, :read_as_admin, :manage_grades, :manage_assignments)
visible_to_observer?(user) || students_with_visibility.pluck(:id).include?(user.id)
end
def visible_to_observer?(user)
return false unless context.user_has_been_observer?(user)
return true if !differentiated_assignments_applies? && self.grants_right?(user, :read)
visible_student_ids = students_with_visibility.pluck(:id)
observed_students_ids = ObserverEnrollment.observed_student_ids(context, user)
# observers can be students as well, so their own assignments must be included
if user.student_enrollments.where(course_id: self.context_id).any?
observed_student_ids << user.id
end
# if the observer doesn't have students, show them published assignments
has_visible_students = (observed_students_ids & visible_student_ids).any?
return has_visible_students if observed_students_ids.any?
self.published?
def students_with_visibility(scope=context.all_students)
return scope unless self.differentiated_assignments_applies?
scope.able_to_see_assignment_in_course_with_da(self.id, context.id)
end
attr_accessor :saved_by
@ -1432,10 +1404,7 @@ class Assignment < ActiveRecord::Base
@visible_students_for_speed_grader ||= {}
@visible_students_for_speed_grader[user.global_id] ||= (
student_scope = user ? context.students_visible_to(user) : context.participating_students
if self.differentiated_assignments_applies?
student_scope = student_scope.able_to_see_assignment_in_course_with_da(self.id, context.id)
end
student_scope
students_with_visibility(student_scope)
).order_by_sortable_name.uniq.to_a
end
@ -1502,12 +1471,7 @@ class Assignment < ActiveRecord::Base
return [] unless self.peer_review_count && self.peer_review_count > 0
submissions = self.submissions.having_submission.include_assessment_requests
student_ids = if self.differentiated_assignments_applies?
context.students.able_to_see_assignment_in_course_with_da(self.id, self.context.id).pluck(:id)
else
context.student_ids
end
student_ids = students_with_visibility(context.students).pluck(:id)
submissions = submissions.select{|s| student_ids.include?(s.user_id) }
submission_ids = Set.new(submissions) { |s| s.id }

10
app/models/assignment_student_visibility.rb
View File

@ -46,16 +46,6 @@ class AssignmentStudentVisibility < ActiveRecord::Base
self.where(opts).pluck(:assignment_id)
end
def self.filter_for_differentiated_assignments(collection, user, context, opts={}, &filter)
return collection if !user || context.grants_any_right?(user, :manage_content, :read_as_admin, :manage_grades, :manage_assignments)
return filter.call(collection, [user.id]) unless context.user_has_been_observer?(user)
observed_student_ids = opts[:observed_student_ids] || ObserverEnrollment.observed_student_ids(context, user)
user_ids = [user.id].concat(observed_student_ids)
# if observer is following no students, do not filter based on differentiated assignments
observed_student_ids.any? ? filter.call(collection, user_ids) : collection
end
# readonly? is not checked in destroy though
before_destroy { |record| raise ActiveRecord::ReadOnlyRecord }
end

3
app/models/context_module.rb
View File

@ -303,6 +303,7 @@ class ContextModule < ActiveRecord::Base
end
if !self.grants_right?(user, :update) && self.context.feature_enabled?(:differentiated_assignments) && user
opts[:is_teacher]= false
tags = filter_tags_for_da(tags, user, opts)
end
@ -330,7 +331,7 @@ class ContextModule < ActiveRecord::Base
filter = opts[:tags_loaded] ? array_filter : scope_filter
tags = AssignmentStudentVisibility.filter_for_differentiated_assignments(tags, user, self.context, opts) do |tags, user_ids|
tags = DifferentiableAssignment.filter(tags, user, self.context, opts) do |tags, user_ids|
filter.call(tags, user_ids, self.context_id, opts)
end

4
app/models/course.rb
View File

@ -306,14 +306,14 @@ class Course < ActiveRecord::Base
end
def module_items_visible_to(user)
if self.grants_right?(user, :manage_content)
if user_is_teacher = self.grants_right?(user, :manage_content)
tags = self.context_module_tags.not_deleted.joins(:context_module).where("context_modules.workflow_state <> 'deleted'")
else
tags = self.context_module_tags.active.joins(:context_module).where(:context_modules => {:workflow_state => 'active'})
end
if self.feature_enabled?(:differentiated_assignments)
tags = AssignmentStudentVisibility.filter_for_differentiated_assignments(tags, user, self) do |tags, user_ids|
tags = DifferentiableAssignment.filter(tags, user, self, is_teacher: user_is_teacher) do |tags, user_ids|
tags.visible_to_students_with_da_enabled(user_ids)
end
end

9
app/models/quizzes/quiz.rb
View File

@ -1306,15 +1306,6 @@ class Quizzes::Quiz < ActiveRecord::Base
feature_enabled?(:draft_state) ? published? : workflow_state == 'available'
end
def visible_to_user?(user)
return true unless self.context.feature_enabled?(:differentiated_assignments)
visible_quizzes = AssignmentStudentVisibility.filter_for_differentiated_assignments([self],user,self.context) do |quiz_id, user_ids|
# note: quiz_id is an array
Quizzes::QuizStudentVisibility.where(user_id: user_ids, quiz_id: quiz_id)
end
visible_quizzes.any?
end
delegate :feature_enabled?, to: :context
# The IP filters available for this Quiz, which is an aggregate of the Quiz's

4
app/models/submission.rb
View File

@ -784,8 +784,8 @@ class Submission < ActiveRecord::Base
scope :for_user, lambda { |user| where(:user_id => user) }
scope :needing_screenshot, -> { where("submissions.submission_type='online_url' AND submissions.attachment_id IS NULL AND submissions.process_attempts<3").order(:updated_at) }
def assignment_visible_to_user?(user)
assignment.visible_to_user?(user)
def assignment_visible_to_user?(user, opts={})
assignment.visible_to_user?(user, opts)
end
def needs_regrading?

24
app/models/user.rb
View File

@ -1351,27 +1351,13 @@ class User < ActiveRecord::Base
def assignments_visibile_in_course(course)
return course.active_assignments if course.grants_any_right?(self, :read_as_admin, :manage_grades, :manage_assignments)
published_visible_assignments = course.active_assignments.published
if course.feature_enabled?(:differentiated_assignments)
return assignments_visible_as_observer(course) if course.user_has_been_observer?(self)
course.active_assignments.published.visible_to_student_in_course_with_da(self.id, course.id)
else
course.active_assignments.published
end
end
def assignments_visible_as_observer(course)
return [] unless course.user_has_been_observer?(self)
observed_student_ids = ObserverEnrollment.observed_student_ids(course, self)
if self.student_enrollments.any?
observed_student_ids.concat self.student_enrollments.pluck(:user_id)
end
if course.feature_enabled?(:differentiated_assignments) && observed_student_ids.any?
course.active_assignments.visible_to_student_in_course_with_da(observed_student_ids, course.id)
else
course.active_assignments
published_visible_assignments = DifferentiableAssignment.filter(published_visible_assignments,self,course, is_teacher: false) do |assignment_scope,user_ids|
assignment_scope.visible_to_student_in_course_with_da(user_ids, course.id)
end
end
published_visible_assignments
end
def assignments_needing_submitting(opts={})

13
lib/dates_overridable.rb
View File

@ -2,6 +2,7 @@ module DatesOverridable
attr_accessor :applied_overrides, :overridden_for_user, :overridden,
:has_no_overrides
attr_writer :without_overrides
include DifferentiableAssignment
def self.included(base)
base.has_many :assignment_overrides, :dependent => :destroy
@ -70,18 +71,6 @@ module DatesOverridable
all_dates
end
def differentiated_assignments_applies?
return false if !context.feature_enabled?(:differentiated_assignments)
if self.is_a?(Assignment) || Quizzes::Quiz.class_names.include?(self.class_name)
self.only_visible_to_overrides
elsif self.assignment
self.assignment.only_visible_to_overrides
else
false
end
end
def all_dates_visible_to(user)
if user.nil?
all_due_dates

48
lib/differentiable_assignment.rb Normal file
View File

@ -0,0 +1,48 @@
module DifferentiableAssignment
def differentiated_assignments_applies?
return false if !context.feature_enabled?(:differentiated_assignments)
if self.is_a?(Assignment) || Quizzes::Quiz.class_names.include?(self.class_name)
self.only_visible_to_overrides
elsif self.assignment
self.assignment.only_visible_to_overrides
else
false
end
end
def visible_to_user?(user, opts={})
# slightly redundant conditional, but avoiding unnecessary lookups
return true if opts[:differentiated_assignments] == false ||
(opts[:differentiated_assignments] == true && !self.only_visible_to_overrides) ||
!self.differentiated_assignments_applies? #checks if DA enabled on course and then only_visible_to_overrides
# will add users if observer and only filter based on DA when necessary (not for teachers/some observers)
visible_instances = DifferentiableAssignment.filter([self],user,self.context) do |_, user_ids|
conditions = {user_id: user_ids}
conditions[column_name] = self.id
visibility_view.where(conditions)
end
visible_instances.any?
end
def visibility_view
self.is_a?(Assignment) ? AssignmentStudentVisibility : Quizzes::QuizStudentVisibility
end
def column_name
self.is_a?(Assignment) ? :assignment_id : :quiz_id
end
# will not filter the collection for teachers, will for non-observer students
# will filter for observers with observed students but not for observers without observed students
def self.filter(collection, user, context, opts={}, &filter_block)
return collection if !user || (opts[:is_teacher] != false && context.grants_any_right?(user, :manage_content, :read_as_admin, :manage_grades, :manage_assignments))
return filter_block.call(collection, [user.id]) unless context.user_has_been_observer?(user)
observed_student_ids = opts[:observed_student_ids] || ObserverEnrollment.observed_student_ids(context, user)
user_ids = [user.id].concat(observed_student_ids)
# if observer is following no students, do not filter based on differentiated assignments
observed_student_ids.any? ? filter_block.call(collection, user_ids) : collection
end
end

24
spec/models/assignment_spec.rb
View File

@ -277,7 +277,7 @@ describe Assignment do
end
end
describe "visible_to_observer?" do
describe "observers" do
before :once do
setup_DA
@course.enable_feature!(:draft_state)
@ -285,19 +285,13 @@ describe Assignment do
@observer = User.create
end
context "user_is_not_an_observer" do
it "should not be visible" do
@assignment.visible_to_observer?(@student1).should be_false
end
end
context "differentiated_assignment on" do
context "observing only a section (with or without an override)" do
before do
@observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment', :section => @section2, :enrollment_state => 'active')
end
it "should be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
@ -307,7 +301,7 @@ describe Assignment do
@observer_enrollment.update_attribute(:associated_user_id, @student1.id)
end
it "should be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
@ -317,7 +311,7 @@ describe Assignment do
@observer_enrollment.update_attribute(:associated_user_id, @student2.id)
end
it "should not be visible" do
@assignment.visible_to_observer?(@observer).should be_false
@assignment.visible_to_user?(@observer).should be_false
end
end
@ -327,7 +321,7 @@ describe Assignment do
@course.enroll_user(@observer, "ObserverEnrollment", {:allow_multiple_enrollments => true, :associated_user_id => @student2.id})
end
it "should not be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
@ -337,7 +331,7 @@ describe Assignment do
@course.enroll_user(@observer, "ObserverEnrollment", {:allow_multiple_enrollments => true, :associated_user_id => @student2.id})
end
it "should not be visible" do
@assignment.visible_to_observer?(@observer).should be_false
@assignment.visible_to_user?(@observer).should be_false
end
end
end
@ -349,7 +343,7 @@ describe Assignment do
@observer_enrollment = @course.enroll_user(@observer, 'ObserverEnrollment', :section => @section, :enrollment_state => 'active')
end
it "should be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
@ -359,7 +353,7 @@ describe Assignment do
@observer_enrollment.update_attribute(:associated_user_id, @student1.id)
end
it "should be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
@ -369,7 +363,7 @@ describe Assignment do
@observer_enrollment.update_attribute(:associated_user_id, @student2.id)
end
it "should be visible" do
@assignment.visible_to_observer?(@observer).should be_true
@assignment.visible_to_user?(@observer).should be_true
end
end
end