Fetch grading periods for all related accounts

Previously, the controller for grading periods only fetched active
grading periods for the current context and thus missed grading
periods on account chains.

Closes CNVS-18058

Test Plan: Create grading periods for root account, sub accounts
and courses. Ensure they all show up in the index action
of GradingPeriodsController

Change-Id: I9282b458207b8571022cd8ad3e33208131c8a743
Reviewed-on: https://gerrit.instructure.com/49120
Tested-by: Jenkins
Reviewed-by: Josh Simpson <jsimpson@instructure.com>
QA-Review: Robert Lamb <rlamb@instructure.com>
Product-Review: Derek Bender <dbender@instructure.com>

app/coffeescripts/ember/screenreader_gradebook/tests/integration/muter_component.spec.coffee

@@ -65,7 +65,7 @@ define [
       sendSuccess(server, "#{ENV.GRADEBOOK_OPTIONS.context_url}/assignments/#{@con.get('selectedAssignment.id')}/mute", false)
       andThen =>
         dialog = find('.ui-dialog:visible', 'body')
-        equal(dialog.length, 0, 'the dialog is closed WOOOOOO')
+        equal(dialog.length, 0, 'the dialog is closed')
         checkChecked(false)
         checkLabel(ariaUnmuted)
         equal @con.get('selectedAssignment.muted'), false
@@ -103,7 +103,7 @@ define [
       sendSuccess(server, "#{ENV.GRADEBOOK_OPTIONS.context_url}/assignments/#{@con.get('selectedAssignment.id')}/mute", true)
       andThen =>
         dialog = find('.ui-dialog:visible', 'body')
-        equal(dialog.length, 0, 'the dialog is closed WOOOOOO')
+        equal(dialog.length, 0, 'the dialog is closed')
         checkChecked(true)
         checkLabel(ariaMuted)
         equal @con.get('selectedAssignment.muted'), true

app/controllers/assignments_api_controller.rb

@@ -952,6 +952,8 @@ class AssignmentsApiController < ApplicationController
     end
   end
 
+  private
+
   def save_and_render_response
     @assignment.content_being_saved_by(@current_user)
     if update_api_assignment(@assignment, params[:assignment], @current_user)

app/controllers/courses_controller.rb

@@ -1932,7 +1932,7 @@ class CoursesController < ApplicationController
         standard_id = params[:course].delete :grading_standard_id
         if @course.grants_right?(@current_user, session, :manage_grades)
           if standard_id.present?
-            grading_standard = GradingStandard.standards_for(@course).where(id: standard_id).first
+            grading_standard = GradingStandard.for(@course).where(id: standard_id).first
             @course.grading_standard = grading_standard if grading_standard
           else
             @course.grading_standard = nil

app/controllers/grading_periods_controller.rb

@@ -68,10 +68,11 @@ class GradingPeriodsController < ApplicationController
   #
   def index
     if authorized_action(@context, @current_user, :read)
-      # inheritance check instead of #get_context?
-      @grading_periods = @context.grading_periods.active.order('start_date')
-      json, meta = paginate_for(@grading_periods)
-      render json: serialize_jsonapi(json, meta)
+      # TODO: inheritance check instead of #get_context?
+      grading_periods = GradingPeriod.for(@context).sort_by(&:start_date)
+      paginated_grading_periods, meta = paginate_for(grading_periods)
+
+      render json: serialize_json_api(paginated_grading_periods, meta)
     end
   end
 
@@ -87,8 +88,10 @@ class GradingPeriodsController < ApplicationController
   #
   def show
     @grading_period = @context.grading_periods.active.find(params[:id])
+    # TODO: if there is no grading period found then this action will return
+    #       an empty body which is probably not what we want
     if @grading_period && authorized_action(@grading_period, @current_user, :read)
-      render json: serialize_jsonapi(@grading_period)
+      render json: serialize_json_api(@grading_period)
     end
   end
 
@@ -119,7 +122,7 @@ class GradingPeriodsController < ApplicationController
     @grading_period = grading_period_group.grading_periods.new(grading_period_params)
     if @grading_period && authorized_action(@grading_period, @current_user, :manage)
       if @grading_period.save
-        render json: serialize_jsonapi(@grading_period)
+        render json: serialize_json_api(@grading_period)
       else
         render json: @grading_period.errors, status: :bad_request
       end
@@ -150,7 +153,7 @@ class GradingPeriodsController < ApplicationController
 
     if @grading_period && authorized_action(@grading_period, @current_user, :manage)
       if @grading_period.update_attributes(grading_period_params)
-        render json: serialize_jsonapi(@grading_period)
+        render json: serialize_json_api(@grading_period)
       else
         render json: @grading_period.errors, status: :bad_request
       end
@@ -170,15 +173,15 @@ class GradingPeriodsController < ApplicationController
     end
   end
 
-  protected
+  private
+
   def paginate_for(grading_periods)
-    meta = {}
-    grading_periods, meta = Api.jsonapi_paginate(grading_periods, self, named_context_url(@context, :api_v1_context_grading_periods_url))
+    paginated_grading_periods, meta = Api.jsonapi_paginate(grading_periods, self, named_context_url(@context, :api_v1_context_grading_periods_url))
     meta[:primaryCollection] = 'grading_periods'
-    return grading_periods, meta
+    [paginated_grading_periods, meta]
   end
 
-  def serialize_jsonapi(grading_periods, meta = {})
+  def serialize_json_api(grading_periods, meta = {})
     grading_periods = Array.wrap(grading_periods)
 
     Canvas::APIArraySerializer.new(grading_periods, {

app/controllers/grading_standards_controller.rb

@@ -29,7 +29,7 @@ class GradingStandardsController < ApplicationController
         :MULTIPLE_GRADING_PERIODS => multiple_grading_periods?,
         :DEFAULT_GRADING_STANDARD_DATA => GradingStandard.default_grading_standard
       })
-      @standards = GradingStandard.standards_for(@context).sorted.limit(100)
+      @standards = GradingStandard.for(@context).sorted.limit(100)
       respond_to do |format|
         format.html { }
         format.json {
@@ -58,7 +58,7 @@ class GradingStandardsController < ApplicationController
   end
 
   def update
-    @standard = GradingStandard.standards_for(@context).find(params[:id])
+    @standard = GradingStandard.for(@context).find(params[:id])
     if authorized_action(@standard, @current_user, :manage)
       @standard.user = @current_user
       respond_to do |format|
@@ -72,7 +72,7 @@ class GradingStandardsController < ApplicationController
   end
 
   def destroy
-    @standard = GradingStandard.standards_for(@context).find(params[:id])
+    @standard = GradingStandard.for(@context).find(params[:id])
     if authorized_action(@standard, @current_user, :manage)
       respond_to do |format|
         if @standard.destroy
@@ -83,4 +83,11 @@ class GradingStandardsController < ApplicationController
       end
     end
   end
+
+  private
+
+  def default_data
+    GradingStandard.default_grading_standard
+  end
+
 end

app/models/grading_period.rb

@@ -1,7 +1,8 @@
 class GradingPeriod < ActiveRecord::Base
-  attr_accessible :weight, :start_date, :end_date, :title
   include Workflow
 
+  attr_accessible :weight, :start_date, :end_date, :title
+
   belongs_to :grading_period_group, :inverse_of => :grading_periods
   has_many :grading_period_grades
 
@@ -9,9 +10,9 @@ class GradingPeriod < ActiveRecord::Base
   validate :validate_dates
 
   set_policy do
-    [:read, :manage].each do |permission|
-      given { |user| grading_period_group.grants_right?(user, permission) }
-      can permission
+    [:read, :manage].each do |action|
+      given { |user| grading_period_group.grants_right?(user, action) }
+      can action
     end
   end
 
@@ -22,34 +23,44 @@ class GradingPeriod < ActiveRecord::Base
 
   scope :active, -> { where workflow_state: "active" }
   scope :current, -> { where("start_date <= ? AND end_date >= ?", Time.now, Time.now) }
+  scope :grading_periods_by, ->(context_with_ids) {
+    joins(:grading_period_group).where(grading_period_groups: context_with_ids)
+  }
 
+  def self.for(context)
+    "GradingPeriod::#{context.class}GradingPeriodFinder".constantize.new(context).grading_periods
+  end
+
+  # save the previous definition of `destroy` and alias it to `destroy!`
+  # Note: `destroy!` now does NOT throw errors while the newly defined
+  # `destroy` DOES throw errors due to `save!`
   alias_method :destroy!, :destroy
   def destroy
-    update_attribute :workflow_state, "deleted"
+    self.workflow_state = 'deleted'
+    save!
   end
 
   def assignments(assignment_scope)
     # TODO: avoid wasteful queries
-    assignments = assignment_scope.where(
-      "due_at BETWEEN ? AND ?",
-      start_date, end_date
-    ).all
+    assignments = assignment_scope.where( "due_at BETWEEN ? AND ?", start_date, end_date)
 
-    if self.last?
+    if last?
       assignments + assignment_scope.where(due_at: nil)
     else
       assignments
     end
   end
 
+  private
+
   def last?
     grading_period_group.grading_periods.last == self
   end
 
   def validate_dates
-    if self.start_date && self.end_date
-      errors.add(:end_date, t('errors.invalid_grading_period_end_date', "Grading period end date precedes start date")) if self.end_date < self.start_date
+    if start_date && end_date && end_date < start_date
+      errors.add(:end_date, t('errors.invalid_grading_period_end_date',
+                              'Grading period end date precedes start date'))
     end
   end
-  private :validate_dates
 end

app/models/grading_period/account_grading_period_finder.rb

@@ -0,0 +1,34 @@
+#
+# Copyright (C) 2015 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/>.
+#
+
+class GradingPeriod
+  class AccountGradingPeriodFinder
+
+    def initialize(account)
+      @account = account
+    end
+
+    def grading_periods
+      account_ids = Account.all_accounts_for(account).map(&:id).uniq
+      GradingPeriod.active.grading_periods_by(account_id: account_ids)
+    end
+
+    private
+    attr_reader :account
+  end
+end

app/models/grading_period/course_grading_period_finder.rb

@@ -0,0 +1,36 @@
+#
+# Copyright (C) 2015 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/>.
+#
+
+class GradingPeriod
+  class CourseGradingPeriodFinder
+
+    def initialize(course)
+      @course = course
+    end
+
+    def grading_periods
+      account_ids = Account.all_accounts_for(course.account).map(&:id).uniq
+
+      GradingPeriod.active.grading_periods_by(course_id: course.id) +
+      GradingPeriod.active.grading_periods_by(account_id: account_ids)
+    end
+
+    private
+    attr_reader :course
+  end
+end

app/models/grading_standard.rb

@@ -69,6 +69,12 @@ class GradingStandard < ActiveRecord::Base
     can :manage
   end
 
+  def self.for(context)
+    context_codes = [context.asset_string]
+    context_codes.concat Account.all_accounts_for(context).map(&:asset_string)
+    GradingStandard.active.where(:context_code => context_codes.uniq)
+  end
+
   def version
     read_attribute(:version).presence || 1
   end
@@ -189,12 +195,6 @@ class GradingStandard < ActiveRecord::Base
     res
   end
 
-  def self.standards_for(context)
-    context_codes = [context.asset_string]
-    context_codes.concat Account.all_accounts_for(context).map(&:asset_string)
-    GradingStandard.active.where(:context_code => context_codes.uniq)
-  end
-
   def standard_data=(params={})
     params ||= {}
     res = {}

app/models/importers/assignment_importer.rb

@@ -127,7 +127,7 @@ module Importers
         gs = context.grading_standards.where(migration_id: hash[:grading_standard_migration_id]).first
         item.grading_standard = gs if gs
       elsif hash[:grading_standard_id] && migration
-        gs = GradingStandard.standards_for(context).where(id: hash[:grading_standard_id]).first unless migration.cross_institution?
+        gs = GradingStandard.for(context).where(id: hash[:grading_standard_id]).first unless migration.cross_institution?
         if gs
           item.grading_standard = gs if gs
         else

app/models/importers/course_content_importer.rb

@@ -275,7 +275,7 @@ module Importers
             migration.add_warning(t(:copied_grading_standard_warning, "Couldn't find copied grading standard for the course."))
           end
         elsif settings[:grading_standard_id].present?
-          if gs = GradingStandard.standards_for(course).where(id: settings[:grading_standard_id]).first
+          if gs = GradingStandard.for(course).where(id: settings[:grading_standard_id]).first
             course.grading_standard = gs
           else
             migration.add_warning(t(:account_grading_standard_warning,"Couldn't find account grading standard for the course." ))

config/routes.rb

@@ -1373,6 +1373,7 @@ CanvasRails::Application.routes.draw do
       put 'courses/:course_id/quizzes/:quiz_id/submissions/:id', action: :update, as: 'course_quiz_submission_update'
       post 'courses/:course_id/quizzes/:quiz_id/submissions/:id/complete', action: :complete, as: 'course_quiz_submission_complete'
     end
+
     scope(:controller => 'quizzes/outstanding_quiz_submissions') do
       get 'courses/:course_id/quizzes/:quiz_id/outstanding_quiz_submissions', :action => :index, :path_name => 'outstanding_quiz_submission_index'
       post 'courses/:course_id/quizzes/:quiz_id/outstanding_quiz_submissions', :action => :grade, :path_name => 'outstanding_quiz_submission_grade'

lib/api/v1/assignment.rb

@@ -386,7 +386,7 @@ module Api::V1::Assignment
     if update_params.has_key?("grading_standard_id")
       standard_id = update_params.delete("grading_standard_id")
       if standard_id.present?
-        grading_standard = GradingStandard.standards_for(context).where(id: standard_id).first
+        grading_standard = GradingStandard.for(context).where(id: standard_id).first
         assignment.grading_standard = grading_standard if grading_standard
       else
         assignment.grading_standard = nil

spec/apis/v1/grading_periods_api_spec.rb

@@ -19,6 +19,7 @@
 require File.expand_path(File.dirname(__FILE__) + '/../api_spec_helper')
 
 describe GradingPeriodsController, type: :request do
+  context "A grading period is associated with an account." do
     before :once do
       @account = Account.default
       account_admin_user(account: @account)
@@ -27,42 +28,7 @@ describe GradingPeriodsController, type: :request do
       @account.grading_periods.last.destroy
     end
 
-  context "A grading period is associated with an account." do
     context "multiple grading periods feature flag turned on" do
-      describe 'GET index' do
-        def get_index(raw = false, data = {}, headers = {})
-          helper = method(raw ? :raw_api_call : :api_call)
-          helper.call(:get,
-                      "/api/v1/accounts/#{@account.id}/grading_periods",
-          { controller: 'grading_periods', action: 'index', format: 'json', account_id: @account.id },
-            data,
-            headers)
-        end
-
-        it "returns all existing grading periods" do
-          json = get_index
-          periods_json = json['grading_periods']
-          expect(periods_json.size).to eq(2)
-
-          periods_json.each do |period|
-            expect(period).to have_key('id')
-            expect(period).to have_key('weight')
-            expect(period).to have_key('start_date')
-            expect(period).to have_key('end_date')
-            expect(period).to have_key('title')
-            expect(period).to have_key('permissions')
-          end
-        end
-
-        it "paginates to the jsonapi standard" do
-          json = get_index(false, {}, 'Accept' => 'application/vnd.api+json')
-
-          expect(json).to have_key('meta')
-          expect(json['meta']).to have_key('pagination')
-          expect(json['meta']['primaryCollection']).to eq 'grading_periods'
-        end
-      end
-
       describe 'GET show' do
         before :once do
           @grading_period = @account.grading_periods.first
@@ -186,40 +152,6 @@ describe GradingPeriodsController, type: :request do
     end
     context "multiple grading periods feature flag turned on" do
 
-      describe 'GET index' do
-        def get_index(raw = false, data = {}, headers = {})
-          helper = method(raw ? :raw_api_call : :api_call)
-          helper.call(:get,
-                      "/api/v1/courses/#{@course.id}/grading_periods",
-          { controller: 'grading_periods', action: 'index', format: 'json', course_id: @course.id },
-            data,
-            headers)
-        end
-
-        it "returns all existing grading periods" do
-          json = get_index
-          periods_json = json['grading_periods']
-          expect(periods_json.size).to eq(2)
-
-          periods_json.each do |period|
-            expect(period).to have_key('id')
-            expect(period).to have_key('weight')
-            expect(period).to have_key('start_date')
-            expect(period).to have_key('end_date')
-            expect(period).to have_key('title')
-            expect(period).to have_key('permissions')
-          end
-        end
-
-        it "paginates to the jsonapi standard" do
-          json = get_index(false, {}, 'Accept' => 'application/vnd.api+json')
-
-          expect(json).to have_key('meta')
-          expect(json['meta']).to have_key('pagination')
-          expect(json['meta']['primaryCollection']).to eq 'grading_periods'
-        end
-      end
-
       describe 'GET show' do
         before :once do
           @grading_period = @course.grading_periods.first

spec/controllers/grading_periods_controller_spec.rb

@@ -0,0 +1,86 @@
+#
+# Copyright (C) 2015 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_relative '../spec_helper'
+
+describe GradingPeriodsController do
+  describe "GET index" do
+    let(:root_account) { Account.default }
+    let(:sub_account) { root_account.sub_accounts.create! }
+
+    let(:create_grading_periods) { -> (context, start) {
+        context.grading_period_groups.create!
+               .grading_periods.create!(weight: 1,
+                                        start_date: start,
+                                        end_date: (Time.zone.now + 10.days))
+      }
+    }
+
+    let(:remove_while) { -> (string) {
+        string.sub(%r{^while\(1\);}, '')
+      }
+    }
+
+    before do
+      account_admin_user(account: sub_account)
+      user_session(@admin)
+
+      root_account.allow_feature!(:multiple_grading_periods)
+      root_account.enable_feature!(:multiple_grading_periods)
+
+      create_grading_periods.call(root_account, 3.days.ago)
+      create_grading_periods.call(sub_account, Time.zone.now)
+    end
+
+    context "when context is a sub account" do
+      before do
+        get :index, {account_id: sub_account.id}
+        @json = JSON.parse(remove_while.call(response.body))
+      end
+
+      it "contains two grading periods" do
+        expect(@json['grading_periods'].count).to eql 2
+      end
+
+      it "paginates" do
+        expect(@json).to have_key('meta')
+        expect(@json['meta']).to have_key('pagination')
+        expect(@json['meta']['primaryCollection']).to eq 'grading_periods'
+      end
+
+      it "is ordered by start_date" do
+        expect(@json['grading_periods']).to be_sorted_by('start_date')
+      end
+    end
+
+    context "when context is a course" do
+      before do
+        course = Course.create!(account: sub_account)
+
+        create_grading_periods.call(course, 5.days.ago)
+
+        get :index, {course_id: course.id}
+        @json = JSON.parse(remove_while.call(response.body))
+      end
+
+      it "contains three grading periods" do
+        expect(@json['grading_periods'].count).to eql 3
+      end
+    end
+  end
+end
+

spec/models/grading_period/account_grading_period_finder_spec.rb

@@ -0,0 +1,39 @@
+#
+# Copyright (C) 2015 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 'spec_helper'
+
+describe GradingPeriod::AccountGradingPeriodFinder do
+  describe "#grading_periods" do
+    let(:grading_period_params) { { weight: 0, start_date: Time.now, end_date: 2.days.from_now } }
+    let(:root_account) { Account.create! }
+    let(:sub_account) { root_account.sub_accounts.create! }
+    let!(:root_account_grading_period) { root_account.grading_period_groups.create!
+                                                  .grading_periods.create!(grading_period_params) }
+    let!(:sub_account_grading_period) { sub_account.grading_period_groups.create!
+                                                  .grading_periods.create!(grading_period_params) }
+
+    context "when context is an account" do
+      subject { GradingPeriod::AccountGradingPeriodFinder.new(sub_account).grading_periods }
+      it "finds grading periods for the account, and all associated accounts" do
+
+        is_expected.to eq [root_account_grading_period, sub_account_grading_period]
+      end
+    end
+  end
+end

spec/models/grading_period/course_grading_period_finder_spec.rb

@@ -0,0 +1,41 @@
+#
+# Copyright (C) 2015 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 'spec_helper'
+
+describe GradingPeriod::CourseGradingPeriodFinder do
+  describe "#grading_periods" do
+    let(:grading_period_params) { { weight: 0, start_date: Time.now, end_date: 2.days.from_now } }
+    let(:root_account) { Account.create! }
+    let(:sub_account) { root_account.sub_accounts.create! }
+    let(:course) { Course.create!(account: sub_account) }
+
+    let!(:course_grading_period) { course.grading_period_groups.create!
+                                         .grading_periods.create!(grading_period_params) }
+    let!(:root_account_grading_period) { root_account.grading_period_groups.create!
+                                                  .grading_periods.create!(grading_period_params) }
+    let!(:sub_account_grading_period) { sub_account.grading_period_groups.create!
+                                                  .grading_periods.create!(grading_period_params) }
+
+    it "finds grading periods for the course, and all associated accounts" do
+      grading_periods = GradingPeriod::CourseGradingPeriodFinder.new(course).grading_periods
+      expect(grading_periods).to eq [course_grading_period, root_account_grading_period, sub_account_grading_period]
+    end
+  end
+end
+

spec/models/grading_period_permissions_spec.rb

@@ -0,0 +1,199 @@
+#
+# Copyright (C) 2015 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_relative '../spec_helper'
+
+describe GradingPeriod, "permissions:" do
+  context "course belonging to root account" do
+    before(:once) do
+      @root_account = Account.default
+      @sub_account = @root_account.sub_accounts.create!
+      course_with_teacher(account: @root_account, active_all: true)
+      course_with_student(course: @course, active_all: true)
+      @root_account_period = grading_periods(context: @root_account, count: 1).first
+      @sub_account_period = grading_periods(context: @sub_account, count: 1).first
+      @course_period = grading_periods(context: @course, count: 1).first
+    end
+
+    context "root-account admin" do
+      before(:once) do
+        account_admin_user(account: @root_account)
+      end
+
+      it "should be able to read and manage root-account level grading periods" do
+        expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+
+      it "should be able to read and manage sub-account level grading periods" do
+        expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+
+      it "should be able to read and manage course level grading periods" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+    end
+
+    context "sub-account admin" do
+      before(:once) do
+        account_admin_user(account: @sub_account)
+      end
+
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should be able to read and manage sub-account level grading periods" do
+        expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+
+
+      it "should NOT be able to read or manage course level grading periods, when the course is under the root-account" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
+      end
+    end
+
+    context "teacher" do
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should NOT be able to read or manage sub-account level grading period" do
+        expect(@sub_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: false, manage: false })
+      end
+
+      it "should be able to read and manage course level grading periods" do
+        expect(@course_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: true})
+      end
+    end
+
+    context "student" do
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should NOT be able to read or manage sub-account level grading period" do
+        expect(@sub_account_period.rights_status(@student, :read, :manage)).to eq({ read: false, manage: false })
+      end
+
+      it "should NOT be able to manage course level grading periods, but should be able to read them" do
+        expect(@course_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false})
+      end
+    end
+
+    context "multiple grading periods feature flag turned off" do
+      before(:once) do
+        account_admin_user(account: @root_account)
+        @root_account.disable_feature! :multiple_grading_periods
+      end
+
+      it "should return false for all permissions" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
+      end
+    end
+  end
+
+  context "course belonging to sub-account" do
+    before(:once) do
+      @root_account = Account.default
+      @sub_account = @root_account.sub_accounts.create!
+      course_with_teacher(account: @sub_account, active_all: true)
+      course_with_student(course: @course, active_all: true)
+      @root_account_period = grading_periods(context: @root_account, count: 1).first
+      @sub_account_period = grading_periods(context: @sub_account, count: 1).first
+      @course_period = grading_periods(context: @course, count: 1).first
+    end
+
+    context "root-account admin" do
+      before(:once) do
+        account_admin_user(account: @root_account)
+      end
+
+      it "should be able to read and manage root-account level grading periods" do
+        expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+
+      it "should be able to read and manage sub-account level grading periods" do
+        expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+
+
+      it "should be able to read and manage course level grading periods" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
+      end
+    end
+
+    context "sub-account admin" do
+      before(:once) do
+        account_admin_user(account: @sub_account)
+      end
+
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should be able to read and manage sub-account level grading periods" do
+        expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({read: true, manage: true })
+      end
+
+
+      it "should be able to read and manage course level grading periods, when the course is under the sub-account" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({read: true, manage: true })
+      end
+    end
+
+    context "teacher" do
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should NOT be able to manage sub-account level grading periods, but should be able to read them" do
+        expect(@sub_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should be able to read and manage course level grading periods" do
+        expect(@course_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: true })
+      end
+    end
+
+    context "student" do
+      it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
+        expect(@root_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should NOT be able to manage sub-account level grading periods, but should be able to read them" do
+        expect(@sub_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
+      end
+
+      it "should NOT be able to manage course level grading periods, but should be able to read them" do
+        expect(@course_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false})
+      end
+    end
+
+    context "multiple grading periods feature flag turned off" do
+      before(:once) do
+        account_admin_user(account: @sub_account)
+        @root_account.disable_feature! :multiple_grading_periods
+      end
+
+      it "should return false for all permissions" do
+        expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
+      end
+    end
+  end
+end
+

spec/models/grading_period_spec.rb

@@ -16,247 +16,88 @@
 # with this program. If not, see <http://www.gnu.org/licenses/>.
 #
 
-require File.expand_path(File.dirname(__FILE__) + '/../spec_helper.rb')
+require_relative '../spec_helper'
 
 describe GradingPeriod do
-  before :once do
-    @grading_period = GradingPeriod.create(course_id: 1, start_date: Time.zone.now, end_date: 1.day.from_now)
+  subject { GradingPeriod.new(params) }
+
+  let(:params) do
+    { course_id: 1, start_date: Time.zone.now, end_date: 1.day.from_now }
   end
 
-  describe "validation" do
-    it "should be valid with appropriate input" do
-      expect(@grading_period).to be_valid
+  it { is_expected.to be_valid }
+
+  it "requires a start_date" do
+    grading_period =  GradingPeriod.new(params.except(:start_date))
+    expect(grading_period).to_not be_valid
+  end
+
+  it "requires an end_date" do
+    grading_period = GradingPeriod.new(params.except(:end_date))
+    expect(grading_period).to_not be_valid
+  end
+
+  it "requires start_date to be before end_date" do
+    subject.update_attributes(start_date: Time.zone.now, end_date: 1.day.ago)
+    is_expected.to_not be_valid
+  end
+
+  describe "#destroy" do
+    before { subject.destroy }
+
+    it "marks workflow as deleted" do
+      expect(subject.workflow_state).to eq "deleted"
     end
 
-    it "should require a start_date" do
-      expect(GradingPeriod.create(course_id: 1, end_date: 1.day.from_now)).to_not be_valid
+    it "does not destroy" do
+      expect(subject).to_not be_destroyed
     end
 
-    it "should require an end_date" do
-      expect(GradingPeriod.create(course_id: 1, start_date: Time.zone.now)).to_not be_valid
-    end
+  end
 
-    context "when end_date is before the start_date" do
-      it "should not be able to create a grading period with end_date before the start_date" do
-        expect(GradingPeriod.create(course_id: 1, start_date: 2.days.from_now, end_date: Time.zone.now)).to_not be_valid
+  describe "#destroy!" do
+    before { subject.destroy! }
+
+    it { is_expected.to be_destroyed }
+  end
+
+  describe "#for" do
+
+    context "when context is an account" do
+      let(:account) { Account.new }
+      let(:finder) { mock }
+
+      it "delegates calls" do
+        GradingPeriod::AccountGradingPeriodFinder.expects(:new).with(account).once.returns(finder)
+        finder.expects(:grading_periods).once
+        GradingPeriod.for(account)
       end
+    end
 
-      it "should not be able to update the end_date to be before the start_date" do
-        grading_period = GradingPeriod.create(course_id: 1, start_date: Time.zone.now, end_date: 1.day.from_now)
-        expect(grading_period).to be_valid
-        grading_period.update_attributes(end_date: 1.day.ago)
-        expect(grading_period).not_to be_valid
-      end
+    context "when context is a course" do
+      let(:course) { Course.new }
+      let(:finder) { mock }
 
-      it "should not be able to update the start_date to be after the end_date" do
-        grading_period = GradingPeriod.create(course_id: 1, start_date: Time.zone.now, end_date: 1.day.from_now)
-        expect(grading_period).to be_valid
-        grading_period.update_attributes(start_date: 2.days.from_now)
-        expect(grading_period).not_to be_valid
+      it "delegates calls" do
+        GradingPeriod::CourseGradingPeriodFinder.expects(:new).with(course).once.returns(finder)
+        finder.expects(:grading_periods).once
+        GradingPeriod.for(course)
       end
     end
   end
 
-  describe '#destroy' do
-    it 'soft deletes' do
-      @grading_period.destroy
-      expect(@grading_period).to be_deleted
-      expect(@grading_period).to_not be_destroyed
-    end
-  end
-
-  describe '#assignments' do
-    before :once do
+  describe "#assignments" do
+    it "filters assignments for grading period" do
       course_with_teacher active_all: true
-      @gp1, @gp2 = grading_periods count: 2
-      @a1, @a2 = [@gp1, @gp2].map { |gp|
+      gp1, gp2 = grading_periods count: 2
+      a1, a2 = [gp1, gp2].map { |gp|
         @course.assignments.create! due_at: gp.start_date + 1
       }
       # no due date goes in final grading period
-      @a3 = @course.assignments.create!
-    end
-
-    it 'filters assignments for grading period' do
-      expect(@gp1.assignments(@course.assignments)).to eq [@a1]
-      expect(@gp2.assignments(@course.assignments)).to eq [@a2, @a3]
-    end
-  end
-
-  describe "permissions:" do
-    context "course belonging to root account" do
-      before(:once) do
-        @root_account = Account.default
-        @sub_account = @root_account.sub_accounts.create!
-        course_with_teacher(account: @root_account, active_all: true)
-        course_with_student(course: @course, active_all: true)
-        @root_account_period = grading_periods(context: @root_account, count: 1).first
-        @sub_account_period = grading_periods(context: @sub_account, count: 1).first
-        @course_period = grading_periods(context: @course, count: 1).first
-      end
-
-      context "root-account admin" do
-        before(:once) do
-          account_admin_user(account: @root_account)
-        end
-
-        it "should be able to read and manage root-account level grading periods" do
-          expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-
-        it "should be able to read and manage sub-account level grading periods" do
-          expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-
-        it "should be able to read and manage course level grading periods" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-      end
-
-      context "sub-account admin" do
-        before(:once) do
-          account_admin_user(account: @sub_account)
-        end
-
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should be able to read and manage sub-account level grading periods" do
-          expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-
-
-        it "should NOT be able to read or manage course level grading periods, when the course is under the root-account" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
-        end
-      end
-
-      context "teacher" do
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should NOT be able to read or manage sub-account level grading period" do
-          expect(@sub_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: false, manage: false })
-        end
-
-        it "should be able to read and manage course level grading periods" do
-          expect(@course_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: true})
-        end
-      end
-
-      context "student" do
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should NOT be able to read or manage sub-account level grading period" do
-          expect(@sub_account_period.rights_status(@student, :read, :manage)).to eq({ read: false, manage: false })
-        end
-
-        it "should NOT be able to manage course level grading periods, but should be able to read them" do
-          expect(@course_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false})
-        end
-      end
-
-      context "multiple grading periods feature flag turned off" do
-        before(:once) do
-          account_admin_user(account: @root_account)
-          @root_account.disable_feature! :multiple_grading_periods
-        end
-
-        it "should return false for all permissions" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
-        end
-      end
-    end
-    context "course belonging to sub-account" do
-      before(:once) do
-        @root_account = Account.default
-        @sub_account = @root_account.sub_accounts.create!
-        course_with_teacher(account: @sub_account, active_all: true)
-        course_with_student(course: @course, active_all: true)
-        @root_account_period = grading_periods(context: @root_account, count: 1).first
-        @sub_account_period = grading_periods(context: @sub_account, count: 1).first
-        @course_period = grading_periods(context: @course, count: 1).first
-      end
-
-      context "root-account admin" do
-        before(:once) do
-          account_admin_user(account: @root_account)
-        end
-
-        it "should be able to read and manage root-account level grading periods" do
-          expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-
-        it "should be able to read and manage sub-account level grading periods" do
-          expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-
-
-        it "should be able to read and manage course level grading periods" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: true })
-        end
-      end
-
-      context "sub-account admin" do
-        before(:once) do
-          account_admin_user(account: @sub_account)
-        end
-
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@admin, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should be able to read and manage sub-account level grading periods" do
-          expect(@sub_account_period.rights_status(@admin, :read, :manage)).to eq({read: true, manage: true })
-        end
-
-
-        it "should be able to read and manage course level grading periods, when the course is under the sub-account" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({read: true, manage: true })
-        end
-      end
-
-      context "teacher" do
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should NOT be able to manage sub-account level grading periods, but should be able to read them" do
-          expect(@sub_account_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should be able to read and manage course level grading periods" do
-          expect(@course_period.rights_status(@teacher, :read, :manage)).to eq({ read: true, manage: true })
-        end
-      end
-
-      context "student" do
-        it "should NOT be able to manage root-account level grading periods, but should be able to read them" do
-          expect(@root_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should NOT be able to manage sub-account level grading periods, but should be able to read them" do
-          expect(@sub_account_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false })
-        end
-
-        it "should NOT be able to manage course level grading periods, but should be able to read them" do
-          expect(@course_period.rights_status(@student, :read, :manage)).to eq({ read: true, manage: false})
-        end
-      end
-
-      context "multiple grading periods feature flag turned off" do
-        before(:once) do
-          account_admin_user(account: @sub_account)
-          @root_account.disable_feature! :multiple_grading_periods
-        end
-
-        it "should return false for all permissions" do
-          expect(@course_period.rights_status(@admin, :read, :manage)).to eq({ read: false, manage: false })
-        end
-      end
+      a3 = @course.assignments.create!
+      expect(gp1.assignments(@course.assignments)).to eq [a1]
+      expect(gp2.assignments(@course.assignments)).to eq [a2, a3]
     end
   end
 end
+

spec/models/grading_standard_spec.rb

@@ -76,11 +76,11 @@ describe GradingStandard do
     compare_schemes(standard.data, GradingStandard.default_grading_standard)
   end
 
-  context "standards_for" do
+  context "#for" do
     it "should return standards that match the context" do
       grading_standard_for @course
 
-      standards = GradingStandard.standards_for(@course)
+      standards = GradingStandard.for(@course)
       expect(standards.length).to eq 1
       expect(standards[0].id).to eq @standard.id
     end
@@ -88,7 +88,7 @@ describe GradingStandard do
     it "should include standards made in the parent account" do
       grading_standard_for @course.root_account
 
-      standards = GradingStandard.standards_for(@course)
+      standards = GradingStandard.for(@course)
       expect(standards.length).to eq 1
       expect(standards[0].id).to eq @standard.id
     end
@@ -104,7 +104,7 @@ describe GradingStandard do
         @course.assignments.create!(:title => "hi", :grading_standard_id => gs.id)
       end
 
-      standards = GradingStandard.standards_for(@course).sorted
+      standards = GradingStandard.for(@course).sorted
       expect(standards.length).to eq 2
       expect(standards.map(&:id)).to eq [gs.id, gs2.id]
     end
@@ -115,7 +115,7 @@ describe GradingStandard do
       gs2.title = nil
       gs2.save!
 
-      standards = GradingStandard.standards_for(@course).sorted
+      standards = GradingStandard.for(@course).sorted
       expect(standards.length).to eq 2
       expect(standards.map(&:id)).to eq [gs.id, gs2.id]
     end

spec/selenium/grading_periods_spec.rb

@@ -25,10 +25,6 @@ describe "grading periods" do
       end
 
       it "should show grading periods created by an associated account" do
-        pending("this test marked as pending until the grading periods API
-                  is changed to return grading periods created at the account
-                  level for a given course (in addition to returning grading periods
-                  created at the course level")
         account_grading_period = create_grading_periods_for(@course.root_account).first
         get "/courses/#{@course.id}/grading_standards"
         expect(f("#period_title_#{account_grading_period.id}").attribute("value")).to eq(account_grading_period.title)

spec/spec_helper.rb

@@ -56,6 +56,8 @@ ENV["RAILS_ENV"] = 'test'
 require File.expand_path('../../config/environment', __FILE__) unless defined?(Rails)
 require 'rspec/rails'
 
+Dir[Rails.root.join("spec/support/**/*.rb")].each { |f| require f }
+
 ActionView::TestCase::TestController.view_paths = ApplicationController.view_paths
 
 module RSpec::Rails

spec/support/sorted_by_matcher.rb

@@ -0,0 +1,6 @@
+RSpec::Matchers.define :be_sorted_by do |attr|
+  match do |records|
+    a = records.map{ |record| record.fetch(attr) }
+    a.sort == a
+  end
+end