Extract Soft Deletion

fixes CNVS-20361

This is the first step in extracting soft deletion and
only touches grading period models. I audited other models
to see if this extraction would work for them, but no other
models met my criteria which are as follows

- [ ] defines workflow state of _only_ active and deleted
- [ ] active scope where state is active
      (not `workflow_state<>'deleted'`)
In `def destroy`:
  - [ ] state set to deleted
  - [ ] then save
  - [ ] runs callbacks

Many variants of soft deletion exist, including scoping
active to `workflow_state<>'deleted'`, not running
callbacks after save, and wrapping the state change in a
transaction block.

This is the first step in making soft deletion behave
consistently. There are approximately 30 models that have
an implementation of soft deletion.

Change-Id: I6cb48571377a4bb403285f95c058020b46ca3a30
Reviewed-on: https://gerrit.instructure.com/53821
Reviewed-by: Derek Bender <dbender@instructure.com>
Product-Review: Strand McCutchen <smccutchen@instructure.com>
QA-Review: Strand McCutchen <smccutchen@instructure.com>
Tested-by: Jenkins

app/models/grading_period.rb

@@ -1,5 +1,5 @@
 class GradingPeriod < ActiveRecord::Base
-  include Workflow
+  include Canvas::SoftDeletable
 
   attr_accessible :weight, :start_date, :end_date, :title
 
@@ -17,12 +17,6 @@ class GradingPeriod < ActiveRecord::Base
     can :manage
   end
 
-  workflow do
-    state :active
-    state :deleted
-  end
-
-  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)
@@ -47,16 +41,6 @@ class GradingPeriod < ActiveRecord::Base
     start_date <= Time.now && end_date >= Time.now
   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
-    self.workflow_state = 'deleted'
-    save!
-    run_callbacks :destroy
-  end
-
   def assignments(assignment_scope)
     # TODO: avoid wasteful queries
     assignments = assignment_scope.where( "due_at BETWEEN ? AND ?", start_date, end_date)

app/models/grading_period_grade.rb

@@ -1,5 +1,5 @@
 class GradingPeriodGrade < ActiveRecord::Base
-  include Workflow
+  include Canvas::SoftDeletable
 
   #TODO: when we create a controller for this, remove attr_accessible and use strong params instead
   attr_accessible :enrollment_id, :grading_period_id, :current_grade, :final_grade
@@ -15,17 +15,4 @@ class GradingPeriodGrade < ActiveRecord::Base
       can permission
     end
   end
-
-  workflow do
-    state :active
-    state :deleted
-  end
-
-  scope :active, -> { where workflow_state: "active" }
-
-  alias_method :destroy!, :destroy
-  def destroy
-    self.workflow_state = 'deleted'
-    save!
-  end
 end

app/models/grading_period_group.rb

@@ -1,8 +1,7 @@
 class GradingPeriodGroup < ActiveRecord::Base
-  include Workflow
-
-  attr_accessible
+  include Canvas::SoftDeletable
 
+  attr_accessible # None of this model's attributes are mass-assignable
   belongs_to :course
   belongs_to :account
   has_many :grading_periods, dependent: :destroy
@@ -20,24 +19,10 @@ class GradingPeriodGroup < ActiveRecord::Base
     can :manage
   end
 
-  workflow do
-    state :active
-    state :deleted
-  end
-
-  scope :active, -> { where workflow_state: "active" }
-
   def multiple_grading_periods_enabled?
     (course || account).feature_enabled?(:multiple_grading_periods) || account_grading_period_allowed?
   end
 
-  alias_method :destroy!, :destroy
-  def destroy
-    self.workflow_state = 'deleted'
-    save!
-    run_callbacks :destroy
-  end
-
   private
   def belongs_to_course_or_account_exclusive
     if course.blank? && account.blank?

lib/canvas/soft_deletable.rb

@@ -0,0 +1,26 @@
+require 'active_support/concern'
+
+module Canvas::SoftDeletable
+  extend ActiveSupport::Concern
+
+  included do
+    include Workflow
+
+    workflow do
+      state :active
+      state :deleted
+    end
+
+    scope :active, -> { where workflow_state: "active" }
+
+    # 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
+      self.workflow_state = 'deleted'
+      save!
+      run_callbacks :destroy
+    end
+  end
+end

spec/models/grading_period_grade_spec.rb

@@ -0,0 +1,36 @@
+#
+# 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 GradingPeriodGrade do
+  context "Soft deletion" do
+    let(:student) { User.create.student_enrollments.create course: course }
+
+    let(:account) { Account.create }
+    let(:group) { account.grading_period_groups.create }
+    let(:period) do
+      group.grading_periods.create  start_date: 1.week.ago,
+                                    end_date: 2.weeks.from_now
+    end
+
+    let(:creation_arguments) { { grading_period_id: period.id } }
+    subject { student.grading_period_grades }
+    include_examples "soft deletion"
+  end
+end

