2
\$\begingroup\$

Recently I rewrote some methods/functions written by a previous developer mostly due to the fact that the previous implementation overdid the whole 'OO' thing, and used inheritance inappropriately. The code also leaked memory like crazy in production long running tasks, so my rewritten code is a response to that, by being more functional, less OO, less state, hopefully less leakage.

# This is app/lib/lonestar_bigcommerce.rb
# to use these functions,
# do require 'lonestar_bigcommerce.rb'
#    include LonestarBigcommerce
# from anywhere in the rails application
module LonestarBigcommerce
  include Temporary

  # enumerable, supports .to_a(), each(), etc
  # https://thoughtbot.com/blog/modeling-a-paginated-api-as-a-lazy-stream
  def bigcommerce_api_v3_get_customers_by_shop(shop, options = {})
    params = options.fetch(:params, {})
    raise if params['page'] || params['limit'] # prevent manual pagination attempt
    pages = options[:pages]  # depth to crawl, how many pages

    Enumerator.new do |yielder|
      params[:page] = 1
      params[:limit] = '250'
      loop do
        r1 = bigcommerce_get_request('https://api.bigcommerce.com' \
                                     "/stores/#{shop.uid}/v3/customers?" +
                                     params.to_query,
                                     shop)
        raise StopIteration unless bigcommerce_api_v3_collection_result_valid?(r1)

        JSON.parse(r1.body)['data'].map { |item| yielder << item }
        # theres one possible optimization here, to save one http reqeust
        raise StopIteration if pages && params[:page] >= pages
        params[:page] += 1
      end
    end.lazy
  end

  def hubspot_api_v1_get_contact_by_vid(vid, shop, options = {})
    params = options.fetch(:params, {})
    r1 = hubspot_get_request("https://api.hubapi.com/contacts/v1/contact/vid/#{vid}/profile", shop)
    JSON.parse(r1.body)
  end

  def hubspot_api_v1_get_contact_by_email(email, shop, options = {})
    params = options.fetch(:params, {})
    r1 = hubspot_get_request("https://api.hubapi.com/contacts/v1/contact/email/#{email}/profile", shop)
    JSON.parse(r1.body)
  end

  # @private
  def default_hubspot_property_group(shop)
    {
      :name =>  "#{shop.uid}customerdata",
      :displayName => "#{shop.domain} Customer Data"
    }
  end

  def hubspot_api_v1_update_contact_by_email(property, email, shop, options = {})
    recursive = options.fetch(:recursive, nil)
    group_name = options.fetch(:group_name, nil)
    group_label = options.fetch(:group_label, nil)
    retries ||=0
    hubspot_post_request(
      "https://api.hubapi.com/contacts/v1/contact/email/#{email}/profile",
      shop,
      body: { :properties => [ property ] }
    )
  rescue RuntimeError => err
    if recursive && err.message=~/PROPERTY_DOESNT_EXIST/ && retries < 2
      hubspot_api_v1_create_property(
        property[:property], shop, recursive: true,
        group_name: group_name, group_label: group_label
      )
      retries+=1
      retry
    end
    raise
  end

  def hubspot_api_v1_create_property(label, shop, options = {})
    retries ||= 0
    name = options.fetch(:name, label.parameterize.underscore)
    group_name = options.fetch(:group_name, default_hubspot_property_group(shop)[:name])
    group_label = options.fetch(:group_label, default_hubspot_property_group(shop)[:displayName])
    recursive = options.fetch(:recursive, nil)
    r1 = hubspot_post_request(
      'https://api.hubapi.com/properties/v1/contacts/properties',
      shop,
      body: {
              :name => name,
              :label => label,
              :description => options.fetch(:description, nil),
              :groupName => group_name,
              :type => "string",
              :fieldType => "text"
            }
    )
    JSON.parse(r1.body)
  rescue RuntimeError => err
    if recursive && err.message =~ /property group.*does not exist/
      retries+=1
      hubspot_api_v1_create_group(group_name, group_label, shop)
      retry if retries < 2
    end
    raise
  end

  def hubspot_api_v1_create_group(group_name, group_label, shop)
    r1 = hubspot_post_request(
      'https://api.hubapi.com/properties/v1/contacts/groups',
      shop,
      body: { :name => group_name, :dislpayName => group_label })
    JSON.parse(r1.body)
  end

  def maybe_cache_http_requests(cassette, options = {})
    options.slice!(:record)
    if ENV['CACHE_HTTP_REQUESTS'] == '1'
      require 'vcr'
      VCR.configure do |config|
        config.cassette_library_dir = 'vcr_cassettes'
        config.hook_into :webmock
        config.allow_http_connections_when_no_cassette = true
      end
      VCR.use_cassette(cassette, options) do
        yield
      end
    else
      yield
    end
  end

  def log(str)
    Rails.logger.debug "DB8899 #{str}"
  end

  # @private
  def bigcommerce_get_request(url, shop, _options = {})
    r1 = net_http_request(
      url,
      headers:
        bigcommerce_headers.merge(bigcommerce_headers_for_shop(shop))
    )
    describe_bigcommerce_response(r1)
    r1
  end

  # @private
  def hubspot_get_request(url, shop, _options = {})
    retries ||= 0
    r1 = net_http_request(
      url,
      headers: hubspot_headers.merge(hubspot_headers_for_shop(shop))
    )
    describe_hubspot_response(r1)
    r1
  rescue RuntimeError => e
    retries += 1
    if e.message =~ /is expired\! expiresAt/ && retries < 2
      renew_hubspot_access_token(shop)
      retry
    end
    raise
  end

  def renew_hubspot_access_token(shop)
    #`curl -d 'grant_type=refresh_token&client_id=a&client_secret=a&refresh_token=a'
    # -X POST -v https://api.hubapi.com/oauth/v1/token`
    log medium_indent + ">>POST https://api.hubapi.com/oauth/v1/token"
    r1 = JSON.parse(
      Net::HTTP.post_form(
        URI('https://api.hubapi.com/oauth/v1/token'),
        { "grant_type" => "refresh_token",
          "client_id" => ENV['HUBSPOT_CLIENT_ID'],
          "client_secret" => ENV['HUBSPOT_CLIENT_SECRET'],
          "refresh_token" => shop.hubspot_site.refresh_token }
      ).body
    )
    shop.hubspot_site.update_columns(
      access_token: r1["access_token"],
      expires_at: Time.now + r1["expires_in"].to_i
    )
  end

  # @private
  def hubspot_post_request(url, shop, options = {})
    retries ||= 0
    r1 = net_http_request(
      url,
      headers: hubspot_headers.merge(hubspot_headers_for_shop(shop)),
      body: options[:body],
      type: :post,
      allowed_responses: [Net::HTTPOK, Net::HTTPNoContent]
    )
    describe_hubspot_response(r1)
    r1
  rescue RuntimeError => e
    retries += 1
    if e.message =~ /is expired\! expiresAt/ && retries < 2
      renew_hubspot_access_token(shop)
      retry
    end
    raise
  end

  # @private
  def hubspot_put_request(url, shop, options = {})
    r1 = net_http_request(
      url,
      shop,
      headers: hubspot_headers.merge(hubspot_headers_for_shop(shop)),
      body: options[:body],
      type: :put
    )
    describe_hubspot_response(r1)
    JSON.parse(r1)
  end

  # @private
  def bigcommerce_headers
    {
      'accept': 'application/json',
      'content-type': 'application/json'
    }
  end

  # @private
  def bigcommerce_headers_for_shop(shop)
    {
      'x-auth-client': ENV['BIGCOMMERCE_CLIENT_ID'],
      'x-auth-token': shop.bigcommerce_token
    }
  end

  # @private
  def describe_bigcommerce_response(response)
    msg = "#{http_logging_prefix_in} #{response.message} (#{response.class})"
    if response.class == Net::HTTPConflict
      msg = "#{http_logging_prefix_in} NOK (existed)"
    elsif response.class == Net::HTTPNoContent
      msg = "#{http_logging_prefix_in} OK"
    end
    log medium_indent + msg
    msg
  end

  # @private
  def medium_indent
    ' ' * 10
  end

  # @private
  def http_logging_prefix_in
    '<<'
  end

  # @private
  def http_logging_prefix_out
    '>>'
  end

  # @private
  # irb(main):004:0> "What is your birthday?".parameterize.underscore
  #=> "what_is_your_birthday"
  def strip_spaces(str)
    str.parameterize.underscore
  end

  # @private
  def describe_hubspot_response(response)
    msg = "#{http_logging_prefix_in} #{response.message} (#{response.class})"
    if response.class == Net::HTTPConflict
      msg = "#{http_logging_prefix_in} NOK (existed)"
    elsif response.class == Net::HTTPNoContent
      msg = "#{http_logging_prefix_in} OK"
    end
    log medium_indent + msg
    msg
  end

  # @private
  def hubspot_headers
    {
      :accept => 'application/json',
      :'Content-Type' => 'application/json'
    }
  end

  # @private
  def hubspot_headers_for_shop(shop)
    { :Authorization => "Bearer #{shop.hubspot_site.access_token}" }
  end

  # @private
  def net_http_request(url, options = {})
    retries ||= 0
    type = options.fetch(:type, :get)
    allowed_responses = options.fetch(:allowed_responses, [Net::HTTPOK] )
    body = options.fetch(:body, {})
    headers = options.fetch(:headers, [])
    uri = URI(url)
    http = Net::HTTP.new(uri.host, uri.port)
    http.use_ssl = true
    http.verify_mode = OpenSSL::SSL::VERIFY_NONE
    request = Net::HTTP::Get.new(uri)  if type == :get
    request = Net::HTTP::Post.new(uri) if type == :post
    request = Net::HTTP::Put.new(uri)  if type == :put
    request.body = body.to_json if (type != :get && body.present?)
    headers.each { |key, value| request[key.to_s.downcase] = value }
    log medium_indent + http_logging_prefix_out + type.to_s.upcase + ':' + url
    r1 = http.request(request)
    raise "#{r1.inspect} #{r1&.body}" unless allowed_responses.include?(r1.class) #r1.is_a?(Net::HTTPOK)
    r1
  rescue Net::HTTPTooManyRequests
    retries += 1
    log medium_indent + '!!!!!!!! Net::HTTPTooManyRequests, will retry'
    sleep_a_bit
    retry if retries <= 3
    raise
  end

  # @private
  def sleep_a_bit
    sleep rand(0..1.0)
  end

  # @private
  def bigcommerce_api_v3_collection_result_valid?(result)
    result.class == Net::HTTPOK && JSON.parse(result.body)['meta']['pagination']['count'].positive?
  end
