refactor multiple grading periods code

some small refactors to multiple grading periods
code based on code review from ccbd8876.

refs CNVS-26727

test plan:
1. turn on multiple grading periods at the account level
2. for a course under the account, verify the
   course /grading_standards page loads when logged in as
    a) a teacher, and
    b) an admin
3. verify the account /grading_standards page loads when
   logged in as an admin

Change-Id: I4e5bd39f442e84c938ab09d71cd44b8fd75cf65a
Reviewed-on: https://gerrit.instructure.com/78850
Reviewed-by: Derek Bender <djbender@instructure.com>
Reviewed-by: Jeremy Neander <jneander@instructure.com>
Tested-by: Jenkins
QA-Review: KC Naegle <knaegle@instructure.com>
Product-Review: Christi Wruck

app/controllers/grading_periods_controller.rb

@@ -75,15 +75,7 @@ class GradingPeriodsController < ApplicationController
     if authorized_action(@context, @current_user, :read)
       grading_periods = GradingPeriod.for(@context).sort_by(&:start_date)
       paginated_grading_periods, meta = paginate_for(grading_periods)
-      can_create_grading_periods = @context.is_a?(Account) &&
+      render json: serialize_json_api(paginated_grading_periods, meta).merge(index_permissions)
-        @context.root_account? && @context.grants_right?(@current_user, :manage)
-      can_toggle_grading_periods = @domain_root_account.grants_right?(@current_user, :manage) ||
-                       @context.feature_allowed?(:multiple_grading_periods, exclude_enabled: true)
-      permissions = {
-        can_create_grading_periods: can_create_grading_periods,
-        can_toggle_grading_periods: can_toggle_grading_periods
-      }.as_json
-      render json: serialize_json_api(paginated_grading_periods, meta).merge(permissions)
     end
   end
 
@@ -248,4 +240,15 @@ class GradingPeriodsController < ApplicationController
       include_root: false
     }).as_json
   end
+
+  def index_permissions
+    can_create_grading_periods = @context.is_a?(Account) &&
+      @context.root_account? && @context.grants_right?(@current_user, :manage)
+    can_toggle_grading_periods = @domain_root_account.grants_right?(@current_user, :manage) ||
+      @context.feature_allowed?(:multiple_grading_periods, exclude_enabled: true)
+    {
+      can_create_grading_periods: can_create_grading_periods,
+      can_toggle_grading_periods: can_toggle_grading_periods
+    }.as_json
+  end
 end

spec/coffeescripts/jsx/gradebook/GradingPeriodTemplateSpec.coffee

