explicitly version submissions refs #4189

Extend simply_versioned to do explicit versioning, meaning keep
the last version up-to-date on save, but only create a new version
when explicitly asked to.

Change-Id: I922dcd5d6fe559a5a656c452131b535f55a2e3fa
Reviewed-on: https://gerrit.instructure.com/3894
Tested-by: Hudson <hudson@instructure.com>
Reviewed-by: JT Olds <jt@instructure.com>

app/models/assignment.rb

@@ -873,7 +873,7 @@ class Assignment < ActiveRecord::Base
     homeworks = []
     ts = Time.now.to_s
     students.each do |student|
-      homework = self.find_or_create_submission(student)
+      homework = Submission.find_or_initialize_by_assignment_id_and_user_id(self.id, student.id)
       homework.grade_matches_current_submission = homework.score ? false : true
       homework.attributes = opts.merge({
         :attachment => nil,
@@ -884,7 +884,7 @@ class Assignment < ActiveRecord::Base
       })
       homework.submitted_at = Time.now
 
-      homework.with_versioning(true) do
+      homework.with_versioning(:explicit => true) do
         group ? homework.save_without_broadcast : homework.save
       end
       context_module_action(student, :submitted)

app/models/submission.rb

@@ -109,6 +109,15 @@ class Submission < ActiveRecord::Base
       SQL
     end
   end
+  trigger.after(:insert) do |t|
+    t.where('#{Submission.needs_grading_conditions("NEW")}') do
+      <<-SQL
+      UPDATE assignments
+      SET needs_grading_count = needs_grading_count + 1
+      WHERE id = NEW.assignment_id;
+      SQL
+    end
+  end
   
   attr_reader :suppress_broadcast
   attr_reader :group_broadcast_submission
@@ -117,7 +126,7 @@ class Submission < ActiveRecord::Base
   
   has_a_broadcast_policy
 
-  simply_versioned
+  simply_versioned :explicit => true
   
   set_policy do
     given {|user| user && user.id == self.user_id }

db/migrate/20110527155754_create_trigger_submissions_insert.rb

@@ -0,0 +1,25 @@
+# This migration was auto-generated via `rake db:generate_trigger_migration'.
+# While you can edit this file, any changes you make to the definitions here
+# will be undone by the next auto-generated trigger migration.
+
+class CreateTriggerSubmissionsInsert < ActiveRecord::Migration
+  def self.up
+    create_trigger("submissions_after_insert_row_tr", :generated => true, :compatibility => 1).
+        on("submissions").
+        after(:insert) do |t|
+      t.where(" NEW.submission_type IS NOT NULL AND ( NEW.score IS NULL OR NOT NEW.grade_matches_current_submission OR NEW.workflow_state IN ('submitted', 'pending_review') ) ") do
+        <<-SQL_ACTIONS
+      UPDATE assignments
+      SET needs_grading_count = needs_grading_count + 1
+      WHERE id = NEW.assignment_id;
+        SQL_ACTIONS
+      end
+    end
+  end
+
+  def self.down
+    drop_trigger("submissions_after_insert_row_tr", "submissions")
+
+    drop_trigger("submissions_after_insert_row_when_new_submission_type_is_not_tr", "submissions")
+  end
+end

spec/apis/v1/submissions_api_spec.rb

@@ -29,7 +29,9 @@ describe SubmissionsApiController, :type => :integration do
     end
     sub.workflow_state = 'submitted'
     yield(sub) if block_given?
-    update_with_protected_attributes!(sub, { :submitted_at => Time.at(@submit_homework_time), :created_at => Time.at(@submit_homework_time) }.merge(opts))
+    sub.with_versioning(:explicit => true) do
+      update_with_protected_attributes!(sub, { :submitted_at => Time.at(@submit_homework_time), :created_at => Time.at(@submit_homework_time) }.merge(opts))
+    end
     sub.versions(true).each { |v| Version.update_all({ :created_at => v.model.created_at }, { :id => v.id }) }
     sub
   end

spec/models/assignment_spec.rb

@@ -36,6 +36,7 @@ describe Assignment do
     @assignment.submissions.size.should eql(1)
     @submission = @assignment.submissions.first
     @submission.user_id.should eql(@user.id)
+    @submission.versions.length.should eql(1)
   end
   
   it "should be able to grade a submission" do
@@ -49,6 +50,7 @@ describe Assignment do
     @submission.should eql(s[0])
     @submission.score.should eql(10.0)
     @submission.user_id.should eql(@user.id)
+    @submission.versions.length.should eql(1)
   end
 
   it "should update needs_grading_count when submissions transition state" do
@@ -96,12 +98,18 @@ describe Assignment do
     s2 = @a.grade_student(@user, :grade => "10")
     s.reload
     s.should eql(s2[0])
+    # there should only be one version, even though the grade changed
+    s.versions.length.should eql(1)
     s2[0].state.should eql(:graded)
   end
   
-  it "should be versioned" do
-    assignment_model
-    @a.should be_respond_to(:versions)
+  it "should create a new version for each submission" do
+    setup_assignment_without_submission
+    @a.submit_homework(@user)
+    @a.submit_homework(@user)
+    @a.submit_homework(@user)
+    @a.reload
+    @a.submissions.first.versions.length.should eql(3)
   end
 
   describe "infer_due_at" do

spec/models/general_model_spec.rb

@@ -49,7 +49,8 @@ describe 'Models' do
         ActiveRecord::SessionStore::Session,
         Delayed::Backend::ActiveRecord::Job,
         Version,
-        Story
+        Story,
+        Woozel
       ]
     (ignore_classes << AddThumbnailUuid::Thumbnail) rescue nil
     (ignore_classes << CustomField) rescue nil

