Driver info suggestions

ride_diff.diff

-      # if it's a Fleet provider but not virtual provider
-      # refactor: have a flag on provider that indicates that there is additional info in Fleet database
-      update_driver(params) if (provider.fleet_provider?(booking_channel.bit_mask) && driver && need_update_driver?(params))
-
-      set_driver_tags(params)
-
+      info = RideDriverInfo.fetch(self,params)
+      params[:driver_phone_number] = info.driver_phone_number
+      params[:can_call_driver] = info.can_call_driver
+      params[:driver_photo_enabled] = info.driver_photo_enabled

Any time you're passing the same argument to many methods within a class,
you probably have a class that you're missing which should be wrapping those values.

With these changes, I can easily see where the values are being updated, and it's only at one level.
and RideDriverInfo can be read-only for the params hash.
we can seperate the part which looks at the hash from the part which updates it,
which is a big reduction in complexity.

ride_driver_info.rb

# Any time you're passing the same argument to many methods within a class,
# you probably have a class that you're missing which should be wrapping those values.
#
# This wraps the params[:driver_phone_number],
# but also the behaviour which depends on that - update_driver_from_fleet.
#
# It also encapsulates the check_fleet_database feature flag access, so that ride
# need not care about that either.

class RideDriverInfo
  
  def self.fetch(ride,params)
    new(ride,params).info
  end
  
  # This is the last method you'll have to look at which uses a 'params' hash.
  # They're nice syntax, but limit their parsing to one level, or one method,
  # or raise an error when they are supposed to have a fixed set of keys.
  # 
  # You can also use the parameter defaults to show what keys and values should be provided.
  #
  # FWIW, I do the many-line argument thing, as well as trailing-commas, to minimize syntax's
  # effect on the diffs in commits. [1]
  def initialize(ride,params={:driver_phone_number => nil})
    @ride = ride
    @driver_phone_number = params[:driver_phone_number]
    update_driver_from_fleet
    @info = DriverInfo.new(
      driver_phone_number, 
      driver_photo_enabled, 
      can_call_driver
    )
  end

  attr_reader :info # An outvar.

  def driver_photo_enabled
    ride.driver_photo_feature_enabled?
  end
  
  # Favor methods and guard clauses over large if statements with variables.
  # It saves one line here, but reduces the duplicate assign.
  # Was:
  #   if params[:driver_phone_number].blank?
  #     can_call_driver = false
  #   else
  #     can_call_driver = ride.can_call_driver?(params[:driver_phone_number])
  #   end
  def can_call_driver
    return false if driver_phone_number.blank?
    ride.can_call_driver?(driver_phone_number)
  end
  
  #private
  
  attr_reader :ride
  attr_reader :driver_phone_number
  
  # Judgement call: it's a toss up whether it's nicer to have 
  # the label of the field here for quick reference,
  # or to have all accesses 'encapsulated' by a class.
  # This seemed a happy medium.
  def enabled?
    # could be: 
    # ride.provider.provider_feature_enabled?('check_fleet_database', ride.booking_channel.bit_mask)
    # ride.provider.fleet_provider?(ride.booking_channel.bit_mask)
    # OR
    # ride.provider_feature_enabled?('check_fleet_database') # see below.
    "Toggle via 'check_fleet_database'"
    ProviderFeatures[ride].fleet_provider?
  end
  
  # The NullObject pattern is a great way to avoid returning nils, 
  # and therefore to avoid causing their associated NoMethodError
  # The null object still needs to be checked for in this instance (see self.driver_from_fleet)
  # but that's because we don't have any default.
  class NoDriver < Hash
  end
  
  # Wrap external calls with your own interface.
  # 1. You won't have to change callers if they change. Here for instance, 
  #    if response['driver']['phone'] changes, there's one place where {:driver_phone_number} needs
  #    to be updated.
  # 2. You can test the service much more directly, i.e. without writing to the db or firing ride events;
  #    in both the console, and your unit tests. 
  #    You can have one very small set of unit tests on RideDriverInfo 
  #    with VCR cassettes to test the calls to fleet magic, 
  #    then a seperate unit test for RideDriverInfo to test the params behaviour. 
  #    That one can mock out the behaviour of this one method 
  #    (since you're now dictating the interface) using mocha.
  #    Finally, your tests for Ride#event_assigned can mock out all of the RideDriverInfo.call, 
  #    to just return an appropriate DriverInfo, to check that db values are assigned correctly.
  def self.driver_from_fleet(ride)
    request = {
      :provider_keyname => ride.provider.key_name, 
      :driver_code => ride.driver.code,
    }
    driver = FleetMagicClient.show_driver(request)
    return NoDriver.new unless driver.response? 
    {
      :driver_phone_number => driver.response['driver']['phone'],
    }
  end
  
  def need_update_driver?
    driver_phone_number.blank?
  end
  
  # By using an object instance, we can wrap all these accesses and params accesses, 
  # and by using command methods, 
  # we can have guard clauses rather than big if statements
  # Compare: 
  #   update_driver(params) if (provider.fleet_provider?(booking_channel.bit_mask) && driver && need_update_driver?(params))
  def update_driver_from_fleet
    return unless enabled?
    return unless ride.driver
    return unless need_update_driver?
    
    driver = self.class.driver_from_fleet(ride)
    return if NoDriver === driver
    
    @driver_phone_number = driver[:driver_phone_number]
  end
