simplify SimplyVersioned option storage and add callbacks

SimplyVersioned plugin needs callbacks for the addition of the
submission_versions index table. Whenever a submission version is
saved, we will want to add an entry to the submission_versions table
in addition to the versions table. This patch provides hooks for
those callbacks.

Test Plan:
- Versioned anything should still work

Refs CNVS-2802

Change-Id: I02a862c5894a98ece8a586330f264680751ab52f
Reviewed-on: https://gerrit.instructure.com/19520
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Jacob Fugal <jacob@instructure.com>
Reviewed-by: Duane Johnson <duane@instructure.com>
Product-Review: Jacob Fugal <jacob@instructure.com>

app/models/learning_outcome_result.rb

@@ -49,13 +49,7 @@ class LearningOutcomeResult < ActiveRecord::Base
       nil
     end
   end
-  
-  def changes_worth_versioning?
-    !(self.changes.keys - [
-      "updated_at",
-    ]).empty?
-  end
-  
+
   def save_to_version(attempt)
     current_version = self.versions.current.try(:model)
     if current_version.try(:attempt) && attempt < current_version.attempt

app/models/submission.rb

@@ -140,8 +140,18 @@ class Submission < ActiveRecord::Base
 
   has_a_broadcast_policy
 
-  simply_versioned :explicit => true
-  
+  simply_versioned :explicit => true, :when => lambda{ |model|
+    model.turnitin_data_changed? || (model.changes.keys - [
+      "updated_at",
+      "processed",
+      "process_attempts",
+      "changed_since_publish",
+      "grade_matches_current_submission",
+      "published_score",
+      "published_grade"
+    ]).present?
+  }
+
   set_policy do
     given {|user| user && user.id == self.user_id }
     can :read and can :comment and can :make_group_comment and can :submit
@@ -1064,19 +1074,11 @@ class Submission < ActiveRecord::Base
   def turnitin_data_changed!
     @turnitin_data_changed = true
   end
-  
-  def changes_worth_versioning?
-    !(self.changes.keys - [
-      "updated_at", 
-      "processed", 
-      "process_attempts", 
-      "changed_since_publish",
-      "grade_matches_current_submission",
-      "published_score",
-      "published_grade"
-    ]).empty? || @turnitin_data_changed
+
+  def turnitin_data_changed?
+    @turnitin_data_changed
   end
-  
+
   def get_web_snapshot
     # This should always be called in the context of a delayed job
     return unless CutyCapt.enabled?

spec/selenium/helpers/wiki_and_tiny_common.rb

@@ -67,9 +67,7 @@ require File.expand_path(File.dirname(__FILE__) + '/../common')
   end
 
   def create_wiki_page(title, hfs, edit_roles)
-    p = @course.wiki.wiki_pages.create(:title => title, :hide_from_students => hfs, :editing_roles => edit_roles, :notify_of_update => true)
-    p.save!
-    p
+    @course.wiki.wiki_pages.create(:title => title, :hide_from_students => hfs, :editing_roles => edit_roles, :notify_of_update => true)
   end
 
   def select_all_wiki
@@ -129,4 +127,4 @@ require File.expand_path(File.dirname(__FILE__) + '/../common')
     wait_for_ajax_requests
     get "/courses/#{@course.id}/wiki" #can't just wait for the dom, for some reason it stays in edit mode
     wait_for_ajax_requests
-  end
+  end

spec/selenium/teacher_wiki_and_tiny_spec.rb

@@ -99,7 +99,7 @@ describe "Wiki pages and Tiny WYSIWYG editor" do
       keep_trying_until { f("#page_history").should be_displayed }
       f('#page_history').click
 
-      ff('a[title]').length.should == 3
+      ff('a[title]').length.should == 2
     end
 
 
@@ -167,7 +167,7 @@ describe "Wiki pages and Tiny WYSIWYG editor" do
       f('.history').click
       wait_for_ajax_requests
 
-      ff('a[title]').length.should == 3
+      ff('a[title]').length.should == 2
     end
   end
 end

vendor/plugins/simply_versioned/lib/simply_versioned.rb

@@ -16,6 +16,17 @@ module SoftwareHeretics
           super( "Keys: #{keys.join( "," )} are not known by SimplyVersioned" )
         end
       end
+
+      DEFAULTS = {
+        :keep => nil,
+        :automatic => true,
+        :exclude => [],
+        :explicit => false,
+        # callbacks
+        :when => nil,
+        :on_create => nil,
+        :on_update => nil
+      }
       
       module ClassMethods
         
@@ -24,11 +35,22 @@ module SoftwareHeretics
         # the +versions+ association.
         #
         # Options:
-        # +limit+ - specifies the number of old versions to keep (default = nil, never delete old versions)
+        # +keep+ - specifies the number of old versions to keep (default = nil, never delete old versions)
         # +automatic+ - controls whether versions are created automatically (default = true, save versions)
         # +exclude+ - specify columns that will not be saved (default = [], save all columns)
-        # +explicit+ - INSTRUCTURE: explicit versioning keeps the last version up to date, but doesn't
-        #              automatically create new versions (default = false)
+        #
+        # Additional INSTRUCTURE options:
+        # +explicit+ - explicit versioning keeps the last version up to date,
+        #              but doesn't automatically create new versions (default = false)
+        # +when+ - callback to indicate whether an instance needs a version
+        #          saved or not. if present, the model is passed to the
+        #          callback which should return true or false, true indicating
+        #          a version should be saved. if absent, versions are saved if
+        #          any attribute other than updated_at is changed.
+        # +on_create+ - callback to allow additional changes to a new version
+        #               that's about to be saved.
+        # +on_update+ - callback to allow additional changes to an updated (see
+        #               +explicit+ parameter) version that's about to be saved.
         #
         # To save the record without creating a version either set +versioning_enabled+ to false
         # on the model before calling save or, alternatively, use +without_versioning+ and save
