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

customize feature flag state transitions for draft state 

test plan:
0. render and consult the API documentation on feature flags,
   specifically looking at the 'transitions' member that
   was added to FeatureFlag
1. enable draft state in a course
2. use the API to show the draft state feature flag for the
   course (GET /courses/X/features/flags/draft_state)
   - verify that transitions['off'] contains a warning
     message about what happens to unpublished data
3. allow draft state in an account
4. as a non-site-admin user, use the API to show the
   draft state feature flag for the account
   (GET /accounts/X/features/flags/draft_state)
   - verify that transitions['on']['locked'] is false
   - verify that transitions['off']['locked'] is true
     and is accompanied by an explanatory message
5. attempt to turn the feature off via the API
   (still as a non-site-admin user)
   - you should get a 403 Forbidden error
6. repeat step 4 as a site-admin user
   - neither transition should be 'locked', but the
     message should still accompany 'off'
7. repeat step 5 as a site-admin user
   - it should succeed

fixes CNVS-10355
fixes CNVS-10356

Change-Id: I36b0d387ad09f98cdf3d4de9e4e351bfd6aa8c58
Reviewed-on: https://gerrit.instructure.com/28541
Tested-by: Jenkins <jenkins@instructure.com>
Reviewed-by: Bracken Mosbacker <bracken@instructure.com>
Product-Review: Bracken Mosbacker <bracken@instructure.com>
QA-Review: Nathan Rogowski <nathan@instructure.com>
This commit is contained in:
Jeremy Stanley 2014-01-14 13:20:02 -07:00
parent ce6bc221d8
commit 4e1641b833
7 changed files with 283 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
app/controllers/feature_flags_controller.rb
View File

@ -27,6 +27,9 @@
# // The user-visible name of the feature
# "display_name": "Fancy Wickets",
#
# // The user-visible description of the feature
# "description": "This feature makes all Wickets 38% fancier",
#
# // The type of object the feature applies to (RootAccount, Account, Course, or User):
# // * RootAccount features may only be controlled by flags on root accounts.
# // * Account features may be controlled by flags on accounts and their parent accounts.
@ -42,7 +45,9 @@
# "feature_flag": {
# "feature": "fancy_wickets",
# "state": "allowed",
# "locking_account_id": null
# "locked": false,
# "locking_account_id": null,
# "transitions": { "on": { "locked": false }, "off": { "locked": false } }
# },
#
# // If true, a feature that is "allowed" globally will be "off" by default in root accounts.
@ -83,7 +88,22 @@
#
# // If set, this FeatureFlag can only be modified by someone with administrative rights
# // in the specified account
# "locking_account_id": null
# "locking_account_id": null,
#
# // Information about the available state transitions for this flag
# "transitions": {
# "off": {
# // If set, the calling user does not have permission to change to this state
# "locked": false,
#
# // An optional message describing the transition to this state
# "message": "Disabling Fancy Wickets mid-semester may be harmful to student morale."
# },
# "on": {
# "locked": true,
# "message": "This feature may only be enabled in individual courses at this time"
# }
# }
# }
#
class FeatureFlagsController < ApplicationController
@ -189,11 +209,15 @@ class FeatureFlagsController < ApplicationController
# check whether the feature is locked
current_flag = @context.lookup_feature_flag(params[:feature])
return render json: { message: "higher account disallows setting feature flag" }, status: :forbidden if current_flag && current_flag.locked?(@context, @current_user)
if current_flag
return render json: { message: "higher account disallows setting feature flag" }, status: :forbidden if current_flag.locked?(@context, @current_user)
prior_state = current_flag.state
end
# if this is a hidden feature, require site admin privileges to create (but not update) a root account flag
if !current_flag && feature_def.hidden?
return render json: { message: "invalid feature" }, status: :bad_request unless @context.is_a?(Account) && @context.root_account? && Account.site_admin.grants_right?(@current_user, session, :read)
prior_state = 'hidden'
end
# create or update flag
@ -203,6 +227,13 @@ class FeatureFlagsController < ApplicationController
new_flag.feature = params[:feature]
new_flag.state = params[:state] if params.has_key?(:state)
# check transition
transitions = Feature.transitions(new_flag.feature, @current_user, @context, prior_state)
if transitions[new_flag.state] && transitions[new_flag.state]['locked']
return render json: { message: "state change not allowed" }, status: :forbidden
end
# check locking account
if params.has_key?(:locking_account_id)
unless params[:locking_account_id].blank?
locking_account = api_find(Account, params[:locking_account_id])

