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

Facebook: infer app-scoped user ids like we infer usernames #651

Merged
merged 2 commits into from
Apr 18, 2016

Conversation

kylewm
Copy link
Contributor

@kylewm kylewm commented Apr 18, 2016

When other applications posse to Facebook, they generally create
URLs with user-ids scoped to that application. These URLs make
no sense to Bridgy. This change canonicalizes them to Bridgy's user
ID for that user, just like we do for usernames.

Open question: does this cause problems if you POSSE to multiple
Facebook accounts (e.g. kyle.mahan and indiewebcamp). Bridgy will
be overly generous in classifying syndication links to one as syndication
links to both, but since the post ID part of the URL will always be unique,
I don't think this will cause any actual harm.

ref #375, #650

When other applications posse to Facebook, they generally create
URLs with user-ids scoped to *that* application. These URLs make
no sense to Bridgy. This change canonicalizes them to Bridgy's user
ID for that user, just like we do for usernames.

ref snarfed#375, snarfed#650
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 92.1% when pulling 0e0b33b on kylewm:inferred-user-ids into 30ccc3a on snarfed:master.

@kylewm
Copy link
Contributor Author

kylewm commented Apr 18, 2016

Full disclosure: I don't have an easy way to test this live and haven't yet...

@@ -91,6 +89,9 @@ class FacebookPage(models.Source):
username = ndb.StringProperty()
# inferred from syndication URLs if username isn't available
inferred_username = ndb.StringProperty()
# inferred application-specific user IDs (from other applications)
inferred_app_scoped_user_ids = ndb.StringProperty(repeated=True)
Copy link
Owner

Choose a reason for hiding this comment

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

minor: consider just inferred_user_ids and similarly elsewhere? technically this happily infers global user ids as well as app scoped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that's a really good point, like if bridgy has an app-scoped ID and the POSSE application has a global ID.

@snarfed
Copy link
Owner

snarfed commented Apr 18, 2016

looks great, thanks again! feel free to merge whenever.

oh and i regularly make changes like this without testing manually. :P not a great habit; unit tests maybe give us a bit too much confidence. ¯_(ツ)_/¯

@snarfed
Copy link
Owner

snarfed commented Apr 18, 2016

oh btw i've turned off the coveralls comments. 🃏 🔫

@kylewm
Copy link
Contributor Author

kylewm commented Apr 18, 2016

Oh great! I did actually just figure out how to test locally, and it looks like it works

2016-04-18 17:36:40.437781 I Inferring app-scoped user id 10101059883222449 from syndication url https://www.facebook.com/10101059883222449/posts/10101699886994759
2016-04-18 17:36:40.465735 D discovered relationships {u'https://www.facebook.com/10101059883222449/posts/10101699886994759': [SyndicatedPost(key=Key('FacebookPage', '12802152', 'SyndicatedPost', 5066549580791808), created=datetime.datetime(2016, 4, 18, 17, 36, 40, 422009), original=u'http://known.dev/2016/test-1-2-1-2', syndication=u'https://www.facebook.com/12802152/posts/10101699886994759', updated=datetime.datetime(2016, 4, 18, 17, 36, 40, 422018))]}

@snarfed
Copy link
Owner

snarfed commented Apr 18, 2016

good timing too, #650 inspired me to put "remove see original OPD" on my todo list this morning. :P

@kylewm kylewm merged commit 0f29773 into snarfed:master Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants