-
Notifications
You must be signed in to change notification settings - Fork 407
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
Update LinkedIn::Api::ShareAndSocialStream#add_share for Api v2 #274
base: master
Are you sure you want to change the base?
Conversation
* The path changed to /v2/ugcPosts * More attributes are required in the payload Along with sending over the comment, you must send the user's "urn" Example: ``` client = LinkedIn::Client.new client.authorize_from_access(@delivery_method_attributes[:token]) client.add_share({ comment: 'hi', urn: 'urn' }) ``` https://docs.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/share-on-linkedin?context=linkedin/consumer/context
@dsandstrom if you want to write a test for this I can review this and merge it and also cut a new gem. Thanks! |
Well the problem is this change breaks all other endpoints as they have not been updated for LinkedIn's API v2. I just wanted to show other people what I got working. |
We could do something like Linkedin::V2.add_share too. That way it wouldn't break for others |
Can't post without it so no reason to make it an option. Also improve docs
Add the url to the payload. The link title and description are also customizable Example: ``` payload = { comment: "Comment", title: "Link Title", description: "Link Description", url: "http://example.com/article" } client.add_share(urn, payload) ```
Rollback share_and_social_stream file back to master Not sure how you want to keep v2 methods separate, but this allows v1 endpoints to still work. You said to do "something like Linkedin::V2.add_share", but that doesn't really make sense since we run the methods on the LinkedIn::Client. A possibility is to add a LinkedIn::V2::Client, then include the v2 methods into that class.
v2_profile and v2_add_share
Not sure how you want to keep v2 methods separate, but this allows v1 endpoints to still work. You said to do "something like Linkedin::V2.add_share", but that doesn't really make sense since we run |
A comment is always required
# @see https://docs.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/sign-in-with-linkedin?context=linkedin/consumer/context#retrieving-member-profiles | ||
# | ||
# @return [void] | ||
def v2_profile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: def profile
since it's already scoped to V2
# | ||
# @macro share_input_fields | ||
# @return [void] | ||
def v2_add_share(urn, share = {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again with the same nitpick. I feel like if it is already scoped to a V2 module it should just be without a prefix on the method.
} | ||
} | ||
|
||
return add_url_to_payload(payload, share) if share[:url] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a nitpick of mine.
I would prefer it to look something more like
if share.has_key?(:url)
add_url_to_payload(payload, share)
else
add_comment_to_payload(payload,share)
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also I think
if !!share.fetch(:url, false)
...
end
# @macro share_input_fields | ||
# @return [void] | ||
def v2_add_share(urn, share = {}) | ||
if !urn.instance_of?(String) || urn.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that really what you're after is something like this:
(urn.respond_to?(:empty?) && urn.empty?) || !urn
def v2_add_share(urn, share = {}) | ||
if !urn.instance_of?(String) || urn.empty? | ||
raise LinkedIn::Errors::UnavailableError, 'LinkedIn API: URN required' | ||
elsif share[:comment].nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be safer
!share.fetch(:comment, false)
|
||
def add_url_to_payload(payload, share) | ||
media = { status: 'READY', originalUrl: share[:url] } | ||
if share[:description] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are tough because we are checking if they are in the hash, but should we be checking for things like blankness?
include Api::QueryHelpers | ||
include Api::People | ||
include Api::Groups | ||
include Api::Companies | ||
include Api::Jobs | ||
include Api::ShareAndSocialStream | ||
include Api::Communications | ||
include Api::V2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see why you prefixed the methods now...
hmmm... well I suppose that's ok it just feels a bit awkward to me is all.
nice work! I just put a few comments, mostly nitpicks and question type things. I'm wondering if we should do something besides the prefixing of the methods for v2_* requests. I think it's ok, it just feels a bit weird is all since v1 is mostly deprecated at this point. |
Prefixing with v2 was just temporary. A V2 module needs to be set up. See #274 (comment) |
Yea Linkedin::V2::Client makes sense. if you want to do that. |
@hexgnu @dsandstrom Just a heads up I added the email endpoint in a fork and opened a PR on @dsandstrom fork. dsandstrom#1. My company needs it for our auth flow. |
@hexgnu @dsandstrom I ended up adding a little more to my pr it now includes the additions:
Meant to say this the first time, but I did not address any of the outstanding pr comments. |
If you all want to combine forces feel free to do so :) will gladly review and merge in. |
@dsandstrom @addbrick - great work! Thank you for doing this! If you all can get this in I can't wait to use it :) |
Again happy to help merge this stuff in I just don't work with the linkedin API anymore so I'd be flying blind here 😄. |
Along with sending over the comment, you must send the user's "urn"
Example (updated):
https://docs.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/share-on-linkedin?context=linkedin/consumer/context