end

## [1]
# Which of these is it easier to see the change in?
# This:
#   - @info = DriverInfo.new(driver_phone_number, driver_photo_enabled, can_call_driver)
#   + @info = DriverInfo.new(driver_phone_number, driver_photo_enabled, can_call_driver, can_message_driver)
# vs
#     @info = DriverInfo.new(
#        driver_phone_number, 
#        driver_photo_enabled, 
#   -    can_call_driver)
#   +    can_call_driver,
#   +    can_message_driver)
#
# And:
#     { 
#       :provider_keyname => ride.provider.key_name, 
#   -    :driver_code => ride.driver.code
#   +    :driver_code => ride.driver.code,
#   +    :driver_name => ride.driver.name
#     }
# vs
#   {
#     :provider_keyname => ride.provider.key_name, 
#     :driver_code => ride.driver.code,
#   + :driver_name => ride.driver.name,
#   }

ride_driver_info_test.rb

# Similar to the Law of Demeter, unit tests shouldn't have to mock more than the
# class's direct collaborators. The way this is achieved is with telescoping tests.
# Take this test case. This tests ride's event_assigned method, but mocks:
# Ride#event_assigned(params) -...> Ride#update_driver(params) -> FleetMagicClient.show_driver -...> HTTP Call * This *
context "features for fleet providers" do
  setup do 
    @ride = Ride.find(35)
    assert @ride.provider.fleet_provider?(@ride.booking_channel.bit_mask)
  end
  should "try to update driver phone number if missing" do
    driver = Driver.find_by_code("5")
    VCR.use_cassette("fleet_call_for_driver", :record => :all) do
      @ride.event_assigned({:driver_name => "Rider Nowat", :driver_id => driver.id})
      # FleetMagicClient.expects(:show_driver).at_least_once.returns(true)
    end
  end
end


