Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to send targeted company shares #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion lib/linked_in/api/update_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,26 @@ def add_share(share)
post(path, defaults.merge(share).to_json, "Content-Type" => "application/json")
end

# Creates a company share
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dude you freaking rock writing this documentation.

#
# Returns an HttpResponse object with a body containing update_key and update_url
#
# @param company_id [String] The company id
# @param share [Hash] Share data
#
# @option share [String] comment Post must contain comment and/or (content/title and content/submitted-url). Max length is 700 characters.
# @option share [Hash] content Content Hash
# @option content [String] title Post must contain comment and/or (content/title and content/submitted-url). Max length is 200 characters.
# @option content [String] submitted-url Post must contain comment and/or (content/title and content/submitted-url).
# @option content [String] submitted-image-url Invalid without (content/title and content/submitted-url).
# @option content [String] description Max length of 256 characters.
# @option share [Hash] targets Hash containing target_code => ['target_value', ...]
#
# @return [HttpResponse] Response
def add_company_share(company_id, share)
path = "/companies/#{company_id}/shares"
defaults = {:visibility => {:code => "anyone"}}
post(path, defaults.merge(share).to_json, "Content-Type" => "application/json")
hashify post(path, render(:company_share, defaults.merge(share)), 'x-li-format' => 'xml', "Content-Type" => "application/xml")
end

def follow_company(company_id)
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

response.body = Mash.from_response response
response
end

end

end
Expand Down
1 change: 1 addition & 0 deletions lib/linked_in/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ class Client
include Api::QueryMethods
include Api::UpdateMethods
include Search
include Template

attr_reader :consumer_token, :consumer_secret, :consumer_options

Expand Down
6 changes: 3 additions & 3 deletions lib/linked_in/helpers/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,13 @@ def raise_errors(response)
# in the HTTP answer (thankfully).
case response.code.to_i
when 401
data = Mash.from_json(response.body)
data = Mash.from_response(response)
raise LinkedIn::Errors::UnauthorizedError.new(data), "(#{data.status}): #{data.message}"
when 400
data = Mash.from_json(response.body)
data = Mash.from_response(response)
raise LinkedIn::Errors::GeneralError.new(data), "(#{data.status}): #{data.message}"
when 403
data = Mash.from_json(response.body)
data = Mash.from_response(response)
raise LinkedIn::Errors::AccessDeniedError.new(data), "(#{data.status}): #{data.message}"
when 404
raise LinkedIn::Errors::NotFoundError, "(#{response.code}): #{response.message}"
Expand Down
17 changes: 17 additions & 0 deletions lib/linked_in/mash.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
require 'hashie'
require 'multi_json'
require 'multi_xml'

module LinkedIn
class Mash < ::Hashie::Mash
Expand All @@ -10,6 +11,22 @@ def self.from_json(json_string)
new(result_hash)
end

# a simple helper to convert an xml string to a Mash
def self.from_xml(xml_string)
result_hash = ::MultiXml.parse(xml_string)

# Drop off the root element
new(result_hash[result_hash.keys.first])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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']
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from_xml(response.body)
else
from_json(response.body)
end
end

# returns a Date if we have year, month and day, and no conflicting key
def to_date
if !self.has_key?('to_date') && contains_date_fields?
Expand Down
37 changes: 37 additions & 0 deletions lib/linked_in/template.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
require 'erb'
require 'ostruct'
require 'hashie'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch!


module LinkedIn
class TemplateBinding < ::Hashie::Mash
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"].

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh yea that seems legit

include ERB::Util
end

module Template

class << self
cache = {}
mutex = Mutex.new

define_method :load_template do |template|
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why use define_method instead of def?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return cache[template] if cache[template]
mutex.synchronize do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During your testing did you run into race conditions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return cache[template] if cache[template]

file = File.join(LinkedIn.templates, "#{template.to_s}.xml.erb")
io = ::IO.respond_to?(:binread) ? ::IO.binread(file) : ::IO.read(file)
erb = ERB.new(io)
erb.filename = file

cache[template] = erb
end
end
end

def render template, data
template = Template.load_template template
namespace = TemplateBinding.new data
template.result namespace.instance_eval { binding }
end
end
end
33 changes: 33 additions & 0 deletions lib/linked_in/templates/company_share.xml.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?xml version="1.0" encoding="UTF-8"?>
<share>
<visibility>
<code><%=h visibility.code%></code>
</visibility>
<% if comment %>
<comment><%= h comment %></comment>
<% end %>
<% if content %>
<content>
<% if content["title"] %><title><%= h content["title"] %></title><% end %>
<submitted-url><%= h content["submitted-url"] %></submitted-url>
<% if content["description"] %><description><%= h content["description"] %></description><% end %>
<% if content["submitted-image-url"] %><submitted-image-url><%= h content["submitted-image-url"] %></submitted-image-url><% end %>
</content>
<% end %>
<% if targets %>
<share-target-reach>
<share-targets>
<% targets.each do |key, values| %>
<share-target>
<code><%= h key %></code>
<tvalues>
<% values.each do |value| %>
<tvalue><%= h value %></tvalue>
<% end %>
</tvalues>
</share-target>
<% end %>
</share-targets>
</share-target-reach>
<% end %>
</share>
5 changes: 4 additions & 1 deletion lib/linkedin.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
module LinkedIn

