-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fixes #6
Fixes #6
Conversation
Gemfile
Outdated
@@ -24,9 +24,13 @@ gem 'listen', '~> 3.1', '>= 3.1.5' | |||
gem 'uglifier', '>= 1.0.3' | |||
#end | |||
|
|||
gem 'byebug' | |||
gem 'puma' | |||
gem 'twitter' |
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.
please include the version for these gems.
app/Utilities/twitter_wrapper.rb
Outdated
@@ -0,0 +1,47 @@ | |||
class TwitterWrapper |
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.
The folder names should use small case - refactor Utilities to utilities.
app/Utilities/twitter_wrapper.rb
Outdated
def user_valid?(uid) | ||
begin | ||
check_user_validity(uid) | ||
rescue |
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 will mark user invalid even if failure is network related. Please add exact exceptions where this should be handled and user marked as invalid.
app/Utilities/twitter_wrapper.rb
Outdated
rescue | ||
return false | ||
else | ||
return true |
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.
Does this else block ever get executed? If no, please remove it.
app/Utilities/twitter_wrapper.rb
Outdated
|
||
def fetch_user_by_uid(uid) | ||
if uid.is_a?(String) | ||
uid = uid.to_i |
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.
You may call to_i
without check if that's a string. to_i
will handle if it's already a number case.
app/models/user.rb
Outdated
twitter_description: auth["info"]["description"], | ||
website: (auth["info"]["urls"]["Website"] rescue nil), | ||
twitter_oauth: auth["credentials"]["token"], | ||
image: auth["info"]["image"] |
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.
Indentation correction -should be two spaces - looks like its's four spaces inside the create block
def change | ||
users = User.all | ||
provider = "twitter" | ||
users.each do |user| |
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.
It's generally better to use - User.find_each
instead of User.all
followed by each
- for_each
saves allocating too many objects at once by not taking everything form db at once.
app/models/user.rb
Outdated
@@ -11,12 +12,14 @@ class User < ActiveRecord::Base | |||
|
|||
class << self | |||
def find_for_twitter_oauth(auth) | |||
user = User.find_by_twitter_oauth(auth["credentials"]["token"]) | |||
user = User.where(uid: auth.uid, provider:auth.provider).first |
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.
missing space correction.
lib/tasks/modify_user_data.rake
Outdated
user.image = DEFAULT_IMAGE | ||
puts "#{ response[:payload] } (#{user.uid}) thus assigning default image to this user" | ||
end | ||
user.save |
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.
just use save!
* Sign in with twitter working * active admin dashboard fixed * server changed from webbrick to puma * migration create to update uid, provider column of users table * migration to populate uid and provider column of existing users * rake task to update users' profile images
This PR fixes the following