vendor/plugins/simply_versioned/lib/simply_versioned.rb

@@ -27,6 +27,8 @@ module SoftwareHeretics
         # +limit+ - 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)
         #
         # 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
@@ -34,13 +36,14 @@ module SoftwareHeretics
         #
         
         def simply_versioned( options = {} )
-          bad_keys = options.keys - [:keep,:automatic,:exclude]
+          bad_keys = options.keys - [:keep,:automatic,:exclude,:explicit]
           raise SimplyVersioned::BadOptions.new( bad_keys ) unless bad_keys.empty?
           
           options.reverse_merge!( {
             :keep => nil,
             :automatic => true,
-            :exclude => []
+            :exclude => [],
+            :explicit => false
           })
           
           has_many :versions, :order => 'number DESC', :as => :versionable, :dependent => :destroy, :extend => VersionsProxyMethods
@@ -58,6 +61,9 @@ module SoftwareHeretics
           
           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]
           
           class_eval do
             def versioning_enabled=( enabled )
@@ -107,18 +113,31 @@ module SoftwareHeretics
         # Invoke the supplied block passing the receiver as the sole block argument with
         # versioning enabled or disabled depending upon the value of the +enabled+ parameter
         # for the duration of the block.
-        def with_versioning( enabled, &block )
+        def with_versioning( enabled = true)
           versioning_was_enabled = self.versioning_enabled?
+          explicit_versioning_was_enabled = @simply_versioned_explicit_enabled
+          explicit_enabled = false
+          if enabled.is_a?(Hash)
+            opts = enabled
+            enabled = true
+            explicit_enabled = true if opts[:explicit]
+          end
           self.versioning_enabled = enabled
+          @simply_versioned_explicit_enabled = explicit_enabled
           # INSTRUCTURE: always create a version if explicitly told to do so
           @versioning_explicitly_enabled = enabled == true
           begin
-            block.call( self )
+            yield self
           ensure
             @versioning_explicitly_enabled = nil
             self.versioning_enabled = versioning_was_enabled
+            @simply_versioned_explicit_enabled = explicit_enabled
           end
         end
+
+        def without_versioning(&block)
+          with_versioning(false, &block)
+        end
         
         def unversioned?
           self.versions.nil? || self.versions.size == 0
@@ -144,7 +163,7 @@ module SoftwareHeretics
             self.versions.current.number
           end
         end
-        
+
         protected
         
         # INSTRUCTURE: If defined on a method, allow a check
@@ -161,7 +180,11 @@ module SoftwareHeretics
             # INSTRUCTURE
             if @versioning_explicitly_enabled || @changes_are_worth_versioning
               @changes_are_worth_versioning = nil
-              if self.versions.create( :yaml => self.attributes.except( *simply_versioned_excluded_columns ).to_yaml )
+              if simply_versioned_explicitly_versioned && !@simply_versioned_explicit_enabled && versioned?
+                version = self.versions.current
+                version.yaml = self.attributes.except( *simply_versioned_excluded_columns ).to_yaml
+                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
               end
             end

vendor/plugins/simply_versioned/spec_canvas/simply_versioned_spec.rb

@@ -0,0 +1,65 @@
+require File.expand_path(File.dirname(__FILE__)+'/../../../../spec/apis/api_spec_helper')
+
+class Woozel< ActiveRecord::Base
+  simply_versioned :explicit => true
+end
+
+Woozel.establish_connection(:adapter => 'sqlite3', :database => ':memory:')
+
+describe 'simply_versioned' do
+  describe "explicit versions" do
+    before do
+      Woozel.connection.create_table :woozels, :force => true do |t|
+        t.string :name
+      end
+    end
+    after do
+      Woozel.connection.drop_table :woozels
+    end
+
+    it "should create the first version on save" do
+      woozel = Woozel.new(:name => 'Eeyore')
+      woozel.should_not be_versioned
+      woozel.save!
+      woozel.should be_versioned
+      woozel.versions.length.should eql(1)
+      woozel.versions.current.model.name.should eql('Eeyore')
+    end
+
+    it "should keep the last version up to date for each save" do
+      woozel = Woozel.create!(:name => 'Eeyore')
+      woozel.should be_versioned
+      woozel.versions.length.should eql(1)
+      woozel.versions.current.model.name.should eql('Eeyore')
+      woozel.name = 'Piglet'
+      woozel.save!
+      woozel.versions.length.should eql(1)
+      woozel.versions.current.model.name.should eql('Piglet')
+    end
+
+    it "should create a new version when asked to" do
+      woozel = Woozel.create!(:name => 'Eeyore')
+      woozel.name = 'Piglet'
+      woozel.with_versioning(:explicit => true, &:save!)
+      woozel.versions.length.should eql(2)
+      woozel.versions.first.model.name.should eql('Eeyore')
+      woozel.versions.current.model.name.should eql('Piglet')
+    end
+
+    it 'should not create a new version when not explicitly asked to' do
+      woozel = Woozel.create!(:name => 'Eeyore')
+      woozel.name = 'Piglet'
+      woozel.with_versioning(&:save!)
+      woozel.versions.length.should eql(1)
+      woozel.versions.current.model.name.should eql('Piglet')
+    end
+
+    it 'should not update the last version when not versioning' do
+      woozel = Woozel.create!(:name => 'Eeyore')
+      woozel.name = 'Piglet'
+      woozel.without_versioning(&:save!)
+      woozel.versions.length.should eql(1)
+      woozel.versions.current.model.name.should eql('Eeyore')
+    end
+  end
+end