end

```


Tests
```ruby
# this is test/lonestar_bigcommerce_test.rb,
# the test file that tests the corresponding app/lib/ file.
# to run tests do bundle exec rake test
require 'test_helper'

require 'lonestar_bigcommerce'

class DummyClass
  include LonestarBigcommerce
end

class LonestarBigcommerceTest < ActiveSupport::TestCase
  test 'bigcommerce_api_v3_get_customers_by_shop_a' do
    # when theres less than a pageful of results
    # ends up making one get request, plus one to realize theres no more
    # limit 250   freeman "count 16 total": 16, total_pages": 1
    VCR.use_cassette("#{self.class}_#{__method__}") do
      dummy = DummyClass.new
      r1 = dummy.bigcommerce_api_v3_get_customers_by_shop(create(:shop, :rbxboxing), params: { :'name:like' => 'freeman' })
      r2 = r1.to_a
      assert_equal 16, r2.size
      assert_equal 'Aviva', r2[0]['first_name']
    end
  end

  test 'bigcommerce_api_v3_get_customers_by_shop_b' do
    # when theres more than 1 page of results
    # ends up making two get requests, plus one to realize theres no more
    # limit 250   ree  count 250 "total": 257, total_pages": 2
    VCR.use_cassette("#{self.class}_#{__method__}") do
      dummy = DummyClass.new
      r1 = dummy.bigcommerce_api_v3_get_customers_by_shop(create(:shop, :rbxboxing), params: { :'name:like' => 'ree' })
      assert_equal 257, r1.to_a.size
    end
  end

  test 'bigcommerce_api_v3_get_customers_by_shop_c' do
    # when theres zero results
    VCR.use_cassette("#{self.class}_#{__method__}") do
      dummy = DummyClass.new
      r1 = dummy.bigcommerce_api_v3_get_customers_by_shop(create(:shop, :rbxboxing), params: { :'name:like' => 'Nonexistantname' })
      assert_equal 0, r1.to_a.size
    end
  end

  test 'hubspot_api_v1_get_contact_by_vid' do
    VCR.use_cassette("#{self.class}_#{__method__}") do
      dummy = DummyClass.new
      r1 = dummy.hubspot_api_v1_get_contact_by_vid(32051, create(:shop, :rbxboxing) )
      assert_equal 32051, r1["vid"]
      assert_equal 125, r1["properties"].size
    end
  end

  test 'hubspot_api_v1_get_contact_by_email' do
    VCR.use_cassette("#{self.class}_#{__method__}") do
      dummy = DummyClass.new
      r1 = dummy.hubspot_api_v1_get_contact_by_email('ka@gmail.com', create(:shop, :rbxboxing) )
      assert_equal 'kalish.nd@gmail.com', r1["properties"]["email"]["value"]
      assert_equal 7164201, r1["vid"]
    end
  end

  #    when Events::OrderCreated::TOPIC
  #      Events::OrderCreated.new(event).process!
  #    when Events::OrderUpdated::TOPIC
  #      Events::OrderUpdated.new(event).process!
  #    when Events::CustomerCreated::TOPIC
  #  ->  Events::CustomerCreated.new(event).process!
  #    when Events::CustomerUpdated::TOPIC
  #      Events::CustomerUpdated.new(event).process!
  #    when Events::CustomerFullSync::TOPIC
  #      Events::CustomerFullSync.new(event).process!
  #    when Events::CartAbandoned::TOPIC
  #      Events::CartAbandoned.new(event).process!
  test 'custom fields handled during background processing scenario a' do
    VCR.use_cassette("#{self.class}_#{__method__}") do
      event = create(:event, :rbxboxing, :topic_store_customer_created)
      Events::CustomerCreated.new(event).process!
    end
  end

  def test_hubspot_api_v1_create_group
    VCR.use_cassette("#{self.class}_#{__method__}") do
      shop = create(:shop, :hsfbc)
      r1 = DummyClass.new.hubspot_api_v1_create_group("example002", "Example Name", shop)
      assert_equal ({"portalId"=>6300907, "name"=>"example002", "displayOrder"=>8}), r1
    end
  end

  # fails when group already exists
  def test_hubspot_api_v1_create_group_fails_001
    VCR.use_cassette("#{self.class}_#{__method__}") do
      shop = create(:shop, :hsfbc)
      err = assert_raises do
        r1 = DummyClass.new.hubspot_api_v1_create_group("example001", "Example Name", shop)
      end
      assert_match /Net::HTTPConflict 409/, err.message
    end
  end

  def test_hubspot_api_v1_create_property
    VCR.use_cassette("#{self.class}_#{__method__}") do
      shop = create(:shop, :hsfbc)
      r1 = DummyClass.new.hubspot_api_v1_create_property(
        "example001",
        shop,
        group_name: 'example001')
      assert_match '{"name"=>"example001", "label"=>"example001", "description"=>"", "groupName"=>"example001", ', r1.to_s
    end
  end

  # fails when group does not exist
  def test_hubspot_api_v1_create_property_fails_001
    VCR.use_cassette("#{self.class}_#{__method__}") do
      shop = create(:shop, :hsfbc)
      err = assert_raises do
        DummyClass.new.hubspot_api_v1_create_property(
          "example001",
          shop)
      end
      assert_match /property group 1h2smorqcustomerdata does not exist/, err.message
    end
  end

  # update contact success
  def test_hubspot_api_v1_update_contact
    VCR.use_cassette("#{self.class}_#{__method__}") do
      r1 = DummyClass.new.hubspot_api_v1_update_contact_by_email(
        { :property => "example003", :value => "example003value"},
        'ch@gr.com',
        create(:shop, :hsfbc)
      )
      assert_equal Net::HTTPNoContent, r1.class
    end
  end

  # update contact fails when property not exist
  def test_hubspot_api_v1_update_contact_fails_001
    VCR.use_cassette("#{self.class}_#{__method__}") do
      err = assert_raises do
        DummyClass.new.hubspot_api_v1_update_contact_by_email(
          { :property => "example004", :value => "example004value"},
          'ch@gr.com',
          create(:shop, :hsfbc)
        )
      end
      assert_match /error":"PROPERTY_DOESNT_EXIST/, err.message
    end
  end

  # update contact success when recursive needed and supplied
  def test_hubspot_api_v1_update_contact_recursive
    VCR.use_cassette("#{self.class}_#{__method__}") do
      r1 = DummyClass.new.hubspot_api_v1_update_contact_by_email(
        { :property => "example004", :value => "example004value"},
        'ch@gr.com',
        create(:shop, :hsfbc),
        recursive: true,
        group_name: 'example005',
        group_label: 'example005 label'
      )
      assert r1.is_a?(Net::HTTPNoContent)
    end
  end


