Merge pull request #6 from tubergen/dirty

Add ActiveRecord::Dirty functionality to resources

Author: pmen

lib/protip/resource.rb

@@ -13,6 +13,9 @@
 require 'active_model/translation'
 require 'active_model/errors'
 
+require 'active_model/attribute_methods' # ActiveModel::Dirty depends on this
+require 'active_model/dirty'
+
 require 'forwardable'
 
 require 'protip/error'
@@ -129,14 +132,15 @@ def self.collection(resource_class, action, method, message, response_type)
     include ActiveModel::Validations
     include ActiveModel::Conversion
 
+    include ActiveModel::Dirty
+
     included do
       extend ActiveModel::Naming
       extend ActiveModel::Translation
       extend Forwardable
 
       def_delegator :@wrapper, :message
       def_delegator :@wrapper, :as_json
-      def_delegator :@wrapper, :assign_attributes
     end
     module ClassMethods
 
@@ -166,12 +170,23 @@ def resource(actions:, message:, query: nil)
         @message = message
         @message.descriptor.each do |field|
           def_delegator :@wrapper, :"#{field.name}"
-          def_delegator :@wrapper, :"#{field.name}="
           if ::Protip::Wrapper.matchable?(field)
             def_delegator :@wrapper, :"#{field.name}?"
           end
+
+          define_method "#{field.name}=" do |new_value|
+            old_wrapped_value = @wrapper.send(field.name)
+            @wrapper.send("#{field.name}=", new_value)
+            new_wrapped_value = @wrapper.send(field.name)
+
+            # needed for ActiveModel::Dirty
+            send("#{field.name}_will_change!") if new_wrapped_value != old_wrapped_value
+          end
         end
 
+        # needed for ActiveModel::Dirty
+        define_attribute_methods @message.descriptor.map(&:name)
+
         # Validate arguments
         actions.map!{|action| action.to_sym}
         (actions - %i(show index create update destroy)).each do |action|
@@ -258,6 +273,12 @@ def initialize(message_or_attributes = {})
       super()
     end
 
+    def assign_attributes(attributes)
+      # the resource needs to call its own setters so that fields get marked as dirty
+      attributes.each { |field_name, value| send("#{field_name}=", value) }
+      nil # return nil to match ActiveRecord behavior
+    end
+
     def message=(message)
       @wrapper = Protip::Wrapper.new(message, self.class.converter)
     end
@@ -271,6 +292,7 @@ def save
         else
           create!
         end
+        changes_applied
       rescue Protip::UnprocessableEntityError => error
         success = false
         error.errors.messages.each do |message|
@@ -298,5 +320,13 @@ def attributes
     def errors
       @errors ||= ActiveModel::Errors.new(self)
     end
+
+    private
+
+    # needed for ActiveModel::Dirty
+    def changes_applied
+      @previously_changed = changes
+      @changed_attributes.clear
+    end
   end
 end

lib/protip/wrapper.rb

@@ -28,7 +28,9 @@ def respond_to?(name)
     def method_missing(name, *args)
       if (name =~ /=$/ && field = message.class.descriptor.detect{|field| :"#{field.name}=" == name})
         raise ArgumentError unless args.length == 1
-        set field, args[0]
+        attributes = {}.tap { |hash| hash[field.name] = args[0] }
+        assign_attributes attributes
+        args[0] # return the input value (to match ActiveRecord behavior)
       elsif (name =~ /\?$/ && field = message.class.descriptor.detect{|field| self.class.matchable?(field) && :"#{field.name}?" == name})
         raise ArgumentError unless args.length == 1
         matches? field, args[0]
@@ -98,9 +100,11 @@ def assign_attributes(attributes)
         if field.type == :message && !converter.convertible?(field.subtype.msgclass)
           if value.is_a?(field.subtype.msgclass) # If a message, set it directly
             set(field, value)
-          else # If a hash, pass it through to the nested message
+          elsif value.is_a?(Hash) # If a hash, pass it through to the nested message
             wrapper = get(field) || build(field.name) # Create the field if it doesn't already exist
             wrapper.assign_attributes value
+          else # If value is a simple type (e.g. nil), set the value directly
+            set(field, value)
           end
         # Otherwise, if the field is a convertible message or a simple type, we set the value directly
         else

protip.gemspec