@@ -98,37 +98,30 @@ define [
     endDateNode = React.findDOMNode(@gradingPeriod.refs.endDate)
     equal endDateNode.value, "May 31, 2015 at 12am"
 
-
   module 'custom prop validation for editable periods',
-    defaultProps: ->
-      title: "Spring"
-      startDate: new Date("2015-03-01T00:00:00Z")
-      endDate: new Date("2015-05-31T00:00:00Z")
-      id: "1"
-      readonly: false
-      disabled: false
-      onDeleteGradingPeriod: ->
-      onDateChange: ->
-      onTitleChange: ->
-
     setup: ->
       @consoleWarn = @stub(console, 'warn')
+      @props =
+        title: "Spring"
+        startDate: new Date("2015-03-01T00:00:00Z")
+        endDate: new Date("2015-05-31T00:00:00Z")
+        id: "1"
+        readonly: false
+        disabled: false
+        onDeleteGradingPeriod: ->
+        onDateChange: ->
+        onTitleChange: ->
 
   test 'does not warn of invalid props if all required props are present and of the correct type', ->
-    props = @defaultProps()
+    React.createElement(GradingPeriod, @props)
-    React.createElement(GradingPeriod, props)
     ok @consoleWarn.notCalled
 
-  test 'warns of missing props if required props are missing', ->
+  test 'warns if required props are missing', ->
-    props = @defaultProps()
+    delete @props.disabled
-    delete props.disabled
+    React.createElement(GradingPeriod, @props)
-    invalidPropMessage = "Warning: Failed propType: GradingPeriodTemplate: required prop disabled not provided or of wrong type."
+    ok @consoleWarn.calledOnce
-    React.createElement(GradingPeriod, props)
-    ok @consoleWarn.calledWithExactly(invalidPropMessage)
 
-  test 'warns of invalid props if required props are of the wrong type', ->
+  test 'warns if required props are of the wrong type', ->
-    props = @defaultProps()
+    @props.onDeleteGradingPeriod = "a/s/l?"
-    props.onDeleteGradingPeriod = 'hi ;)'
+    React.createElement(GradingPeriod, @props)
-    invalidPropMessage = "Warning: Failed propType: GradingPeriodTemplate: required prop onDeleteGradingPeriod not provided or of wrong type."
+    ok @consoleWarn.calledOnce
-    React.createElement(GradingPeriod, props)
-    ok @consoleWarn.calledWithExactly(invalidPropMessage)

spec/controllers/grading_periods_controller_spec.rb

@@ -54,11 +54,6 @@ describe GradingPeriodsController do
     describe 'GET index' do
       let(:sub_account_admin) { account_admin_user(account: sub_account) }
       context "can_create_grading_periods" do
-        it "is a key returned with the collection" do
-          get :index, account_id: root_account.id
-          expect(json_parse).to have_key 'can_create_grading_periods'
-        end
-
         it "returns true if the context is a root account " \
         "and the user is a root account admin" do
           get :index, account_id: root_account.id
@@ -73,11 +68,6 @@ describe GradingPeriodsController do
       end
 
       context "can_toggle_grading_periods" do
-        it "is a key returned with the collection" do
-          get :index, account_id: root_account.id
-          expect(json_parse).to have_key 'can_toggle_grading_periods'
-        end
-
         it "returns true if the user is a root account admin" do
           get :index, account_id: root_account.id
           expect(json_parse['can_toggle_grading_periods']).to eq true

spec/lib/feature_flags_spec.rb

@@ -43,17 +43,18 @@ describe FeatureFlags do
     expect(t_sub_account.feature_enabled?(:account_feature)).to be_truthy
   end
 
-  it "should report feature_allowed? correctly" do
+  describe "#feature_allowed?" do
-    expect(t_sub_account.feature_allowed?(:account_feature)).to be_truthy
+    it "returns true if the feature is 'on' or 'allowed', and false otherwise" do
-    expect(t_root_account.feature_allowed?(:root_account_feature)).to be_falsey
+      expect(t_sub_account.feature_allowed?(:account_feature)).to be_truthy
-    expect(t_course.feature_allowed?(:course_feature)).to be_truthy
+      expect(t_root_account.feature_allowed?(:root_account_feature)).to be_falsey
-  end
+      expect(t_course.feature_allowed?(:course_feature)).to be_truthy
+    end
 
-  it "reports feature_allowed? correctly when passed exclude_enabled: true, " \
+    it "returns true if the feature is 'allowed', and false otherwise when passed exclude_enabled: true" do
-  "only returning true if the feature is in 'allowed' state" do
+      expect(t_sub_account.feature_allowed?(:account_feature, exclude_enabled: true)).to be_falsey
-    expect(t_sub_account.feature_allowed?(:account_feature, exclude_enabled: true)).to be_falsey
+      expect(t_root_account.feature_allowed?(:root_account_feature, exclude_enabled: true)).to be_falsey
-    expect(t_root_account.feature_allowed?(:root_account_feature, exclude_enabled: true)).to be_falsey
+      expect(t_course.feature_allowed?(:course_feature, exclude_enabled: true)).to be_truthy
-    expect(t_course.feature_allowed?(:course_feature, exclude_enabled: true)).to be_truthy
+    end
   end
 
   describe "lookup_feature_flag" do