Add ability to send targeted company shares #191

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

ismell commented Dec 9, 2013

This introduces a breaking change to the add_company_share method. We
now parse the response body so the user does not have to parse the xml
response body. This change should not impact many users seeing as the
add_company_share method was just added a while ago and there hasn't
been a release since.

Usage:

response = client.add_company_share("2414183", {
  :comment => "Testing, 1, 2, 3",
  :targets => {
    :geos => ['as', 'eu'],
    :jobFunc => ['acct', 'bd']
  }
puts "ID: #{response.body.update_key}"
puts "URL: #{response.body.update_url}"
Raul E Rangel Raul E Rangel
Add ability to send targeted company shares
This introduces a breaking change to the add_company_share method. We
now parse the response body so the user does not have to parse the xml
response body. This change should not impact many users seeing as the
add_company_share method was just added a while ago and there hasn't
been a release since.

Usage:

    response = client.add_company_share("2414183", {
      :comment => "Testing, 1, 2, 3",
      :targets => {
        :geos => ['as', 'eu'],
        :jobFunc => ['acct', 'bd']
      }
    puts "ID: #{response.body.update_key}"
    puts "URL: #{response.body.update_url}"
+
+ define_method :load_template do |template|
+ return cache[template] if cache[template]
+ mutex.synchronize do
@hexgnu

hexgnu Dec 12, 2013

Owner

During your testing did you run into race conditions?

@ismell

ismell Dec 12, 2013

Contributor

Any time I modify a shared variable I want to make sure it's serialized. Though in this case if multiple threads read and compile the same template it's not really the end of the world.

+ cache = {}
+ mutex = Mutex.new
+
+ define_method :load_template do |template|
@hexgnu

hexgnu Dec 12, 2013

Owner

why use define_method instead of def?

@ismell

ismell Dec 12, 2013

Contributor

I wanted to use a closure to keep the cache and mutex private.

+require 'hashie'
+
+module LinkedIn
+ class TemplateBinding < ::Hashie::Mash
@hexgnu

hexgnu Dec 12, 2013

Owner

A TemplateBinding is a HashieMash? This seems like it violates SRP to me.

@ismell

ismell Dec 12, 2013

Contributor

The real reason I went with a HashieMash is to allow the user to send in either symbols or strings for the parameter keys. This allows the template to use content to read either param[:content] or param["content"].

@hexgnu

hexgnu Dec 12, 2013

Owner

Ahh yea that seems legit

@@ -9,10 +9,26 @@ def add_share(share)
post(path, defaults.merge(share).to_json, "Content-Type" => "application/json")
end
+ # Creates a company share
@hexgnu

hexgnu Dec 12, 2013

Owner

dude you freaking rock writing this documentation.

@@ -74,6 +90,13 @@ def post_group_discussion(group_id, discussion)
post(path, discussion.to_json, "Content-Type" => "application/json")
end
+ private
+
+ def hashify response
@hexgnu

hexgnu Dec 12, 2013

Owner

This method confuses me a bit. It takes a response (from net/http?) and changes the body? It seems dangerous to be mutating it like that.

@ismell

ismell Dec 12, 2013

Contributor

The reason I did it is because the response for this method will be XML. I figured instead of forcing the developer to check the headers and handle both xml or json that we can just return a hash like we do for any of the query api methods.

@hexgnu

hexgnu Dec 12, 2013

Owner

Well I think I'm weary of this because it really only gets used once and seems to be for the XML -> Hash case. Id say either inline it or rename it to something more specific.

@ismell

ismell Dec 12, 2013

Contributor

Or should I just get rid of it and have the developer check the headers and handle the parsing like before?

@hexgnu

hexgnu Dec 12, 2013

Owner

That's tough to say... I think there's a huge advantage in convenience for having hashes to play around with but I will say that sometimes what we want is something to run xpath / css queries on. What do you think about wrapping up xml responses in it's own object? XML doesn't really map to hash as easily as JSON does.

This is just a hack right now but something like this?

require 'forwardable'
require 'multi_xml'

class XMLResponse
  extend Forwardable
  attr_reader :xml
  def_delegators :to_hash, *(Hash.new.methods - Object.methods)

  def initialize(xml)
    @xml = xml
  end

  def to_hash
    @hash ||= MultiXml.parse(self.xml)
  end
end
+ result_hash = ::MultiXml.parse(xml_string)
+
+ # Drop off the root element
+ new(result_hash[result_hash.keys.first])
@hexgnu

hexgnu Dec 12, 2013

Owner
result_hash.fetch(result_hash.keys.first, DEFAULT_RESP)

I'd prefer to minimize the nils in this code if we can

+ end
+
+ def self.from_response(response)
+ if response['x-li-format'] == 'xml' or /\bxml\b/.match response['Content-Type']
@hexgnu

hexgnu Dec 12, 2013

Owner

Is there a Content-Type that is not application/xml? If so I'd make a test for that

@ismell

ismell Dec 12, 2013

Contributor

text/xml is also a valid xml mime-type. But the real reason I did it is that sometimes the Content-Type header contains ;charset=utf-8. This was a quick and easy way to match the xml header. Though in theory the x-li-format should always come back.

@hexgnu

hexgnu Dec 12, 2013

Owner

yes very good point. could you write a test making sure that's tested? Right now I think you're testing for application/xml.

@@ -0,0 +1,37 @@
+require 'erb'
+require 'ostruct'
+require 'hashie'
@hexgnu

hexgnu Dec 12, 2013

Owner

hashie should already be loaded I believe, and ostruct isn't used

@ismell

ismell Dec 12, 2013

Contributor

Oh good catch!

@@ -4,6 +4,7 @@ require File.expand_path('../lib/linked_in/version', __FILE__)
Gem::Specification.new do |gem|
gem.add_dependency 'hashie', ['>= 1.2', '< 2.1']
gem.add_dependency 'multi_json', '~> 1.0'
+ gem.add_dependency 'multi_xml'
@hexgnu

hexgnu Dec 12, 2013

Owner

'multi_xml', '~>0.5.5' instead of leaving it open in case they introduce breaking changes to their api

@ismell

ismell Dec 12, 2013

Contributor

I left it open because the only real method it offers is .parse. If they break then it's not multi_xml anymore ;) Would a '< 1.0' be more appropriate?

@hexgnu

hexgnu Dec 12, 2013

Owner

lol yea < 1.0 sounds fine to me. I doubt they'll change much but you never know.

@@ -192,6 +185,44 @@
response.code.should == "201"
end
+ it "should be able to share a new company status" do
@hexgnu

hexgnu Dec 12, 2013

Owner

awesome tests! I appreciate you not just checking for instance of Linkedin::Mash 😄

Contributor

ismell commented Dec 12, 2013

Thanks for the awesome code review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment