Skip to content

Validate twitter agent credentials #360

Open
wants to merge 5 commits into from

3 participants

@jvans1
jvans1 commented Jun 1, 2014

Added variable caching and validation of credentials. Would love to get the specs passing but for some reason I'm getting a NoMethodError on allow_any_instance_of which is bizarre is rspec 2.14 has this available. Open to other ways of doing this or advice if I'm doing something wrong.

@coveralls

Coverage Status

Coverage decreased (-1.5%) when pulling c80b03e on jvans1:validate_twitter_agent_credentials into 43d0256 on cantino:master.

@coveralls

Coverage Status

Coverage decreased (-1.62%) when pulling 34460f2 on jvans1:validate_twitter_agent_credentials into 43d0256 on cantino:master.

@cantino
Owner
cantino commented Jun 1, 2014

Thanks @jvans1!

@cantino cantino commented on the diff Jun 1, 2014
app/concerns/twitter_concern.rb
end
def twitter_oauth_token_secret
- options['oauth_token_secret'].presence || options['access_secret'].presence || credential('twitter_oauth_token_secret')
+ @twitter_oauth_token_secret ||= options['oauth_token_secret'].presence || options['access_secret'].presence || credential('twitter_oauth_token_secret')
@cantino
Owner
cantino added a note Jun 1, 2014

These memoized caches may not be necessary, as the credential does caching.

@jvans1
jvans1 added a note Jun 20, 2014

On one hand I agree with you, the cache in the #credentials method makes this less useful. But when you make the decision not to cache here because of that, you're making an assumption about how another class behaves which could at some point change. For this one it's probably unlikely to change and not memoizing here is pretty safe. However writing this method with no assumptions about how methods in Agent behave seems slightly better to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cantino cantino commented on an outdated diff Jun 1, 2014
spec/models/agents/twitter_stream_agent_spec.rb
@@ -17,6 +17,26 @@
@agent.save!
end
+ describe "#valid_credentials" do
+ let(:stream) { Agents::TwitterStreamAgent.new }
+ let(:invalid_error_message) { "Twitter credentials are invalid, a connection could not be established" }
+ context "invalid credentials" do
+ xit 'includes invalid credential errors' do
+ allow_any_instance_of(Twitter::REST::Client).to receive(:verify_credentials).and_raise Twitter::Error::Unauthorized.new("invalid")
@cantino
Owner
cantino added a note Jun 1, 2014

We use rr for mocking, so you'll need something like:

mock.any_instance_of(Twitter::REST::Client).verify_credentials { raise Twitter::Error::Unauthorized.new("invalid") }
@cantino
Owner
cantino added a note Jun 11, 2014

I just noticed that these its are xited out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cantino
Owner
cantino commented Jun 19, 2014

@jvans1 do you need any help with this PR?

@jvans1
jvans1 commented Jun 19, 2014

I'm good, I've just been super busy at work. I should have some time to wrap this up over the weekend. Thanks though!

@jvans1
jvans1 commented Jun 20, 2014

Not quite done yet but close

@coveralls

Coverage Status

Coverage increased (+0.13%) when pulling 159550d on jvans1:validate_twitter_agent_credentials into fb0b423 on cantino:master.

@coveralls

Coverage Status

Coverage increased (+0.15%) when pulling 478f06c on jvans1:validate_twitter_agent_credentials into fb0b423 on cantino:master.

@jvans1 jvans1 commented on the diff Jun 25, 2014
spec/support/spec_helpers/twitter_credentials.rb
@@ -0,0 +1,15 @@
+module SpecHelpers
+ module TwitterCredentials
+ def self.included(klass)
+ klass.class_eval do
@jvans1
jvans1 added a note Jun 25, 2014

I don't think this is too expensive and if I don't do this it could break people's builds when the update if they have other specs for twitter agents.

@cantino
Owner
cantino added a note Jun 29, 2014

I'm torn. I think it'd be better for you to consider master up-to-date and fix all specs around Twitter, instead of monkeypatching the verify_credentials mock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jvans1
jvans1 commented Jun 25, 2014

This build should pass, let me know what you think

@coveralls

Coverage Status

Coverage increased (+0.15%) when pulling 346e1ee on jvans1:validate_twitter_agent_credentials into fb0b423 on cantino:master.

@cantino cantino commented on the diff Jun 29, 2014
app/concerns/twitter_concern.rb
config.consumer_key = twitter_consumer_key
config.consumer_secret = twitter_consumer_secret
config.access_token = twitter_oauth_token
config.access_token_secret = twitter_oauth_token_secret
end
end
+
+
+ private
+ def twitter_consumer_key_present
+ if twitter_consumer_key.nil?
@cantino
Owner
cantino added a note Jun 29, 2014

I think these (here and below) should be .present? instead of .nil? since an empty string also isn't valid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cantino cantino commented on the diff Jun 29, 2014
...models/agents/twitter_stream_agent_spec/check_spec.rb
@@ -0,0 +1,57 @@
+require 'spec_helper'
+
+describe Agents::TwitterStreamAgent do
@cantino
Owner
cantino added a note Jun 29, 2014

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cantino
Owner
cantino commented Jul 29, 2014

Hey @jvans1, I like your work on this PR. One concern I have is that now any Agent that uses the TwitterConcern will do an actual web request to Twitter anytime a save is attempted. I think this should only happen when the Twitter fields have actually changed, so that internal saves to the object don't run these validations. What do you think?

Unfortunately, to do that, we need to wire through something like option_changed?('consumer_secret') in JSONSerializedField so that we can test it, since the Rails-generated options_changed? won't do what we want.

@jvans1
jvans1 commented Aug 5, 2014

Yea good catch. We definitely should not be validating this on every save. I could implement my own def credentials_changed? with something like options_was['consumer_secret'] != options['consumer_secret'] || options_was etc etc.

@cantino
Owner
cantino commented Aug 5, 2014

Yea, that makes sense. You might be able to add it to the JSONSerializedField Concern in a more generic way if you're feeling ambitious :)

@jvans1
jvans1 commented Aug 5, 2014

True, it's definitely better to make it more generalized in this situation. I'll give that option a go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.