3
lib/api/v1/feature_flag.rb
View File

@ -24,7 +24,7 @@ module Api::V1::FeatureFlag
hash = feature.as_json.slice('feature', 'applies_to', 'enable_at', 'root_opt_in', 'beta', 'development', 'release_notes_url')
add_localized_attr(hash, feature, 'display_name')
add_localized_attr(hash, feature, 'description')
hash['hidden'] = true if feature.hidden? && Account.site_admin.grants_right?(current_user, session, :manage_feature_flags)
hash['hidden'] = true if feature.hidden? && Account.site_admin.grants_right?(current_user, session, :read)
hash
end
@ -42,6 +42,7 @@ module Api::V1::FeatureFlag
keys = %w(feature context_id context_type state locking_account_id)
api_json(feature_flag, current_user, session, only: keys)
end
hash['transitions'] = Feature.transitions(feature_flag.feature, current_user, context, feature_flag.state)
hash['locked'] = feature_flag.locked?(context, current_user)
hash
end

50
lib/feature.rb
View File

@ -17,7 +17,7 @@
#
class Feature
ATTRS = [:feature, :display_name, :description, :applies_to, :state, :root_opt_in, :enable_at, :beta, :development, :release_notes_url]
ATTRS = [:feature, :display_name, :description, :applies_to, :state, :root_opt_in, :enable_at, :beta, :development, :release_notes_url, :custom_transition_proc]
attr_reader *ATTRS
def initialize(opts = {})
@ -61,8 +61,16 @@ class Feature
# will be inherited in "off" state by root accounts
enable_at: Date.new(2014, 1, 1), # estimated release date shown in UI
beta: false, # 'beta' tag shown in UI
development: false # 'development' tag shown in UI
release_notes_url: 'http://example.com/'
development: false, # 'development' tag shown in UI
release_notes_url: 'http://example.com/',
# optional: you can supply a Proc to attach warning messages to and/or forbid certain transitions
# see lib/feature/draft_state.rb for example usage
custom_transition_proc: ->(user, context, from_state, transitions) do
if from_state == 'off' && context.is_a?(Course) && context.has_submitted_essays?
transitions['on']['warning'] = I18n.t('features.automatic_essay_grading.enable_warning',
'Enabling this feature after some students have submitted essays may yield inconsistent grades.')
end
end
}
=end
@ -77,21 +85,6 @@ class Feature
# TODO: register built-in features here
# (plugins may register additional features during application initialization)
register(
'draft_state' =>
{
display_name: lambda { I18n.t('features.draft_state', 'Draft State') },
description: lambda { I18n.t('draft_state_description', <<DRAFT) },
Draft state is a *beta* feature that allows course content to be published and unpublished.
Unpublished content won't be visible to students and won't affect grades.
It also includes a redesign of some course areas to make them more consistent in look and functionality.
Unpublished content may not be available if Draft State is disabled.
DRAFT
applies_to: 'Course',
state: 'hidden',
root_opt_in: true,
development: true
},
'google_docs_domain_restriction' =>
{
display_name: -> { I18n.t('features.google_docs_domain_restriction', 'Google Docs Domain Restriction') },
@ -174,4 +167,25 @@ END
definitions.values.select{ |fd| applicable_types.include?(fd.applies_to) }
end
def self.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
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)
end
h
end
end
# load feature definitions
Dir.glob("#{Rails.root}/lib/features/*.rb").each { |file| require file }

53
lib/features/draft_state.rb Normal file
View File

@ -0,0 +1,53 @@
#
# Copyright (C) 2014 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 <http://www.gnu.org/licenses/>.
#
Feature.register('draft_state' => {
display_name: lambda { I18n.t('features.draft_state', 'Draft State') },
description: lambda { I18n.t('draft_state_description', <<END) },
Draft state is a *beta* feature that allows course content to be published and unpublished.
Unpublished content won't be visible to students and won't affect grades.
It also includes a redesign of some course areas to make them more consistent in look and functionality.
Unpublished content may not be available if Draft State is disabled.
END
applies_to: 'Course',
state: 'hidden',
root_opt_in: true,
development: true,
custom_transition_proc: ->(user, context, from_state, transitions) do
if context.is_a?(Course) && from_state == 'on'
transitions['off']['message'] = I18n.t('features.draft_state_course_disable_warning', <<END)
Turning this feature off will publish ALL existing objects in the course. Please make sure all draft content
is ready to be published and available to all users in the course before continuing.
END
elsif context.is_a?(Account) && from_state != 'off'
site_admin = Account.site_admin.grants_right?(user, :read)
warning = I18n.t('features.draft_state_account_disable_warning', <<END)
Turning this feature off will impact existing courses. For assistance in disabling this feature, please contact
your Canvas Success Manager.
END
%w(allowed off).each do |target_state|
if transitions.has_key?(target_state)
transitions[target_state]['message'] = warning
transitions[target_state]['locked'] = true unless site_admin
end
end
end
end
}
)

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

@ -32,7 +32,7 @@ describe "Feature Flags API", type: :request do
'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'),
'root_opt_in_feature' => Feature.new(feature: 'root_opt_in_feature', applies_to: 'Course', state: 'allowed', root_opt_in: true),
'hidden_feature' => Feature.new(feature: 'hidden_feature', applies_to: 'Course', state: 'hidden')
'hidden_feature' => Feature.new(feature: 'hidden_feature', applies_to: 'Course', state: 'hidden'),
})
end
@ -56,7 +56,8 @@ describe "Feature Flags API", type: :request do
"feature_flag"=>
{"feature"=>"account_feature",
"state"=>"on",
"locked"=>true}},
"locked"=>true,
"transitions"=>{"allowed"=>{"locked"=>false}, "off"=>{"locked"=>false}}}},
{"feature"=>"course_feature",
"applies_to"=>"Course",
"development"=>true,
@ -69,13 +70,15 @@ describe "Feature Flags API", type: :request do
"locking_account_id"=>t_site_admin.id,
"feature"=>"course_feature",
"state"=>"on",
"locked"=>true}},
"locked"=>true,
"transitions"=>{"allowed"=>{"locked"=>false}, "off"=>{"locked"=>false}}}},
{"feature"=>"root_account_feature",
"applies_to"=>"RootAccount",
"feature_flag"=>
{"feature"=>"root_account_feature",
"state"=>"off",
"locked"=>true}},
"locked"=>true,
"transitions"=>{"allowed"=>{"locked"=>false}, "on"=>{"locked"=>false}}}},
{"feature"=>"root_opt_in_feature",
"applies_to"=>"Course",
"root_opt_in"=>true,
@ -85,7 +88,8 @@ describe "Feature Flags API", type: :request do
"feature"=>"root_opt_in_feature",
"state"=>"off",
"locking_account_id"=>nil,
"locked"=>false}}])
"locked"=>false,
"transitions"=>{"allowed"=>{"locked"=>false}, "on"=>{"locked"=>false}}}}])
end
it "should paginate" do
@ -188,12 +192,13 @@ describe "Feature Flags API", type: :request do
it "should return the correct format" do
json = api_call_as_user(t_teacher, :get, "/api/v1/users/#{t_teacher.id}/features/flags/user_feature",
{ controller: 'feature_flags', action: 'show', format: 'json', user_id: t_teacher.to_param, feature: 'user_feature' })
json.should eql({"feature"=>"user_feature", "state"=>"allowed", "locked"=>false})
json.should eql({"feature"=>"user_feature", "state"=>"allowed", "locked"=>false, "transitions"=>{"on"=>{"locked"=>false}, "off"=>{"locked"=>false}}})
t_teacher.feature_flags.create! feature: 'user_feature', state: 'on'
json = api_call_as_user(t_teacher, :get, "/api/v1/users/#{t_teacher.id}/features/flags/user_feature",
{ controller: 'feature_flags', action: 'show', format: 'json', user_id: t_teacher.to_param, feature: 'user_feature' })
json.should eql({"feature"=>"user_feature", "state"=>"on", "context_type"=>"User", "context_id"=>t_teacher.id, "locked"=>false, "locking_account_id"=>nil})
json.should eql({"feature"=>"user_feature", "state"=>"on", "context_type"=>"User", "context_id"=>t_teacher.id, "locked"=>false, "locking_account_id"=>nil,
"transitions"=>{"off"=>{"locked"=>false}}})
end
describe "hidden" do
@ -420,4 +425,51 @@ describe "Feature Flags API", type: :request do
{}, {}, { expected_status: 403 })
end
end
describe "custom_transition_proc" do
before do
Feature.stubs(:definitions).returns({
'custom_feature' => Feature.new(feature: 'custom_feature', applies_to: 'Course', state: 'allowed',
custom_transition_proc: ->(user, context, from_state, transitions) do
transitions['off'] = { 'locked'=>true, 'message'=>"don't ever turn this off" } if from_state == 'on'
transitions['on'] = { 'locked'=>false, 'message'=>"this is permanent?!" } if transitions.has_key?('on')
end
)
})
end
it "should give message for unlocked transition" do
json = api_call_as_user(t_teacher, :get, "/api/v1/courses/#{t_course.id}/features",
{ controller: 'feature_flags', action: 'index', format: 'json', course_id: t_course.to_param })
json.should eql([
{"feature"=>"custom_feature",
"applies_to"=>"Course",
"feature_flag"=>
{"feature"=>"custom_feature",
"state"=>"allowed",
"locked"=>false,
"transitions"=>{"on"=>{"locked"=>false,"message"=>"this is permanent?!"},"off"=>{"locked"=>false}}}}])
end
context "locked transition" do
before do
t_course.enable_feature! :custom_feature
end
it "should indicate a transition is locked" do
json = api_call_as_user(t_teacher, :get, "/api/v1/courses/#{t_course.id}/features/flags/custom_feature",
{ controller: 'feature_flags', action: 'show', format: 'json', course_id: t_course.id, feature: 'custom_feature' })
json.should eql({"context_id"=>t_course.id,"context_type"=>"Course","feature"=>"custom_feature",
"locking_account_id"=>nil,"state"=>"on", "locked"=>false,
"transitions"=>{"off"=>{"locked"=>true,"message"=>"don't ever turn this off"}}})
end
it "should reject a locked state transition" do
api_call_as_user(t_root_admin, :put, "/api/v1/courses/#{t_course.id}/features/flags/custom_feature?state=off",
{ controller: 'feature_flags', action: 'update', format: 'json', course_id: t_course.to_param, feature: 'custom_feature', state: 'off' },
{}, {}, { expected_status: 403 })
end
end
end
end

