From d8f9ee617410bbfa6e3270f43ffb0ad7df3bc323 Mon Sep 17 00:00:00 2001 From: Brian Palmer Date: Mon, 7 Feb 2011 17:33:03 -0700 Subject: [PATCH] revert an explicit permission to the proper default In /accounts/*/role_overrides The value was being saved correctly, but the UI was incorrect and super confusing. It'd always show a bold "explicit" green checkmark, rather than the semi-transparent check/cross depending on the actual default. refs #3711 Change-Id: Ide0a0603b6c820ea0ec94646c4327239d980b09c Reviewed-on: https://gerrit.instructure.com/2194 Tested-by: Hudson Reviewed-by: Brian Whitmer --- app/models/role_override.rb | 40 ++++++++++------ app/views/role_overrides/index.html.erb | 2 +- spec/models/role_override_spec.rb | 62 +++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 14 deletions(-) create mode 100644 spec/models/role_override_spec.rb diff --git a/app/models/role_override.rb b/app/models/role_override.rb index 2a9adeb18fc..d9937456818 100644 --- a/app/models/role_override.rb +++ b/app/models/role_override.rb @@ -479,13 +479,16 @@ class RoleOverride < ActiveRecord::Base def self.css_class_for(context, permission, enrollment_type) generated_permission = self.permission_for(context, permission, enrollment_type) - css = "" + css = [] if generated_permission[:readonly] - css += "six-checkbox-disabled-#{generated_permission[:enabled] ? 'checked' : 'unchecked' }" + css << "six-checkbox-disabled-#{generated_permission[:enabled] ? 'checked' : 'unchecked' }" else - css += "six-checkbox#{generated_permission[:explicit] ? '' : '-default' }-#{generated_permission[:enabled] ? 'checked' : 'unchecked' }" + if generated_permission[:explicit] + css << "six-checkbox-default-#{generated_permission[:prior_default] ? 'checked' : 'unchecked'}" + end + css << "six-checkbox#{generated_permission[:explicit] ? '' : '-default' }-#{generated_permission[:enabled] ? 'checked' : 'unchecked' }" end - css + css.join(' ') end def self.readonly_for(context, permission, enrollment_type) @@ -550,16 +553,17 @@ class RoleOverride < ActiveRecord::Base overrides = @@role_override_chain[permissionless_key] unless overrides contexts = [] - while context - contexts << context - if context.respond_to?(:course) - context = context.course - elsif context.respond_to?(:account) - context = context.account - elsif context.respond_to?(:parent_account) - context = context.parent_account + context_walk = context + while context_walk + contexts << context_walk + if context_walk.respond_to?(:course) + context_walk = context_walk.course + elsif context_walk.respond_to?(:account) + context_walk = context_walk.account + elsif context_walk.respond_to?(:parent_account) + context_walk = context_walk.parent_account else - context = nil + context_walk = nil end end context_codes = contexts.reverse.map(&:asset_string) @@ -579,6 +583,16 @@ class RoleOverride < ActiveRecord::Base if !generated_permission[:locked] && context_override = override unless context_override.enabled.nil? + if context_override.context == context + # if the explicit override is for the target context, the prior default + # is the parent context's value + generated_permission[:prior_default] = generated_permission[:enabled] + else + # otherwise, the prior default is the same as the new override, + # since changing it in the target context will create a new + # override + generated_permission[:prior_default] = context_override.enabled? + end generated_permission.merge!({ :enabled => context_override.enabled?, :explicit => !context_override.enabled.nil? diff --git a/app/views/role_overrides/index.html.erb b/app/views/role_overrides/index.html.erb index 8d9552cb79e..ddabff7a436 100644 --- a/app/views/role_overrides/index.html.erb +++ b/app/views/role_overrides/index.html.erb @@ -168,7 +168,7 @@
- Cancel + Cancel
<% end -%> diff --git a/spec/models/role_override_spec.rb b/spec/models/role_override_spec.rb new file mode 100644 index 00000000000..40f0dded049 --- /dev/null +++ b/spec/models/role_override_spec.rb @@ -0,0 +1,62 @@ +# +# Copyright (C) 2011 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 . +# + +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb') + +describe RoleOverride do + it "should retain the prior permission when it encounters the first explicit override" do + course_model + RoleOverride.create!(:context => @course, :permission => 'moderate_forum', + :enrollment_type => "TeacherEnrollment", :enabled => false) + permissions = RoleOverride.permission_for(@course.account, :moderate_forum, "TeacherEnrollment") + permissions[:enabled].should == true + permissions.key?(:prior_default).should == false + permissions[:explicit].should == false + + permissions = RoleOverride.permission_for(@course, :moderate_forum, "TeacherEnrollment") + permissions[:enabled].should == false + permissions[:prior_default].should == true + permissions[:explicit].should == true + end + + it "should use the immediately parent context as the prior permission when there are multiple explicit levels" do + a1 = account_model + a2 = account_model(:parent_account => a1) + a3 = account_model(:parent_account => a2) + + RoleOverride.create!(:context => a1, :permission => 'moderate_forum', + :enrollment_type => "TeacherEnrollment", :enabled => false) + RoleOverride.create!(:context => a2, :permission => 'moderate_forum', + :enrollment_type => "TeacherEnrollment", :enabled => true) + + permissions = RoleOverride.permission_for(a1, :moderate_forum, "TeacherEnrollment") + permissions[:enabled].should == false + permissions[:prior_default].should == true + permissions[:explicit].should == true + + permissions = RoleOverride.permission_for(a2, :moderate_forum, "TeacherEnrollment") + permissions[:enabled].should == true + permissions[:prior_default].should == false + permissions[:explicit].should == true + + permissions = RoleOverride.permission_for(a3, :moderate_forum, "TeacherEnrollment") + permissions[:enabled].should == true + permissions[:prior_default].should == true + permissions[:explicit].should == true + end +end