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

Separate feature flags into settings and features 

fixes LS-2468
flag=feature_flag_filter

test plan:
 - Enable the Feature Flag Filter flag
 - Flags should be organized first by Feature Option or Settings
 - Subcategories should be the applies_to category
 - Search should still work and hide anything that has no results
 - Filter pills should still work and hide anything that has no results
 - With the feature flag filter off the feature flags page should be
unchanged

Change-Id: I5473df54f1cb473b88a889bdec50316689617130
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/271373
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Nate Armstrong <narmstrong@instructure.com>
QA-Review: Nate Armstrong <narmstrong@instructure.com>
Product-Review: Eric Saupe <eric.saupe@instructure.com>
This commit is contained in:
Eric Saupe 2021-08-11 08:35:22 -07:00
parent 2ac7d493bb
commit 5d51f78135
11 changed files with 179 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/feature_flags_controller.rb
View File

@ -156,7 +156,7 @@ class FeatureFlagsController < ApplicationController
def index
if authorized_action(@context, @current_user, :read)
route = polymorphic_url([:api_v1, @context, :features])
features = Feature.applicable_features(@context)
features = Feature.applicable_features(@context, type: params[:type])
features = Api.paginate(features, self, route)
skip_cache = @context.grants_right?(@current_user, session, :manage_feature_flags)

2
config/feature_flags/00_standard.yml
View File

@ -99,6 +99,7 @@ duplicate_modules:
applies_to: Account
root_opt_in: true
embedded_release_notes:
type: setting
state: allowed_on
display_name: Embedded Release Notes
description: Show Instructure-provided release notes filtered by user role in the
@ -369,6 +370,7 @@ rubric_criterion_range:
applies_to: RootAccount
root_opt_in: true
self_service_user_merge:
type: setting
state: allowed_on
applies_to: RootAccount
display_name: Self Service User Merge

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

@ -24,7 +24,7 @@ module Api::V1::FeatureFlag
def feature_json(feature, current_user, session)
# this isn't an AR object, so api_json doesn't work
hash = feature.as_json.slice('feature', 'applies_to', 'enable_at', 'root_opt_in', 'beta',
'release_notes_url', 'autoexpand', 'pending_enforcement')
'release_notes_url', 'autoexpand', 'pending_enforcement', 'type')
add_localized_attr(hash, feature, 'display_name')
add_localized_attr(hash, feature, 'description')
hash

8
lib/feature.rb
View File

@ -20,7 +20,7 @@
class Feature
ATTRS = [:feature, :display_name, :description, :applies_to, :state,
:root_opt_in, :enable_at, :beta, :pending_enforcement,
:root_opt_in, :enable_at, :beta, :type, :pending_enforcement,
:release_notes_url, :custom_transition_proc, :visible_on,
:after_state_change_proc, :autoexpand, :touch_context].freeze
attr_reader *ATTRS
@ -133,6 +133,7 @@ class Feature
VALID_STATES = [STATE_ON, STATE_DEFAULT_OFF, STATE_DEFAULT_ON, STATE_HIDDEN, STATE_DISABLED].freeze
VALID_APPLIES_TO = %w(Course Account RootAccount User SiteAdmin).freeze
VALID_ENVS = %i(development ci beta test production).freeze
VALID_TYPES = %w(feature_option setting).freeze
DISABLED_FEATURE = Feature.new.freeze
@ -155,6 +156,7 @@ class Feature
raise "applies_to is required for feature #{feature}" unless attrs[:applies_to]
raise "invalid 'state' for feature #{feature}: must be one of #{VALID_STATES}, is #{attrs[:state]}" unless VALID_STATES.include? attrs[:state]
raise "invalid 'applies_to' for feature #{feature}: must be one of #{VALID_APPLIES_TO}, is #{attrs[:applies_to]}" unless VALID_APPLIES_TO.include? attrs[:applies_to]
raise "invalid 'type' for feature #{feature}: must be one of #{VALID_TYPES}, is #{attrs[:type]}" unless VALID_TYPES.include? attrs[:type]
end
def self.definitions
@ -201,7 +203,7 @@ class Feature
feature_def.applies_to_object(object)
end
def self.applicable_features(object)
def self.applicable_features(object, type: nil)
applicable_types = []
if object.is_a?(Account)
applicable_types << 'Account'
@ -214,7 +216,7 @@ class Feature
elsif object.is_a?(User)
applicable_types << 'User'
end
definitions.values.select { |fd| applicable_types.include?(fd.applies_to) }
definitions.values.select { |fd| applicable_types.include?(fd.applies_to) && (type.nil? || fd.type == type) }
end
def default_transitions(context, orig_state)

