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

remove "Allowed" state for RootAccount features on root accounts 

the "Allowed" state means a feature is off in an account, but
sub-accounts or courses below it are allowed to enable it.

the 'allowed' state does not, however, make a lot of sense for
features that apply to RootAccount. since the feature cannot
be controlled in sub-accounts or courses, the 'allowed' state
is equivalent to 'off' here.

so to make this less confusing, remove the "allowed" state
for RootAccount features.

(specifically, lock the transition in the API, and make the
UI hide buttons for locked transitions that don't have
messages to display when the user tries to perform them)

test plan:
 - set the Use New Styles feature to "Allowed" in Site Admin
   account settings
 - in a root account settings page, ensure the 'Allowed'
   option is not selectable for this feature
 - ensure that the API reports the new_styles feature is "off"
   and its "allowed" transition is locked
   i.e., GET /api/v1/accounts/1/features/flags/new_styles
   includes state: 'off' and transitions: {allowed: {locked: true}}
 - ensure the API refuses to set the new_styles feature to "allowed"
   in the root account
   i.e., PUT /api/v1/accounts/1/features/flags/new_styles?state=allowed
   should return a 403 error and not change the state
 - regression test: in a sub-account, ensure the 'Use New Styles'
   feature does not appear
 - regression test: verify that when a (non-site-admin)
   root account admin attempts to disable Draft State in
   an account, the Off button is still there, and a message appears
   when you click it, and the feature remains enabled

fixes CNVS-13220

Change-Id: I4d41076c10696b02d0c482a778d2555714487f17
Reviewed-on: https://gerrit.instructure.com/35473
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Bracken Mosbacker <bracken@instructure.com>
QA-Review: Clare Strong <clare@instructure.com>
Product-Review: Bracken Mosbacker <bracken@instructure.com>
Product-Review: Hilary Scharton <hilary@instructure.com>
This commit is contained in:
Jeremy Stanley 2014-05-27 16:11:56 -06:00
parent bf724dba8b
commit cc4f0925ef
5 changed files with 91 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
app/coffeescripts/models/FeatureFlag.coffee
View File

@ -60,10 +60,17 @@ define ['jquery', 'underscore', 'Backbone'], ($, _, Backbone) ->
transitions: ->
@get('feature_flag').transitions
transitionLocked: (action) ->
settings = @transitions()[action]
# the button remains enabled if there's an associated message
return settings?.locked && !settings.message
toJSON: ->
_.extend(super, isAllowed: @isAllowed(), isHidden: @isHidden(),
isOff: @isOff(true), isOn: @isOn(), isSiteAdmin: @isSiteAdmin(),
currentContextIsAccount: @isContext('account'))
currentContextIsAccount: @isContext('account'),
disableOn: @transitionLocked('on'), disableAllow: @transitionLocked('allowed'),
disableOff: @transitionLocked('off'))
parse: (json) ->
_.extend(json, @attributes)

38
app/views/jst/feature_flags/featureFlag.handlebars
View File