82
spec/lib/draft_state_spec.rb Normal file
View File

@ -0,0 +1,82 @@
# Copyright (C) 2014 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 <http://www.gnu.org/licenses/>.
#
require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
describe "Draft State Feature Flag Weirdness" do
let(:t_site_admin) { Account.site_admin }
let(:t_account_manager) { account_admin_user account: t_site_admin, membership_type: 'AccountMembership' }
let(:t_root_account) { account_model }
let(:t_root_admin) { account_admin_user account: t_root_account }
let(:t_course) { course(account: t_root_account, active_all: true) }
context "Course" do
it "should give a warning for 'off'" do
t = Feature.transitions(:draft_state, t_root_admin, t_course, 'on')
t.keys.should eql ['off']
t['off']['locked'].should be_false
t['off']['message'].should =~ /will publish/
end
end
context "Account" do
context "as root admin" do
it "from 'off', should allow 'allowed' and 'on'" do
t = Feature.transitions(:draft_state, t_root_admin, t_root_account, 'off')
t.keys.sort.should eql ['allowed', 'on']
t['allowed']['locked'].should be_false
t['on']['locked'].should be_false
end
it "from 'allowed', should forbid 'off' but allow 'on'" do
t = Feature.transitions(:draft_state, t_root_admin, t_root_account, 'allowed')
t.keys.sort.should eql ['off', 'on']
t['off']['locked'].should be_true
t['off']['message'].should =~ /impact existing courses/
t['on']['locked'].should be_false
end
it "from 'on', should forbid both 'off' and 'allowed'" do
t = Feature.transitions(:draft_state, t_root_admin, t_root_account, 'on')
t.keys.sort.should eql ['allowed', 'off']
t['off']['locked'].should be_true
t['off']['message'].should =~ /impact existing courses/
t['allowed']['locked'].should be_true
t['allowed']['message'].should =~ /impact existing courses/
end
end
context "as account manager" do
it "from 'allowed', should allow 'off' and 'on', but warn for 'off'" do
t = Feature.transitions(:draft_state, t_account_manager, t_root_account, 'allowed')
t.keys.sort.should eql ['off', 'on']
t['off']['locked'].should be_false
t['off']['message'].should =~ /impact existing courses/
t['on']['locked'].should be_false
end
it "from 'on', should allow 'off' and 'allowed', but warn for both" do
t = Feature.transitions(:draft_state, t_account_manager, t_root_account, 'on')
t.keys.sort.should eql ['allowed', 'off']
t['off']['locked'].should be_false
t['off']['message'].should =~ /impact existing courses/
t['allowed']['locked'].should be_false
t['allowed']['message'].should =~ /impact existing courses/
end
end
end
end

22
spec/lib/feature_spec.rb
View File

@ -106,5 +106,25 @@ describe Feature do
Feature.definitions['U'].locked?(t_site_admin).should be_false
end
end
end
describe "default_transitions" do
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}})
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}})
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}})
end
end
end