class << self
attr_accessor :token, :secret, :default_profile_fields
attr_accessor :token, :secret, :default_profile_fields, :templates

# config/initializers/linkedin.rb (for instance)
#
Expand All @@ -22,11 +22,14 @@ def configure
end
end

@templates = File.join(File.expand_path(File.dirname(__FILE__)), 'linked_in', 'templates')

autoload :Api, "linked_in/api"
autoload :Client, "linked_in/client"
autoload :Mash, "linked_in/mash"
autoload :Errors, "linked_in/errors"
autoload :Helpers, "linked_in/helpers"
autoload :Search, "linked_in/search"
autoload :Version, "linked_in/version"
autoload :Template, "linked_in/template"
end
1 change: 1 addition & 0 deletions linkedin.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

gem.add_dependency 'oauth', '~> 0.4'
# gem.add_development_dependency 'json', '~> 1.6'
gem.add_development_dependency 'rake', '~> 10'
Expand Down
45 changes: 38 additions & 7 deletions spec/cases/api_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,6 @@
response.code.should == "201"
end

it "should be able to share a new company status" do
stub_request(:post, "https://api.linkedin.com/v1/companies/123456/shares").to_return(:body => "", :status => 201)
response = client.add_company_share("123456", { :comment => "Testing, 1, 2, 3" })
response.body.should == nil
response.code.should == "201"
end

it "returns the shares for a person" do
stub_request(:get, "https://api.linkedin.com/v1/people/~/network/updates?type=SHAR&scope=self&after=1234&count=35").to_return(
:body => "{}")
Expand Down Expand Up @@ -192,6 +185,44 @@
response.code.should == "201"
end

it "should be able to share a new company status" do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

stub_request(:post, "https://api.linkedin.com/v1/companies/2414183/shares").with(:headers => { 'Content-Type' => 'application/xml' }).to_return(:headers => {'Content-Type' => 'application/xml'}, :body => '<?xml version="1.0" encoding="UTF-8"?><update><update-key>UNIU-c2414183-5811244423991812096-SHARE</update-key><update-url>http://www.linkedin.com/company/2414183/comments?topic=5811244423991812096&amp;type=U&amp;scope=2414183&amp;stype=C&amp;a=FlWW</update-url></update>', :status => 201)
response = client.add_company_share("2414183", { :comment => "Testing, 1, 2, 3" })
response.body.update_key.should == 'UNIU-c2414183-5811244423991812096-SHARE'
response.body.update_url.should == 'http://www.linkedin.com/company/2414183/comments?topic=5811244423991812096&type=U&scope=2414183&stype=C&a=FlWW'
response.code.should == "201"
end

it "should be able to handle an error" do
stub_request(:post, "https://api.linkedin.com/v1/companies/2414183/shares").with(:headers => { 'Content-Type' => 'application/xml' }).to_return(:headers => {'Content-Type' => 'application/xml'}, :body => '<?xml version="1.0" encoding="UTF-8" standalone="yes"?> <error> <status>403</status> <timestamp>1386620304843</timestamp> <request-id>ZIBEJ5MXJ2</request-id> <error-code>0</error-code> <message>Member 172914333 cannot post updates on behalf of company 2414183 due to too few targeted followers</message> </error>', :status => 403)

expect {
client.add_company_share("2414183", { :comment => "Testing, 1, 2, 3" })
}.to raise_error(LinkedIn::Errors::AccessDeniedError){ |error|
error.data.message.should_not be_nil
}
end

it "should be able to target a new company status" do
stub_request(:post, "https://api.linkedin.com/v1/companies/2414183/shares").with(:headers => { 'Content-Type' => 'application/xml' }).to_return(:headers => {'Content-Type' => 'application/xml'}, :body => '<?xml version="1.0" encoding="UTF-8"?><update><update-key>UNIU-c2414183-5811244423991812096-SHARE</update-key><update-url>http://www.linkedin.com/company/2414183/comments?topic=5811244423991812096&amp;type=U&amp;scope=2414183&amp;stype=C&amp;a=FlWW</update-url></update>', :status => 201)
response = client.add_company_share("2414183", {
:comment => "Testing, 1, 2, 3",
:content => {
:"submitted-url" => "http://www.example.com/content.html",
:title => "Test Share with Content",
:description => "content description",
:"submitted-image-url" => "http://www.example.com/image.jpg"
},
:targets => {
:geos => ['as', 'eu'],
:jobFunc => ['acct', 'bd']
}
})
response.body.update_key.should == 'UNIU-c2414183-5811244423991812096-SHARE'
response.body.update_url.should == 'http://www.linkedin.com/company/2414183/comments?topic=5811244423991812096&type=U&scope=2414183&stype=C&a=FlWW'
response.code.should == "201"
end

end

context "Job API" do
Expand Down