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

Restore backwards compatibility with 0.13.0 Facebook/Instagram providers #78

Merged
merged 34 commits into from
Jan 1, 2021

Conversation

metavida
Copy link
Collaborator

@metavida metavida commented Nov 16, 2020

This PR resolves Issue #77 by converting the built-in Facebook & Instagram providers back into OEmbed::Provider instances, therefore restoring backwards compatibility with version < 0.14.0 using this syntax:

ENV['OEMBED_FACEBOOK_TOKEN'] #=> 'my-access-token']
OEmbed::Providers.register(OEmbed::Providers::Flickr,
                           OEmbed::Providers::Instagram)

Don't want to use an environment variable? You can use this syntax instead:

OEmbed::Providers::Instagram.access_token = @my_facebook_token
OEmbed::Providers.register(OEmbed::Providers::Flickr,
                           OEmbed::Providers::Instagram)

It also temporarily implements a class-like #new method on the built-in FacebookPost, FacebookVideo & Instagram providers, to maintain compatibility with version ~> 0.14.0 for folks who many have been using this syntax (my suggested short-term fix for Issue #77):

OEmbed::Providers.register(OEmbed::Providers::Flickr,
                           OEmbed::Providers::Instagram.new(access_token: @my_facebook_token))

More broadly, this PR also adds the generic concept required_query_params for OEmbed::Provider instances! In the short term this let me DRI up the implementation for the 3 built-in Facebook providers. In the long term I can improve the built-in Embedly provider to use this pattern and we'll be ready for any future providers that require custom query params (either for access tokens, or for any other reason)

@metavida metavida added the wip Do NOT merge! (Work In Progress) label Nov 16, 2020
@coveralls
Copy link

coveralls commented Nov 16, 2020

Coverage Status

Coverage increased (+0.6%) to 96.219% when pulling 0045658 on restore-fb-provider-backwards-compatibility into 2628f13 on master.

@metavida metavida force-pushed the restore-fb-provider-backwards-compatibility branch from a2f9adf to 616ae32 Compare November 16, 2020 01:01
Marcos Wright-Kuhns added 26 commits November 15, 2020 17:09
Also switch to use a subject rather than `let(:provider_class)`
...because it kept adding duplicate requests & I couldn't figure out why
...which is the specific bit of backwards compatibility that broke in 0.14.0
...with forged "200 OK" responses in the VCR cassette, so that the test pass despite the legacy Instagram oEmbed endpoint having been deprecated on October 23rd.
...where, if there exists an OEMBED_FACEBOOK_TOKEN env var, the Instagram class responds to enough of the instance methods to work on a basic level.
...which is a fine balance. I want to make it possible to capture real-world Instagram & Facebook API responses into a VCR cassette.

Also I want to make it fairly difficult for developers to accidentally record their secrets into a VCR cassette.
...in preparation for adding a new initialize option
...which makes way for an upcoming "required_query_params" argument
...by using the OEmbed::Provider#includes? method, which always returns false if there are missing required_query_params.

Also lots more test coverage for OEmbed::Providers.find
...with a class-like `Instagram.new` method to avoid breaking things for folks who added v0.14.0 work-arounds
...since it explains the additional env var for CI testing.
…-backwards-compatibility

# Conflicts:
#	CHANGELOG.rdoc
#	spec/provider_spec.rb
#	spec/support/shared_examples_for_providers.rb
...to prevent developers' local OEMBED_FACEBOOK_TOKEN env var from leaking into the cassettes, while still allowing "200 OK" responses to be recorded!
...and ensure the "behaves like a custom OEmbed::Provider class" tests don't leak scope, since the "new" method modiefies the instance now.
...with a class-like `FacebookPost.new` method to avoid breaking things for folks who added v0.14.0 work-arounds
...with a class-like `FacebookVideo.new` method to avoid breaking things for folks who added v0.14.0 work-arounds
Marcos Wright-Kuhns added 3 commits December 14, 2020 06:54
Also test a different photo URL pattern.
Instead add support for calling `add_official_provider` with `:access_token` options.

Also move all built-in providers into a separate ruby file.
@metavida metavida force-pushed the restore-fb-provider-backwards-compatibility branch from 9c611a8 to 81219e3 Compare December 14, 2020 16:04
@metavida metavida force-pushed the restore-fb-provider-backwards-compatibility branch from 8a2ebb2 to fc757e7 Compare December 27, 2020 15:46
@metavida
Copy link
Collaborator Author

@elektronaut In order to fix Issue #77 I built on top of your PR to add the concept of required_query_params. If you have the time, I'd love any feedback, questions, or comments you have!

@metavida metavida removed the wip Do NOT merge! (Work In Progress) label Dec 27, 2020
Required *quite* a bit of re-refactoring, but I think we got there!

# Conflicts:
#	CHANGELOG.rdoc
#	lib/oembed/providers.rb
#	spec/cassettes/OEmbed_Providers_Youtube.yml
#	spec/providers/youtube_spec.rb
@metavida metavida changed the title WIP: Restore backwards compatibility with 0.13.0 Facebook/Instagram providers Restore backwards compatibility with 0.13.0 Facebook/Instagram providers Jan 1, 2021
@metavida metavida merged commit 7e2e900 into master Jan 1, 2021
@metavida metavida deleted the restore-fb-provider-backwards-compatibility branch January 1, 2021 20:44
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

2 participants