end
\$\endgroup\$
2
\$\begingroup\$
def bigcommerce_api_v3_get_customers_by_shop(shop, options = {})

Since you are only extracting a couple values from options here, consider using keyword params instead. It makes it more obvious what specific keys are used by the method, without needing to dig into the method body.


raise if params['page'] || params['limit'] # prevent manual pagination attempt

You should raise a specific error here, or at least include a message. Currently, you'll just get a RuntimeError with no message, and would need go to that line in the source code to figure out what is the actual error


r1 = hubspot_get_request("https://api.hubapi.com/contacts/v1/contact/vid/#{vid}/profile", shop)

It's kinda nice to have URLs like this as constants. Of course, you do have to interpolate a value in there, but it can be done using the % operator as well (see https://stackoverflow.com/questions/28556946/percent-operator-string-interpolation):

# Note I changed #{} here to %{}
HUBSPOT_USER_PROFILE_PATH = "https://api.hubapi.com/contacts/v1/contact/vid/%{vid}/profile"
# ...
r1 = hubspot_get_request(
  HUBSPOT_USER_PROFILE_PATH % { vid: vid },
  shop
)

:name =>  "#{shop.uid}customerdata",

You should use the shorter syntax for symbol keys, name: "#{shop.uid}customerdata",


recursive = options.fetch(:recursive, nil)

Using fetch here is pointless, just use options[:recursive] which will return nil if the key isn't found anyway. But still, it's better to use keyword params anyway.


This is as far as I'm gonna get right now, this is quite a long program and I have to head to work. But, those issues I outlined above are applicable in many parts of the program.

\$\endgroup\$
  • \$\begingroup\$ hey thanks for reviewing the code \$\endgroup\$ – american-ninja-warrior Dec 30 '19 at 14:04

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.