1
lib/feature_flags/loader.rb
View File

@ -48,6 +48,7 @@ module FeatureFlags
[:custom_transition_proc, :after_state_change_proc, :visible_on].each do |check|
definition[check] = wrap_hook_method(definition[check]) if definition[check]
end
definition[:type] ||= 'feature_option'
[:display_name, :description].each do |field|
definition[field] = wrap_translate_text(definition[field])
end

3
spec/lib/feature_spec.rb
View File

@ -215,7 +215,8 @@ describe "Feature.register" do
display_name: -> { "some feature or other" },
description: -> { "this does something" },
applies_to: 'RootAccount',
state: 'allowed'
state: 'allowed',
type: 'feature_option'
}
end

45
ui/shared/feature-flags/react/FeatureFlagTable.js
View File

@ -29,26 +29,28 @@ import {Text} from '@instructure/ui-text'
const {Head, Body, ColHeader, Row, Cell} = Table
function FeatureFlagFilterTable({title, rows, disableDefaults}) {
return (
<>
<Heading as="h2" level="h3" data-testid="ff-table-heading">
{title}
</Heading>
<Table caption={title} margin="medium 0">
function FeatureFlagFilterTable({title, rows, disableDefaults, showTitle}) {
const typeTables = []
// render type tables
for (const type in rows) {
rows[type].sort((a, b) => a.display_name.localeCompare(b.display_name))
if (rows[type].length < 1) {
continue
}
typeTables.push(
<Table key={type} caption={title} margin="medium 0">
<Head>
<Row>
<ColHeader id="display_name" width="50%">
{I18n.t('Feature')}
{type}
</ColHeader>
<ColHeader id="status" width="50%">
{I18n.t('Status')}
</ColHeader>
<ColHeader id="state">{I18n.t('State')}</ColHeader>
<ColHeader id="status" width="50%" />
<ColHeader id="state" />
</Row>
</Head>
<Body>
{rows.map(feature => (
{rows[type].map(feature => (
<Row key={feature.feature} data-testid="ff-table-row">
<Cell>
<ToggleDetails summary={feature.display_name} defaultExpanded={feature.autoexpand}>
@ -102,15 +104,26 @@ function FeatureFlagFilterTable({title, rows, disableDefaults}) {
))}
</Body>
</Table>
)
}
return (
<>
{showTitle ? (
<Heading as="h2" level="h3" data-testid="ff-table-heading">
{title}
</Heading>
) : null}
{typeTables}
</>
)
}
function FeatureFlagTable({title, rows, disableDefaults}) {
rows.sort((a, b) => a.display_name.localeCompare(b.display_name))
function FeatureFlagTable({title, rows, disableDefaults, showTitle}) {
if (ENV.FEATURES?.feature_flag_filters) {
return FeatureFlagFilterTable({title, rows, disableDefaults})
return FeatureFlagFilterTable({title, rows, disableDefaults, showTitle})
}
rows.sort((a, b) => a.display_name.localeCompare(b.display_name))
return (
<>
<Heading as="h2" level="h3" data-testid="ff-table-heading">

88
ui/shared/feature-flags/react/FeatureFlags.js
View File

@ -38,31 +38,6 @@ export default function FeatureFlags({hiddenFlags, disableDefaults}) {
const [searchQuery, setSearchQuery] = useState('')
const [selectedFeatureStates, setSelectedFeatureStates] = useState([])
useFetchApi({
success: setFeatures,
loading: setLoading,
path: `/api/v1${ENV.CONTEXT_BASE_URL}/features`,
fetchAllPages: true,
params: {
hide_inherited_enabled: true,
per_page: 50
}
})
const groupedFeatures = groupBy(
features.filter(
feat =>
(!hiddenFlags || !hiddenFlags.includes(feat.feature)) &&
feat.display_name.toLowerCase().includes(searchQuery.toLowerCase()) &&
(!selectedFeatureStates.length || selectedFeatureStates.some(state => feat[state]))
),
'applies_to'
)
if (groupedFeatures.Account || groupedFeatures.RootAccount) {
groupedFeatures.Account = (groupedFeatures.Account || []).concat(
groupedFeatures.RootAccount || []
)
}
const categories = [
{
id: 'SiteAdmin',
@ -79,9 +54,69 @@ export default function FeatureFlags({hiddenFlags, disableDefaults}) {
{
id: 'User',
title: I18n.t('User')
},
{
id: 'feature_option',
title: I18n.t('Feature Option')
},
{
id: 'setting',
title: I18n.t('Setting')
}
]
useFetchApi({
success: setFeatures,
loading: setLoading,
path: `/api/v1${ENV.CONTEXT_BASE_URL}/features`,
fetchAllPages: true,
params: {
hide_inherited_enabled: true,
per_page: 50
}
})
let groupedFeatures = {}
if (ENV.FEATURES?.feature_flag_filters) {
groupedFeatures = features.reduce((accum, feat) => {
if (
(!hiddenFlags || !hiddenFlags.includes(feat.feature)) &&
feat.display_name.toLowerCase().includes(searchQuery.toLowerCase()) &&
(!selectedFeatureStates.length || selectedFeatureStates.some(state => feat[state]))
) {
let {applies_to} = feat
if (applies_to === 'RootAccount') {
applies_to = 'Account'
}
accum[feat.type] = accum[feat.type] || {}
for (const category of categories) {
accum[feat.type][category.title] = accum[feat.type][category.title] || []
if (category.id === applies_to) {
accum[feat.type][category.title] = accum[feat.type][category.title].concat(feat)
}
}
}
return accum
}, {})
} else {
groupedFeatures = groupBy(
features.filter(
feat =>
(!hiddenFlags || !hiddenFlags.includes(feat.feature)) &&
feat.display_name.toLowerCase().includes(searchQuery.toLowerCase()) &&
(!selectedFeatureStates.length || selectedFeatureStates.some(state => feat[state]))
),
'applies_to'
)
if (groupedFeatures.Account || groupedFeatures.RootAccount) {
groupedFeatures.Account = (groupedFeatures.Account || []).concat(
groupedFeatures.RootAccount || []
)
}
}
const handleQueryChange = debounce(setSearchQuery, SEARCH_DELAY, {
leading: false,
trailing: true
@ -124,7 +159,7 @@ export default function FeatureFlags({hiddenFlags, disableDefaults}) {
</Flex>
{categories.map(cat => {
if (!groupedFeatures[cat.id]?.length) {
if (!groupedFeatures[cat.id] || !Object.keys(groupedFeatures[cat.id]).length) {
return null
}
return (
@ -133,6 +168,7 @@ export default function FeatureFlags({hiddenFlags, disableDefaults}) {
title={cat.title}
rows={groupedFeatures[cat.id]}
disableDefaults={disableDefaults}
showTitle={Object.keys(groupedFeatures[cat.id]).length > 0}
/>
)
})}

40
ui/shared/feature-flags/react/__tests__/FeatureFlagTable.test.js
View File

@ -22,15 +22,17 @@ import {render, act, fireEvent} from '@testing-library/react'
import FeatureFlagTable from '../FeatureFlagTable'
import sampleData from './sampleData.json'
const rows = [
sampleData.allowedOnFeature,
sampleData.allowedFeature,
sampleData.betaFeature,
sampleData.onFeature,
sampleData.offFeature,
sampleData.pendingEnforcementOnFeature,
sampleData.pendingEnforcementOffFeature
]
const rows = {
feature_option: [
sampleData.allowedOnFeature,
sampleData.betaFeature,
sampleData.onFeature,
sampleData.offFeature,
sampleData.pendingEnforcementOnFeature,
sampleData.pendingEnforcementOffFeature
],
setting: [sampleData.allowedFeature]
}
const title = 'Section 123'
describe('feature_flags::FeatureFlagTable', () => {
@ -43,19 +45,27 @@ describe('feature_flags::FeatureFlagTable', () => {
})
it('Shows the title', () => {
const {getByTestId} = render(<FeatureFlagTable rows={rows} title={title} />)
const {getByTestId} = render(<FeatureFlagTable rows={rows} title={title} showTitle />)
expect(getByTestId('ff-table-heading')).toHaveTextContent(title)
})
it('Sorts the features', () => {
it('Sorts the features within groups', () => {
const {getAllByTestId} = render(<FeatureFlagTable rows={rows} title={title} />)
// Feature option
expect(getAllByTestId('ff-table-row')[0]).toHaveTextContent('Beta Feature')
expect(getAllByTestId('ff-table-row')[1]).toHaveTextContent('Feature 1')
expect(getAllByTestId('ff-table-row')[2]).toHaveTextContent('Feature 2')
expect(getAllByTestId('ff-table-row')[3]).toHaveTextContent('Feature 3')
expect(getAllByTestId('ff-table-row')[4]).toHaveTextContent('Feature 4')
expect(getAllByTestId('ff-table-row')[1]).toHaveTextContent('Feature 2')
expect(getAllByTestId('ff-table-row')[2]).toHaveTextContent('Feature 3')
expect(getAllByTestId('ff-table-row')[3]).toHaveTextContent('Feature 4')
expect(getAllByTestId('ff-table-row')[4]).toHaveTextContent(
'Feature with Pending Enforcement Off'
)
expect(getAllByTestId('ff-table-row')[5]).toHaveTextContent(
'Feature with Pending Enforcement On'
)
// Setting
expect(getAllByTestId('ff-table-row')[6]).toHaveTextContent('Feature 1')
})
it('Includes the descriptions, respecting autoexpand', () => {

35
ui/shared/feature-flags/react/__tests__/FeatureFlags.test.js
View File

@ -117,13 +117,44 @@ describe('feature_flags::FeatureFlags', () => {
const filterField = await findByPlaceholderText('Filter Features')
fireEvent.change(filterField, {target: {value: 'active'}})
fireEvent.click(await getAllByText('Active Development')[1])
expect(await getAllByText('Course')[0]).toBeInTheDocument()
expect(await getAllByText('Beta Feature')[0]).toBeInTheDocument()
await waitFor(() => {
expect(getAllByText('Course')[0]).toBeInTheDocument()
expect(getAllByText('Beta Feature')[0]).toBeInTheDocument()
expect(queryByText('Account')).not.toBeInTheDocument()
expect(queryByText('User')).not.toBeInTheDocument()
})
})
it('renders section titles for types', async () => {
const {queryAllByTestId} = render(
<div>
<FeatureFlags />
<div id="aria_alerts" role="alert" />
</div>
)
await waitFor(() => {
expect(queryAllByTestId('ff-table-heading')[0]).toHaveTextContent('Feature Option')
expect(queryAllByTestId('ff-table-heading')[1]).toHaveTextContent('Setting')
})
})
it('hides section titles if no row exists in section after search', async () => {
const {findByPlaceholderText, getAllByText, queryAllByTestId, queryByText} = render(
<div>
<FeatureFlags />
<div id="aria_alerts" role="alert" />
</div>
)
const searchField = await findByPlaceholderText('Search')
fireEvent.change(searchField, {target: {value: 'Feature 4'}})
expect(await getAllByText('User')[0]).toBeInTheDocument()
await waitFor(() => {
expect(queryAllByTestId('ff-table-heading')[0]).toHaveTextContent('Feature Option')
expect(queryByText('Setting')).not.toBeInTheDocument()
expect(queryByText('Account')).not.toBeInTheDocument()
expect(queryByText('Course')).not.toBeInTheDocument()
})
})
})
})
})

27
ui/shared/feature-flags/react/__tests__/sampleData.json
View File

@ -22,7 +22,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed"
}
},
"type": "setting"
},
"allowedOnFeature": {
"feature": "feature2",
@ -47,7 +48,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed"
}
},
"type": "feature_option"
},
"onFeature": {
"feature": "feature3",
@ -72,7 +74,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed"
}
},
"type": "feature_option"
},
"offFeature": {
"feature": "feature4",
@ -98,7 +101,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed"
}
},
"type": "feature_option"
},
"allowedOnRootAccountFeature": {
"feature": "feature5",
@ -123,7 +127,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed_on"
}
},
"type": "feature_option"
},
"allowedOnCourseFeature": {
"feature": "feature6",
@ -145,7 +150,8 @@
"locked": false,
"hidden": false,
"parent_state": "allowed_on"
}
},
"type": "feature_option"
},
"betaFeature": {
"feature": "betaFeature",
@ -168,7 +174,8 @@
"hidden": false,
"parent_state": "allowed_on"
},
"beta": true
"beta": true,
"type": "feature_option"
},
"pendingEnforcementOnFeature": {
"feature": "feature7",
@ -192,7 +199,8 @@
"locked": false,
"hidden": true,
"parent_state": "hidden"
}
},
"type": "feature_option"
},
"pendingEnforcementOffFeature": {
"feature": "feature8",
@ -215,6 +223,7 @@
"locked": false,
"hidden": true,
"parent_state": "hidden"
}
},
"type": "feature_option"
}
}