@ -38,27 +38,33 @@
{{else}}
<div class="btn-group" role="radiogroup">
{{#unless isSiteAdmin}}
<button type="button"
class="btn {{#if isOn}}active{{/if}}"
data-action="on"
data-name="{{id}}"
role="radio"
aria-checked="{{#if isOn}}true{{else}}false{{/if}}">{{#t "on"}}On{{/t}}</button>
{{#unless disableOn}}
<button type="button"
class="btn {{#if isOn}}active{{/if}}"
data-action="on"
data-name="{{id}}"
role="radio"
aria-checked="{{#if isOn}}true{{else}}false{{/if}}">{{#t "on"}}On{{/t}}</button>
{{/unless}}
{{/unless}}
{{#if currentContextIsAccount}}
{{#unless disableAllow}}
<button type="button"
class="btn {{#if isAllowed}}active{{/if}}"
data-action="allowed"
data-name="{{id}}"
role="radio"
aria-checked="{{#if isAllowed}}true{{else}}false{{/if}}">{{#t "allow"}}Allow{{/t}}</button>
{{/unless}}
{{/if}}
{{#unless disableOff}}
<button type="button"
class="btn {{#if isAllowed}}active{{/if}}"
data-action="allowed"
class="btn {{#if isOff}}active{{/if}}"
data-action="off"
data-name="{{id}}"
role="radio"
aria-checked="{{#if isAllowed}}true{{else}}false{{/if}}">{{#t "allow"}}Allow{{/t}}</button>
{{/if}}
<button type="button"
class="btn {{#if isOff}}active{{/if}}"
data-action="off"
data-name="{{id}}"
role="radio"
aria-checked="{{#if isOff}}true{{else}}false{{/if}}">{{#t "off"}}Off{{/t}}</button>
aria-checked="{{#if isOff}}true{{else}}false{{/if}}">{{#t "off"}}Off{{/t}}</button>
{{/unless}}
</div>
{{/if}}
</div>

22
lib/feature.rb
View File

@ -27,6 +27,8 @@ class Feature
next if key == :state && !%w(hidden off allowed on).include?(val)
instance_variable_set "@#{key}", val
end
# for RootAccount features, "allowed" state is redundant; show "off" instead
@root_opt_in = true if @applies_to == 'RootAccount'
end
def default?
@ -248,23 +250,29 @@ END
definitions.values.select{ |fd| applicable_types.include?(fd.applies_to) }
end
def self.default_transitions(context, orig_state)
def default_transitions(context, orig_state)
valid_states = %w(off on)
valid_states << 'allowed' if context.is_a?(Account)
(valid_states - [orig_state]).inject({}) do |transitions, state|
transitions[state] = { 'locked' => false }
transitions[state] = { 'locked' => (state == 'allowed' && @applies_to == 'RootAccount' &&
context.is_a?(Account) && context.root_account? && !context.site_admin?) }
transitions
end
end
def self.transitions(feature, user, context, orig_state)
h = Feature.default_transitions(context, orig_state)
fd = definitions[feature.to_s]
if fd.custom_transition_proc.is_a?(Proc)
fd.custom_transition_proc.call(user, context, orig_state, h)
def transitions(user, context, orig_state)
h = default_transitions(context, orig_state)
if @custom_transition_proc.is_a?(Proc)
@custom_transition_proc.call(user, context, orig_state, h)
end
h
end
def self.transitions(feature_name, user, context, orig_state)
fd = definitions[feature_name.to_s]
return nil unless fd
fd.transitions(user, context, orig_state)
end
end
# load feature definitions

19
spec/apis/v1/feature_flags_api_spec.rb
View File

@ -27,7 +27,7 @@ describe "Feature Flags API", type: :request do
before do
Feature.stubs(:definitions).returns({
'root_account_feature' => Feature.new(feature: 'root_account_feature', applies_to: 'RootAccount', state: 'off'),
'root_account_feature' => Feature.new(feature: 'root_account_feature', applies_to: 'RootAccount', state: 'allowed'),
'account_feature' => Feature.new(feature: 'account_feature', applies_to: 'Account', state: 'on', display_name: lambda { "Account Feature FRD" }, description: lambda { "FRD!!" }, beta: true),
'course_feature' => Feature.new(feature: 'course_feature', applies_to: 'Course', state: 'allowed', development: true, release_notes_url: 'http://example.com', display_name: "not localized", description: "srsly"),
'user_feature' => Feature.new(feature: 'user_feature', applies_to: 'User', state: 'allowed'),
@ -74,11 +74,15 @@ describe "Feature Flags API", type: :request do
"transitions"=>{"allowed"=>{"locked"=>false}, "off"=>{"locked"=>false}}}},
{"feature"=>"root_account_feature",
"applies_to"=>"RootAccount",
"root_opt_in"=>true,
"feature_flag"=>
{"feature"=>"root_account_feature",
{"context_id"=>t_root_account.id,
"context_type"=>"Account",
"locking_account_id"=>nil,
"feature"=>"root_account_feature",
"state"=>"off",
"locked"=>true,
"transitions"=>{"allowed"=>{"locked"=>false}, "on"=>{"locked"=>false}}}},
"locked"=>false,
"transitions"=>{"allowed"=>{"locked"=>true}, "on"=>{"locked"=>false}}}},
{"feature"=>"root_opt_in_feature",
"applies_to"=>"Course",
"root_opt_in"=>true,
@ -268,6 +272,13 @@ describe "Feature Flags API", type: :request do
flag.should_not be_new_record
end
it "should disallow 'allowed' setting for RootAccount features on (non-site-admin) root accounts" do
t_root_account.disable_feature! :root_account_feature
api_call_as_user(t_root_admin, :put, "/api/v1/accounts/#{t_root_account.id}/features/flags/root_account_feature?state=allowed",
{ controller: 'feature_flags', action: 'update', format: 'json', account_id: t_root_account.to_param, feature: 'root_account_feature', state: 'allowed' },
{}, {}, { expected_status: 403 })
end
describe "locking_account_id" do
it "should require admin rights in the locking account to lock a flag" do
api_call_as_user(t_teacher, :put, "/api/v1/courses/#{t_course.id}/features/flags/course_feature?state=on&locking_account_id=#{t_root_account.id}",

40
spec/lib/feature_spec.rb
View File

@ -107,23 +107,45 @@ describe Feature do
end
end
describe "RootAccount feature" do
it "should imply root_opt_in" do
Feature.definitions['RA'].root_opt_in.should be_true
end
end
describe "default_transitions" do
it "should enumerate RootAccount transitions" do
fd = Feature.definitions['RA']
fd.default_transitions(t_site_admin, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_site_admin, 'on').should eql({'allowed'=>{'locked'=>false},'off'=>{'locked'=>false}})
fd.default_transitions(t_site_admin, 'off').should eql({'allowed'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_root_account, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_root_account, 'on').should eql({'allowed'=>{'locked'=>true},'off'=>{'locked'=>false}})
fd.default_transitions(t_root_account, 'off').should eql({'allowed'=>{'locked'=>true},'on'=>{'locked'=>false}})
end
it "should enumerate Account transitions" do
Feature.default_transitions(t_root_account, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
Feature.default_transitions(t_root_account, 'on').should eql({'allowed'=>{'locked'=>false},'off'=>{'locked'=>false}})
Feature.default_transitions(t_root_account, 'off').should eql({'allowed'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd = Feature.definitions['A']
fd.default_transitions(t_root_account, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_root_account, 'on').should eql({'allowed'=>{'locked'=>false},'off'=>{'locked'=>false}})
fd.default_transitions(t_root_account, 'off').should eql({'allowed'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_sub_account, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_sub_account, 'on').should eql({'allowed'=>{'locked'=>false},'off'=>{'locked'=>false}})
fd.default_transitions(t_sub_account, 'off').should eql({'allowed'=>{'locked'=>false},'on'=>{'locked'=>false}})
end
it "should enumerate Course transitions" do
Feature.default_transitions(t_course, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
Feature.default_transitions(t_course, 'on').should eql({'off'=>{'locked'=>false}})
Feature.default_transitions(t_course, 'off').should eql({'on'=>{'locked'=>false}})
fd = Feature.definitions['C']
fd.default_transitions(t_course, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_course, 'on').should eql({'off'=>{'locked'=>false}})
fd.default_transitions(t_course, 'off').should eql({'on'=>{'locked'=>false}})
end
it "should enumerate User transitions" do
Feature.default_transitions(t_user, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
Feature.default_transitions(t_user, 'on').should eql({'off'=>{'locked'=>false}})
Feature.default_transitions(t_user, 'off').should eql({'on'=>{'locked'=>false}})
fd = Feature.definitions['U']
fd.default_transitions(t_user, 'allowed').should eql({'off'=>{'locked'=>false},'on'=>{'locked'=>false}})
fd.default_transitions(t_user, 'on').should eql({'off'=>{'locked'=>false}})
fd.default_transitions(t_user, 'off').should eql({'on'=>{'locked'=>false}})
end
end