@@ -1,7 +1,7 @@
 # encoding: utf-8
 Gem::Specification.new do |spec|
   spec.name          = 'protip'
-  spec.version       = '0.12.4'
+  spec.version       = '0.13.0'
   spec.summary       = 'ActiveModel resources backed by protocol buffers'
   spec.licenses      = ['MIT']
   spec.homepage      = 'https://github.com/AngelList/protip'

test/functional/protip/resource_test.rb

@@ -1,6 +1,7 @@
 require 'test_helper'
 
 require 'protip'
+require 'google/protobuf/wrappers'
 
 describe 'Protip::Resource (functional)' do
 

test/test_helper.rb

@@ -2,4 +2,4 @@
 require 'mocha/mini_test'
 require 'webmock/minitest'
 
-require 'minitest/pride'
+require 'minitest/pride'

test/unit/protip/resource_test.rb

@@ -112,17 +112,17 @@ class << self
       end
 
       it 'checks with the converter when setting message types' do
-        converter.expects(:convertible?).once.with(nested_message_class).returns(false)
+        converter.expects(:convertible?).at_least_once.with(nested_message_class).returns(false)
         resource = resource_class.new
         assert_raises(ArgumentError) do
           resource.nested_message = 5
         end
       end
 
       it 'converts message types to and from their Ruby values when the converter allows' do
-        converter.expects(:convertible?).times(2).with(nested_message_class).returns(true)
+        converter.expects(:convertible?).at_least_once.with(nested_message_class).returns(true)
         converter.expects(:to_message).once.with(6, nested_message_class).returns(nested_message_class.new number: 100)
-        converter.expects(:to_object).once.with(nested_message_class.new number: 100).returns 'intern'
+        converter.expects(:to_object).at_least_once.with(nested_message_class.new number: 100).returns 'intern'
 
         resource = resource_class.new
         resource.nested_message = 6
@@ -434,13 +434,41 @@ def self.it_converts_query_parameters
           attrs = {id: 2}
           assert_equal resource_message_class.new(attrs), resource_class.new(attrs).message
         end
+      end
+    end
 
-        it 'delegates to #assign_attributes on its wrapper object when a hash is given' do
-          attrs = {id: 3}
-          Protip::Wrapper.any_instance.expects(:assign_attributes).once.with({id: 3})
-          resource_class.new(attrs)
+    describe 'attribute writer' do
+      before do
+        resource_class.class_exec(resource_message_class) do |resource_message_class|
+          resource actions: [], message: resource_message_class
         end
       end
+
+      it 'delegates writes to the wrapper object' do
+        resource = resource_class.new
+        test_string = 'new'
+        Protip::Wrapper.any_instance.expects(:string=).with(test_string)
+        resource.string = test_string
+      end
+
+      it 'marks the resource and attribute as changed if the value is changed' do
+        resource = resource_class.new string: 'original'
+        resource.string = 'new'
+        assert resource.changed?, 'resource should be marked as changed'
+        assert resource.string_changed?, 'string field should be marked as changed'
+      end
+
+      it 'does not mark the resource and attribute as changed if the value is not changed' do
+        resource = resource_class.new string: 'original'
+        resource.send :changes_applied # clear the changes
+        # establish that the changes were cleared
+        assert !resource.changed?, 'resource should be not marked as changed'
+        assert !resource.string_changed?, 'string field should not be marked as changed'
+
+        resource.string = 'original'
+        assert !resource.changed?, 'resource should be not marked as changed'
+        assert !resource.string_changed?, 'string field should not be marked as changed'
+      end
     end
 
     describe '#assign_attributes' do
@@ -449,11 +477,17 @@ def self.it_converts_query_parameters
           resource actions: [], message: resource_message_class
         end
       end
-      it 'delegates to #assign_attributes on its wrapper object' do
+
+      it 'calls the attribute writer for each attribute' do
         resource = resource_class.new
+        test_string = 'whodunnit'
+        resource.expects(:string=).with(test_string)
+        resource.assign_attributes(string: test_string)
+      end
 
-        Protip::Wrapper.any_instance.expects(:assign_attributes).once.with(string: 'whodunnit').returns('boo')
-        assert_equal 'boo', resource.assign_attributes(string: 'whodunnit')
+      it 'returns nil' do
+        resource = resource_class.new
+        assert_nil resource.assign_attributes(string: 'asdf')
       end
     end
 