@@ -36,15 +58,11 @@ module SoftwareHeretics
         #
         
         def simply_versioned( options = {} )
-          bad_keys = options.keys - [:keep,:automatic,:exclude,:explicit]
+          bad_keys = options.keys - SimplyVersioned::DEFAULTS.keys
           raise SimplyVersioned::BadOptions.new( bad_keys ) unless bad_keys.empty?
           
-          options.reverse_merge!( {
-            :keep => nil,
-            :automatic => true,
-            :exclude => [],
-            :explicit => false
-          })
+          options.reverse_merge!(DEFAULTS)
+          options[:exclude] = Array( options[ :exclude ] ).map( &:to_s )
           
           has_many :versions, :order => 'number DESC', :as => :versionable, :dependent => :destroy, :extend => VersionsProxyMethods
           # INSTRUCTURE: Added to allow quick access to the most recent version
@@ -53,17 +71,8 @@ module SoftwareHeretics
           before_save :check_if_changes_are_worth_versioning
           after_save :simply_versioned_create_version
           
-          cattr_accessor :simply_versioned_keep_limit
-          self.simply_versioned_keep_limit = options[:keep]
-          
-          cattr_accessor :simply_versioned_save_by_default
-          self.simply_versioned_save_by_default = options[:automatic]
-          
-          cattr_accessor :simply_versioned_excluded_columns
-          self.simply_versioned_excluded_columns = Array( options[ :exclude ] ).map( &:to_s )
-
-          cattr_accessor :simply_versioned_explicitly_versioned
-          self.simply_versioned_explicitly_versioned = options[:explicit]
+          cattr_accessor :simply_versioned_options
+          self.simply_versioned_options = options
           
           class_eval do
             def versioning_enabled=( enabled )
@@ -73,7 +82,7 @@ module SoftwareHeretics
             def versioning_enabled?
               enabled = self.instance_variable_get( :@simply_versioned_enabled )
               if enabled.nil?
-                enabled = self.instance_variable_set( :@simply_versioned_enabled, self.simply_versioned_save_by_default )
+                enabled = self.instance_variable_set( :@simply_versioned_enabled, self.simply_versioned_options[:automatic] )
               end
               enabled
             end
@@ -170,8 +179,9 @@ module SoftwareHeretics
         # on the before_create to see if the changes are worth
         # creating a new version for
         def check_if_changes_are_worth_versioning
-          @changes_are_worth_versioning = !self.respond_to?(:changes_worth_versioning?) || self.changes_worth_versioning?
-          @changes_are_worth_versioning = false if self.changes.keys == ["updated_at"]
+          @changes_are_worth_versioning = simply_versioned_options[:when] ?
+            simply_versioned_options[:when].call(self) :
+            (self.changes.keys - ["updated_at"]).present?
           true
         end
         
@@ -180,12 +190,17 @@ module SoftwareHeretics
             # INSTRUCTURE
             if @versioning_explicitly_enabled || @changes_are_worth_versioning
               @changes_are_worth_versioning = nil
-              if simply_versioned_explicitly_versioned && !@simply_versioned_explicit_enabled && versioned?
+              if simply_versioned_options[:explicit] && !@simply_versioned_explicit_enabled && versioned?
                 version = self.versions.current
-                version.yaml = self.attributes.except( *simply_versioned_excluded_columns ).to_yaml
+                version.yaml = self.attributes.except( *simply_versioned_options[:exclude] ).to_yaml
+                simply_versioned_options[:on_update].try(:call, self, version)
                 version.save
-              elsif self.versions.create( :yaml => self.attributes.except( *simply_versioned_excluded_columns ).to_yaml )
-                self.versions.clean_old_versions( simply_versioned_keep_limit.to_i ) if simply_versioned_keep_limit
+              else
+                version = self.versions.create( :yaml => self.attributes.except( *simply_versioned_options[:exclude] ).to_yaml )
+                if version
+                  simply_versioned_options[:on_create].try(:call, self, version)
+                  self.versions.clean_old_versions( simply_versioned_options[:keep].to_i ) if simply_versioned_options[:keep]
+                end
               end
             end
           end

vendor/plugins/simply_versioned/test/simply_versioned_test.rb

@@ -283,7 +283,7 @@ class SimplyVersionedTest < FixturedTestCase
   end
   
   def test_should_exclude_columns
-    assert_equal ["trouble"], Saproling.simply_versioned_excluded_columns
+    assert_equal ["trouble"], Saproling.simply_versioned_options[:exclude]
     
     sylvia = Saproling.create!( :name => 'Sylvia', :trouble => "big" )
     
@@ -296,7 +296,7 @@ class SimplyVersionedTest < FixturedTestCase
     klass.module_eval <<-DEFN
       simply_versioned :exclude => :some_column
     DEFN
-    assert_equal ['some_column'], klass.simply_versioned_excluded_columns
+    assert_equal ['some_column'], klass.simply_versioned_options[:exclude]
   end
   
   def test_should_report_version_number

vendor/plugins/simply_versioned/test/test_helper.rb

@@ -3,7 +3,7 @@ TEST_FOLDER = File.dirname( __FILE__ )
 
 $:.unshift( File.join( TEST_FOLDER, '..', 'lib' ) )
 
-HOST_APP_FOLDER = File.expand_path( ENV['HOST_APP'] || File.join( TEST_FOLDER, '..', '..', '..' ) )
+HOST_APP_FOLDER = File.expand_path( ENV['HOST_APP'] || File.join( TEST_FOLDER, '..', '..', '..', '..' ) )
 puts "Host application: #{HOST_APP_FOLDER}"
 
 require 'test/unit'