# So we unwrap and mock each message send as it passes from collaborator to subcollaborator.
# Each test which mocks a call has a corresponding test in the collaborator
# which supports that call with those arguments, 
# showing that such a call will in fact work, 
# but focused on the actual behaviour that the collaborating class implements.
class RideTest < ActiveSupport::TestCase
  context "features for fleet providers" do
    setup do 
      @ride = Ride.find(35)
    end
    context "event_assigned" do
      context "driver_phone_number missing" do
        setup do
          RideDriverInfo.
          expects(:call).
          with(@ride,{
             :driver_phone_number => "17037771111"
          }).
          returns(DriverInfo.new("17037771111", #driver_phone_number
            true, #driver_photo_enabled
            true #can_call_driver
           )
        end
        should "apply updated driver phone numbers" do
          driver = Driver.find_by_code("5")
          @ride.event_assigned({:driver_name => "Rider Nowat", :driver_id => driver.id})
          result = [ 
              @ride.driver_photo_feature_enabled?,
              @ride.can_call_driver?,
              @ride.driver_phone_number,
            ]
          good = [
            true,
            true,
            "17037771111",
            ]
          assert_equal(good, result)
        end
    end
  end
end
class RideDriverInfoTest < ActiveSupport::TestCase
  context "features for fleet providers" do
    setup do 
      @ride = Ride.find(35)
      assert @ride.provider.fleet_provider?(@ride.booking_channel.bit_mask)
      provider = @ride.provider
    end
    context "RideDriverInfo.call" do
      context "with driver phone number missing" do
        setup do
          RideDriverInfo.
            expects(:driver_from_fleet).
            with(@ride).
            returns({
              :driver_phone_number => "17037771111",
            })
        end
        should "pass along updated driver phone number" do
          info = RideDriverInfo.call(@ride, {
            :driver_id => 5, 
            :driver_name => "Rider Nowat",
          })
          good = DriverInfo.new(
            "17037771111",
            true,
            true
          )
          assert_equal(good, info)
        end
      end
    end
    context "RideDriverInfo.driver_from_fleet" do
      
      # TODO: This could even go down a level further, and mock FleetMagicClient,
      # then a seperate FleetMagicClient test could be made which uses 
      # the VCR cassette, instead of here.
      should "receive a successful response from the server" do
        VCR.use_cassette("fleet_call_for_driver", :record => :all) do
          info = RideDriverInfo.driver_from_fleet(@ride)
          good = {
            :driver_phone_number => "17037771111",
            }
          assert_equal(good, info)
        end
      end
    end
  end
end

z_driver_info.rb

# Just a data object.
class DriverInfo < Struct.new(
   :driver_phone_number,
   :driver_photo_enabled, 
   :can_call_driver, 
   )
end

z_provider_features.rb

# How has no one made this, given how many instances there are in Ride?

class Ride < ActiveRecord::Base
  def provider_feature_enabled?(key, bit_mask=booking_channel.bit_mask)
    provider.provider_feature_enabled?(key,bit_mask)
  end
end

# ride.provider_feature_enabled?('check_fleet_database')
# ride.provider_feature_enabled?('bounce_payment')

# It'd be even better if all this logic were in a class you could just ask.

# ProviderFeatures[ride].fleet_provider?
# ProviderFeatures[ride].bounce_provider?
# ProviderFeatures[sr,search_context].auto_best_fleet_select?
# ProviderFeatures[provider,bit_mask].booking_fee?
require 'forwardable'
class ProviderFeatures
  extend Forwardable
  def self.[](obj,context=nil)
    case obj
    when Ride then
      new(obj.provider,obj.booking_channel.bit_mask)
    when ProviderSearchResult then
      new(obj.provider,context.booking_channel_bit_mask)
    when Provider then
      new(obj, context)
    else
      raise ArgumentError.new("ProviderFeatures[] does not understand #{obj} nor #{context}")
    end
  end
  
  def initialize(provider,bit_mask)
    @provider = provider
    @bit_mask = bit_mask
  end
  attr_reader :bit_mask
  def_delgator :@provider, :provider_feature_enabled?
  
  def auto_best_fleet_select?
    self.provider_feature_enabled?('auto_best_fleet_select', bit_mask)
  end
  
  def bounce_provider?
    self.provider_feature_enabled?('bounce_payment', bit_mask)
  end

  def booking_fee?
    self.provider_feature_enabled?('booking_fee', bit_mask)
  end

  def booking_fee_waived?
    self.provider_feature_enabled?('booking_fee_waived', bit_mask)
  end

  def calling_enabled?
    self.provider_feature_enabled?('call_driver', bit_mask)
  end

  def driver_photo_enabled?
    self.provider_feature_enabled?('driver_photo', bit_mask)
  end

  def require_cc?
    self.provider_feature_enabled?('require_cc', bit_mask)
  end

  def zero_auth_enabled?
    self.provider_feature_enabled?('zero_auth', bit_mask)
  end

  def eta_required_for_booking?
    self.provider_feature_enabled?('eta_required_for_booking', bit_mask)
  end
end

Can I propose a tweak? Replace the type switching in https://gist.github.com/tsnow/9182426#file-z_provider_features-rb-L22-L31.

1. Extract the object creation to a module.

Multiple types (potentially more later?) share a variant of this functionality, so it's a good candidate. I'd actually use names other than RideStrategy and ProviderStrategy but it's too early for me to brain; this is a hand-wave we can talk about later if you want. In general, assume names throughout are up for grabs.

module Aspects
  module ProviderFeature
    module RideStrategy
      def get_provider_features(context=nil)
        ::ProviderFeatures.new(self.provider, self.booking_channel)
      end
    end

    module ProviderStrategy
      def get_provider_features(context=nil)
        ::ProviderFeatures.new(self, context)
      end
    end
  end
end

2. Include the relevant strategy in your model.

Objects opt-in by including a strategy.

class Ride
  include Aspects::ProviderFeature::RideStrategy
  attr_reader :provider, :booking_channel
end

class Provider
  include Aspects::ProviderFeature::ProviderStrategy
end

# oh, snap!
class MockProvider
  include Aspects::ProviderFeature::ProviderStrategy
end

3. Tell the duck to quack.

The caller is more "confident" (channeling one's inner Avdi) and assumes it can call the method it needs; if it can't, it deals with that accordingly.

class ProviderFeatures
  def self.[](obj,context=nil)
    obj.get_provider_features(context)
    rescue NameError
      raise ArgumentError.new("ProviderFeatures[] does not understand #{obj} nor #{context}")
  end

  def initialize(provider, bitmask)
    # ...
  end
end

4. Make it rain.

"I can't believe they takin' Warren's wealth..."

puts ProviderFeatures[Ride.new]         # #<ProviderFeatures:0x007fdca30ae100>
puts ProviderFeatures[Provider.new]     # #<ProviderFeatures:0x007fdca30adea8>
puts ProviderFeatures[MockProvider.new] # #<ProviderFeatures:0x007fa2388ad2c0>
puts ProviderFeatures[Object.new]       # bar.rb:5:in `rescue in []':...

---

@anachronistic you're right, I should be favoring objects with a strategy protocol over a case statement in ProviderFeatures.

It does seem like a judgement call as to whether to put the methods directly on the incoming classes, or
to include modules from a common namespace.

class Ride
  def get_provider_features(context=nil)
    ProviderFeatures.new(self.provider,self.booking_channel.bit_mask)
  end
end

Vs. the Ride with the module included above.

It's kinda a toss up as to which is clearer.

Yeah, you absolutely could drop it directly onto the model. My motivation is primarily making the models smaller / making the individual modules easy to unit test. You could make a convincing argument for both sides without much effort though.