@@ -495,6 +529,14 @@ def self.it_converts_query_parameters
           resource.save
           assert_equal response, resource.message
         end
+
+        it 'marks changes as applied' do
+          client.stubs(:request).returns(response)
+          resource = resource_class.new(string: 'time')
+          assert resource.string_changed?, 'string should initially be changed'
+          assert resource.save
+          assert !resource.string_changed?, 'string should no longer be changed after save'
+        end
       end
 
       describe 'for an existing record' do
@@ -528,6 +570,14 @@ def self.it_converts_query_parameters
           resource.save
           assert_equal response, resource.message
         end
+
+        it 'marks changes as applied' do
+          client.stubs(:request).returns(response)
+          resource = resource_class.new id: 5, string: 'new_string'
+          assert resource.string_changed?, 'string should initially be changed'
+          assert resource.save
+          assert !resource.string_changed?, 'string should no longer be changed after save'
+        end
       end
 
       describe 'when validation errors are thrown' do
@@ -567,6 +617,13 @@ def self.it_converts_query_parameters
         it 'returns false' do
           refute @resource.save, 'save returned true'
         end
+
+        it 'does not mark changes as applied' do
+          @resource.string = 'new_string'
+          assert @resource.string_changed?, 'string should initially be changed'
+          refute @resource.save
+          assert @resource.string_changed?, 'string should still be changed after unsuccessful save'
+        end
       end
     end
 

test/unit/protip/wrapper_test.rb

@@ -216,6 +216,13 @@ module Protip::WrapperTest # namespace for internal constants
           wrapper.assign_attributes inner: {value: 50, note: 'noted'}
         end
 
+        it "sets message fields to nil when they're assigned nil" do
+          wrapped_message.inner = inner_message_class.new(value: 60)
+          assert wrapped_message.inner
+          wrapper.assign_attributes inner: nil
+          assert_nil wrapped_message.inner
+        end
+
         it 'allows messages to be assigned directly' do
           message = inner_message_class.new
           wrapper.assign_attributes inner: message
@@ -341,7 +348,7 @@ module Protip::WrapperTest # namespace for internal constants
       end
     end
 
-    describe '#set' do
+    describe 'attribute writer' do # generated via method_missing?
       it 'does not convert simple fields' do
         converter.expects(:convertible?).never
         converter.expects(:to_message).never
@@ -351,34 +358,36 @@ module Protip::WrapperTest # namespace for internal constants
       end
 
       it 'converts convertible messages' do
-        converter.expects(:convertible?).with(inner_message_class).once.returns(true)
+        converter.expects(:convertible?).at_least_once.with(inner_message_class).returns(true)
         converter.expects(:to_message).with(40, inner_message_class).returns(inner_message_class.new(value: 30))
 
         wrapper.inner = 40
         assert_equal inner_message_class.new(value: 30), wrapper.message.inner
       end
 
       it 'removes message fields when assigning nil' do
-        converter.expects(:convertible?).never
+        converter.expects(:convertible?).at_least_once.with(inner_message_class).returns(false)
         converter.expects(:to_message).never
 
         wrapper.inner = nil
         assert_equal nil, wrapper.message.inner
       end
 
       it 'raises an error when setting inconvertible messages' do
-        converter.expects(:convertible?).with(inner_message_class).once.returns(false)
+        converter.expects(:convertible?).at_least_once.with(inner_message_class).returns(false)
         converter.expects(:to_message).never
         assert_raises ArgumentError do
           wrapper.inner = 'cannot convert me'
         end
       end
 
       it 'passes through messages without checking whether they are convertible' do
+        converter.expects(:convertible?).once.returns(true)
+        message = inner_message_class.new(value: 50)
+
         converter.expects(:convertible?).never
         converter.expects(:to_message).never
-
-        wrapper.inner = inner_message_class.new(value: 50)
+        wrapper.inner = message
         assert_equal inner_message_class.new(value: 50), wrapper.message.inner
       end
 
@@ -410,6 +419,11 @@ module Protip::WrapperTest # namespace for internal constants
         wrapper.number = m
         assert_equal :ONE, wrapper.number
       end
+
+      it 'returns the input value' do
+        input_value = 'str'
+        assert_equal input_value, wrapper.send(:string=, input_value)
+      end
     end
 
     describe '#matches?' do