From 20f39caa42282c73b13e2309f83ea829a85def62 Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Thu, 14 May 2020 22:51:07 +0530 Subject: [PATCH] Chore: Twitter Integration house cleaning (#855) Co-authored-by: Pranav Raj Sreepuram --- Gemfile | 3 +- Gemfile.lock | 11 +--- app/builders/contact_builder.rb | 22 ++++--- .../api/v1/accounts/callbacks_controller.rb | 4 +- .../twitter/callbacks_controller.rb | 47 +++++++------- app/models/channel/twitter_profile.rb | 9 ++- app/models/channel/web_widget.rb | 2 +- app/services/twitter/send_reply_service.rb | 2 +- .../twitter/webhook_subscribe_service.rb | 43 ++++++++++++- .../twitter/callbacks_controller_spec.rb | 17 +++-- .../twitter/webhook_subscribe_service_spec.rb | 62 ++++++++++++++++--- 11 files changed, 159 insertions(+), 63 deletions(-) diff --git a/Gemfile b/Gemfile index b9e9c4f4c..50e3e2ce8 100644 --- a/Gemfile +++ b/Gemfile @@ -64,7 +64,8 @@ gem 'facebook-messenger' gem 'telegram-bot-ruby' gem 'twilio-ruby', '~> 5.32.0' # twitty will handle subscription of twitter account events -gem 'twitty', git: 'https://github.com/chatwoot/twitty' +# gem 'twitty', git: 'https://github.com/chatwoot/twitty' +gem 'twitty' # facebook client gem 'koala' # Random name generator diff --git a/Gemfile.lock b/Gemfile.lock index 1659d8cdf..cc6d481ee 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,10 +1,3 @@ -GIT - remote: https://github.com/chatwoot/twitty - revision: af4f3e45dca55e42c64f7741a1fedfaa94d36419 - specs: - twitty (0.1.0) - oauth - GIT remote: https://github.com/sds/mock_redis revision: 16d00789f0341a3aac35126c0ffe97a596753ff9 @@ -489,6 +482,8 @@ GEM faraday (~> 1.0.0) jwt (>= 1.5, <= 2.5) nokogiri (>= 1.6, < 2.0) + twitty (0.1.1) + oauth tzinfo (1.2.7) thread_safe (~> 0.1) tzinfo-data (1.2020.1) @@ -598,7 +593,7 @@ DEPENDENCIES telephone_number time_diff twilio-ruby (~> 5.32.0) - twitty! + twitty tzinfo-data uglifier valid_email2 diff --git a/app/builders/contact_builder.rb b/app/builders/contact_builder.rb index 9bb3ef3ac..3e362c9b5 100644 --- a/app/builders/contact_builder.rb +++ b/app/builders/contact_builder.rb @@ -14,6 +14,18 @@ class ContactBuilder @account ||= inbox.account end + def create_contact_inbox(contact) + ::ContactInbox.create!( + contact_id: contact.id, + inbox_id: inbox.id, + source_id: source_id + ) + end + + def update_contact_avatar(contact) + ::ContactAvatarJob.perform_later(contact, contact_attributes[:avatar_url]) if contact_attributes[:avatar_url] + end + def build_contact ActiveRecord::Base.transaction do contact = account.contacts.create!( @@ -23,16 +35,12 @@ class ContactBuilder identifier: contact_attributes[:identifier], additional_attributes: contact_attributes[:additional_attributes] ) - contact_inbox = ::ContactInbox.create!( - contact_id: contact.id, - inbox_id: inbox.id, - source_id: source_id - ) - ::ContactAvatarJob.perform_later(contact, contact_attributes[:avatar_url]) if contact_attributes[:avatar_url] + contact_inbox = create_contact_inbox(contact) + update_contact_avatar(contact) contact_inbox rescue StandardError => e - Rails.logger e + Rails.logger.info e end end end diff --git a/app/controllers/api/v1/accounts/callbacks_controller.rb b/app/controllers/api/v1/accounts/callbacks_controller.rb index bf42b215a..54c7bace4 100644 --- a/app/controllers/api/v1/accounts/callbacks_controller.rb +++ b/app/controllers/api/v1/accounts/callbacks_controller.rb @@ -14,7 +14,7 @@ class Api::V1::Accounts::CallbacksController < Api::BaseController @facebook_inbox = current_account.inboxes.create!(name: inbox_name, channel: facebook_channel) set_avatar(@facebook_inbox, page_id) rescue StandardError => e - Rails.logger e + Rails.logger.info e end end @@ -62,7 +62,7 @@ class Api::V1::Accounts::CallbacksController < Api::BaseController koala = Koala::Facebook::OAuth.new(ENV['FB_APP_ID'], ENV['FB_APP_SECRET']) koala.exchange_access_token_info(omniauth_token)['access_token'] rescue StandardError => e - Rails.logger e + Rails.logger.info e end def mark_already_existing_facebook_pages(data) diff --git a/app/controllers/twitter/callbacks_controller.rb b/app/controllers/twitter/callbacks_controller.rb index 6065e3546..f317f3ffa 100644 --- a/app/controllers/twitter/callbacks_controller.rb +++ b/app/controllers/twitter/callbacks_controller.rb @@ -2,18 +2,18 @@ class Twitter::CallbacksController < Twitter::BaseController def show return redirect_to twitter_app_redirect_url if permitted_params[:denied] - @response = twitter_client.access_token( - oauth_token: permitted_params[:oauth_token], - oauth_verifier: permitted_params[:oauth_verifier] - ) - if @response.status == '200' - inbox = build_inbox + @response = ensure_access_token + return redirect_to twitter_app_redirect_url if @response.status != '200' + + ActiveRecord::Base.transaction do + inbox = create_inbox ::Redis::Alfred.delete(permitted_params[:oauth_token]) ::Twitter::WebhookSubscribeService.new(inbox_id: inbox.id).perform redirect_to app_twitter_inbox_agents_url(account_id: account.id, inbox_id: inbox.id) - else - redirect_to twitter_app_redirect_url end + rescue StandardError => e + Rails.logger.info e + redirect_to twitter_app_redirect_url end private @@ -34,20 +34,23 @@ class Twitter::CallbacksController < Twitter::BaseController app_new_twitter_inbox_url(account_id: account.id) end - def build_inbox - ActiveRecord::Base.transaction do - twitter_profile = account.twitter_profiles.create( - twitter_access_token: parsed_body['oauth_token'], - twitter_access_token_secret: parsed_body['oauth_token_secret'], - profile_id: parsed_body['user_id'] - ) - account.inboxes.create( - name: parsed_body['screen_name'], - channel: twitter_profile - ) - rescue StandardError => e - Rails.logger e - end + def ensure_access_token + twitter_client.access_token( + oauth_token: permitted_params[:oauth_token], + oauth_verifier: permitted_params[:oauth_verifier] + ) + end + + def create_inbox + twitter_profile = account.twitter_profiles.create( + twitter_access_token: parsed_body['oauth_token'], + twitter_access_token_secret: parsed_body['oauth_token_secret'], + profile_id: parsed_body['user_id'] + ) + account.inboxes.create( + name: parsed_body['screen_name'], + channel: twitter_profile + ) end def permitted_params diff --git a/app/models/channel/twitter_profile.rb b/app/models/channel/twitter_profile.rb index 1a11360f7..dd6ee1f92 100644 --- a/app/models/channel/twitter_profile.rb +++ b/app/models/channel/twitter_profile.rb @@ -35,7 +35,7 @@ class Channel::TwitterProfile < ApplicationRecord source_id: profile_id ) rescue StandardError => e - Rails.logger e + Rails.logger.info e end end @@ -53,7 +53,10 @@ class Channel::TwitterProfile < ApplicationRecord private def unsubscribe - webhooks_list = twitter_client.fetch_webhooks.body - twitter_client.unsubscribe_webhook(id: webhooks_list.first['id']) if webhooks_list.present? + ### Fix unsubscription with new endpoint + unsubscribe_response = twitter_client.remove_subscription(user_id: profile_id) + Rails.logger.info 'TWITTER_UNSUBSCRIBE: ' + unsubscribe_response.body.to_s + rescue StandardError => e + Rails.logger.info e end end diff --git a/app/models/channel/web_widget.rb b/app/models/channel/web_widget.rb index 4b927c7ba..203049f64 100644 --- a/app/models/channel/web_widget.rb +++ b/app/models/channel/web_widget.rb @@ -55,7 +55,7 @@ class Channel::WebWidget < ApplicationRecord ) contact_inbox rescue StandardError => e - Rails.logger e + Rails.logger.info e end end end diff --git a/app/services/twitter/send_reply_service.rb b/app/services/twitter/send_reply_service.rb index 88879c68b..e30786dc0 100644 --- a/app/services/twitter/send_reply_service.rb +++ b/app/services/twitter/send_reply_service.rb @@ -47,7 +47,7 @@ class Twitter::SendReplyService tweet_data = response.body message.update!(source_id: tweet_data['id_str']) else - Rails.logger 'TWITTER_TWEET_REPLY_ERROR' + response.body + Rails.logger.info 'TWITTER_TWEET_REPLY_ERROR' + response.body end end diff --git a/app/services/twitter/webhook_subscribe_service.rb b/app/services/twitter/webhook_subscribe_service.rb index 0c2badaff..3fae23e8f 100644 --- a/app/services/twitter/webhook_subscribe_service.rb +++ b/app/services/twitter/webhook_subscribe_service.rb @@ -4,9 +4,13 @@ class Twitter::WebhookSubscribeService pattr_initialize [:inbox_id] def perform - register_response = twitter_client.register_webhook(url: webhooks_twitter_url(protocol: 'https')) - twitter_client.subscribe_webhook if register_response.status == '200' - Rails.logger.info 'TWITTER_REGISTER_WEBHOOK_FAILURE: ' + register_response.body.to_s + ensure_webhook + unless subscription? + subscribe_response = twitter_client.create_subscription + raise StandardError, 'Twitter Subscription Failed' unless subscribe_response.status == '204' + end + + true end private @@ -17,4 +21,37 @@ class Twitter::WebhookSubscribeService def inbox Inbox.find_by!(id: inbox_id) end + + def twitter_url + webhooks_twitter_url(protocol: 'https') + end + + def ensure_webhook + webhooks = fetch_webhooks + return true if webhooks&.first&.try(:[], 'url') == twitter_url + + # twitter supports only one webhook url per environment + # so we will delete the existing one if its not chatwoot + unregister_webhook(webhooks.first) if webhooks&.first + register_webhook + end + + def unregister_webhook(webhook) + unregister_response = twitter_client.unregister_webhook(id: webhook.try(:[], 'id')) + Rails.logger.info 'TWITTER_UNREGISTER_WEBHOOK: ' + unregister_response.body.to_s + end + + def register_webhook + register_response = twitter_client.register_webhook(url: twitter_url) + Rails.logger.info 'TWITTER_UNREGISTER_WEBHOOK: ' + register_response.body.to_s + end + + def subscription? + response = twitter_client.fetch_subscriptions + response.status == '204' + end + + def fetch_webhooks + twitter_client.fetch_webhooks.body + end end diff --git a/spec/controllers/twitter/callbacks_controller_spec.rb b/spec/controllers/twitter/callbacks_controller_spec.rb index 9b8c9ce46..213f4704d 100644 --- a/spec/controllers/twitter/callbacks_controller_spec.rb +++ b/spec/controllers/twitter/callbacks_controller_spec.rb @@ -1,27 +1,26 @@ require 'rails_helper' RSpec.describe 'Twitter::CallbacksController', type: :request do - subject(:webhook_subscribe_service) { described_class.new(inbox_id: twitter_inbox.id) } - let(:twitter_client) { instance_double(::Twitty::Facade) } let(:twitter_response) { instance_double(::Twitty::Response, status: '200', body: { message: 'Valid' }) } let(:raw_response) do object_double('raw_response', body: 'oauth_token=1&oauth_token_secret=1&user_id=100&screen_name=chatwoot') end let(:account) { create(:account) } + let(:webhook_service) { double } before do allow(::Twitty::Facade).to receive(:new).and_return(twitter_client) allow(::Redis::Alfred).to receive(:get).and_return(account.id) allow(::Redis::Alfred).to receive(:delete).and_return('OK') allow(twitter_client).to receive(:access_token).and_return(twitter_response) - allow(twitter_client).to receive(:register_webhook).and_return(twitter_response) - allow(twitter_client).to receive(:subscribe_webhook).and_return(true) allow(twitter_response).to receive(:raw_response).and_return(raw_response) + allow(::Twitter::WebhookSubscribeService).to receive(:new).and_return(webhook_service) end describe 'GET /twitter/callback' do - it 'returns correct response' do + it 'creates inboxes if subscription is successful' do + allow(webhook_service).to receive(:perform).and_return true get twitter_callback_url account.reload expect(response).to redirect_to app_twitter_inbox_agents_url(account_id: account.id, inbox_id: account.inboxes.last.id) @@ -29,5 +28,13 @@ RSpec.describe 'Twitter::CallbacksController', type: :request do expect(account.twitter_profiles.last.inbox.name).to eq 'chatwoot' expect(account.twitter_profiles.last.profile_id).to eq '100' end + + it 'does not create inbox if subscription fails' do + allow(webhook_service).to receive(:perform).and_raise StandardError + get twitter_callback_url + account.reload + expect(response).to redirect_to app_new_twitter_inbox_url(account_id: account.id) + expect(account.inboxes.count).to be 0 + end end end diff --git a/spec/services/twitter/webhook_subscribe_service_spec.rb b/spec/services/twitter/webhook_subscribe_service_spec.rb index 8af7042de..c94314836 100644 --- a/spec/services/twitter/webhook_subscribe_service_spec.rb +++ b/spec/services/twitter/webhook_subscribe_service_spec.rb @@ -12,24 +12,66 @@ describe ::Twitter::WebhookSubscribeService do before do allow(::Twitty::Facade).to receive(:new).and_return(twitter_client) - allow(twitter_client).to receive(:register_webhook) - allow(twitter_client).to receive(:subscribe_webhook) + allow(twitter_client).to receive(:register_webhook).and_return(twitter_success_response) + allow(twitter_client).to receive(:unregister_webhook).and_return(twitter_success_response) + allow(twitter_client).to receive(:fetch_subscriptions).and_return(instance_double(::Twitty::Response, status: '204', body: { message: 'Valid' })) + allow(twitter_client).to receive(:create_subscription).and_return(instance_double(::Twitty::Response, status: '204', body: { message: 'Valid' })) end describe '#perform' do - context 'with successful registration' do - it 'calls subscribe webhook' do - allow(twitter_client).to receive(:register_webhook).and_return(twitter_success_response) + context 'when webhook is not registered' do + it 'calls register_webhook' do + allow(twitter_client).to receive(:fetch_webhooks).and_return( + instance_double(::Twitty::Response, status: '200', body: {}) + ) webhook_subscribe_service.perform - expect(twitter_client).to have_received(:subscribe_webhook) + expect(twitter_client).not_to have_received(:unregister_webhook) + expect(twitter_client).to have_received(:register_webhook) end end - context 'with unsuccessful registration' do - it 'does not call subscribe webhook' do - allow(twitter_client).to receive(:register_webhook).and_return(twitter_error_response) + context 'when valid webhook is registered' do + it 'calls unregister_webhook and then register webhook' do + allow(twitter_client).to receive(:fetch_webhooks).and_return( + instance_double(::Twitty::Response, status: '200', + body: [{ 'url' => webhook_subscribe_service.send(:twitter_url) }]) + ) webhook_subscribe_service.perform - expect(twitter_client).not_to have_received(:subscribe_webhook) + expect(twitter_client).not_to have_received(:unregister_webhook) + expect(twitter_client).not_to have_received(:register_webhook) + end + end + + context 'when invalid webhook is registered' do + it 'calls unregister_webhook and then register webhook' do + allow(twitter_client).to receive(:fetch_webhooks).and_return( + instance_double(::Twitty::Response, status: '200', + body: [{ 'url' => 'invalid_url' }]) + ) + webhook_subscribe_service.perform + expect(twitter_client).to have_received(:unregister_webhook) + expect(twitter_client).to have_received(:register_webhook) + end + end + + context 'when correct webhook is present' do + it 'calls create subscription if subscription is not present' do + allow(twitter_client).to receive(:fetch_webhooks).and_return( + instance_double(::Twitty::Response, status: '200', + body: [{ 'url' => webhook_subscribe_service.send(:twitter_url) }]) + ) + allow(twitter_client).to receive(:fetch_subscriptions).and_return(instance_double(::Twitty::Response, status: '500')) + webhook_subscribe_service.perform + expect(twitter_client).to have_received(:create_subscription) + end + + it 'does not call create subscription if subscription is already present' do + allow(twitter_client).to receive(:fetch_webhooks).and_return( + instance_double(::Twitty::Response, status: '200', + body: [{ 'url' => webhook_subscribe_service.send(:twitter_url) }]) + ) + webhook_subscribe_service.perform + expect(twitter_client).not_to have_received(:create_subscription) end end end