spec/models/grading_period_group_spec.rb

@@ -86,27 +86,25 @@ describe GradingPeriodGroup do
     end
   end
 
-  describe "#destroy" do
-    let(:grading_period_group) { Account.default.grading_period_groups.create }
+  context "Soft deletion" do
+    let(:account) { Account.create }
+    let(:creation_arguments) { {} }
+    subject { account.grading_period_groups }
+    include_examples "soft deletion"
 
-    it "should soft delete by setting the workflow status to deleted" do
-      expect(grading_period_group).to_not be_deleted
-      grading_period_group.destroy
-      expect(grading_period_group).to be_deleted
-      expect(grading_period_group).to_not be_destroyed
-    end
+    describe "#destroy" do
+      let(:grading_period_group) { Account.default.grading_period_groups.create }
 
-    it "should soft delete grading periods that belong to the grading period group when it is destroyed" do
-      grading_period_1 = grading_period_group.grading_periods.create!(start_date: Time.now, end_date: 2.months.from_now)
-      grading_period_2 = grading_period_group.grading_periods.create!(start_date: 2.months.from_now, end_date: 3.months.from_now)
-      expect(grading_period_group).to_not be_deleted
-      expect(grading_period_1).to_not be_deleted
-      expect(grading_period_2).to_not be_deleted
-      grading_period_group.destroy
-      expect(grading_period_1).to be_deleted
-      expect(grading_period_1).to_not be_destroyed
-      expect(grading_period_2).to be_deleted
-      expect(grading_period_2).to_not be_destroyed
+      it "should soft delete grading periods that belong to the grading period group when it is destroyed" do
+        grading_period_1 = grading_period_group.grading_periods.create!(start_date: Time.now, end_date: 2.months.from_now)
+        grading_period_2 = grading_period_group.grading_periods.create!(start_date: 2.months.from_now, end_date: 3.months.from_now)
+
+        grading_period_group.destroy
+        expect(grading_period_1).to be_deleted
+        expect(grading_period_1).to_not be_destroyed
+        expect(grading_period_2).to be_deleted
+        expect(grading_period_2).to_not be_destroyed
+      end
     end
   end
 

spec/models/grading_period_spec.rb

@@ -137,5 +137,13 @@ describe GradingPeriod do
       expect(grading_period.current?).to be false
     end
   end
+
+  context "Soft deletion" do
+    let(:account) { Account.create }
+    let(:group) { account.grading_period_groups.create }
+    let(:creation_arguments) { { start_date: 1.week.ago, end_date: 2.weeks.from_now } }
+    subject { group.grading_periods }
+    include_examples "soft deletion"
+  end
 end
 

spec/shared_examples/soft_deletion.rb

@@ -0,0 +1,41 @@
+shared_examples "soft deletion" do
+  let(:first)         { subject.create creation_arguments }
+  let(:second)        { subject.create creation_arguments }
+  let(:active_scope)  { subject.active }
+
+  describe "workflow" do
+    it "defaults to active" do
+      expect(first.active?).to be true
+    end
+
+    it "is deleted after destroy is called" do
+      first.destroy
+      expect(first.deleted?).to be true
+    end
+  end
+
+  describe "#active" do
+    let!(:destroy_the_second_active_object) { second.destroy }
+    it "includes active grading_periods" do
+      expect(active_scope).to include first
+    end
+
+    it "does not include inactive grading_periods" do
+      expect(active_scope).to_not include second
+    end
+  end
+
+  describe "#destroy" do
+    it "marks deleted periods workflow_state as deleted" do
+      first.destroy
+
+      expect(first.workflow_state).to eq "deleted"
+    end
+
+    # Use Mocha to test this.
+    it "calls save"
+    it "calls save! if destroy! was called"
+
+    it "triggers destroy callbacks"
+  end
+end

spec/spec_helper.rb

@@ -165,7 +165,11 @@ Mocha::ObjectMethods.instance_methods.each do |m|
   RUBY
 end
 
-Dir.glob("#{File.dirname(__FILE__).gsub(/\\/, "/")}/factories/*.rb").each { |file| require file }
+factories = "#{File.dirname(__FILE__).gsub(/\\/, "/")}/factories/*.rb"
+Dir.glob(factories).each { |file| require file }
+
+examples = "#{File.dirname(__FILE__).gsub(/\\/, "/")}/shared_examples/*.rb"
+Dir.glob(examples).each { |file| require file }
 
 def pend_with_bullet
   if defined?(Bullet) && Bullet.enable?