From 9d1e77aeba90a13765b593480f349a664a05b015 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 1 Jan 2023 23:46:10 +0100 Subject: [PATCH 01/10] Revoke Twitter connection when the token is revoked --- .../styles/components/_notifications.scss | 5 +++-- app/models/notification/service_token_expired.rb | 4 ++++ app/models/notification/twitter_token_expired.rb | 4 ++++ app/models/user/expired_service_connection.rb | 5 +++++ .../user/expired_twitter_service_connection.rb | 4 ++++ .../_expiredtwitterserviceconnection.html.haml | 8 ++++++++ app/workers/share_worker.rb | 15 ++++++++++++--- config/locales/views.en.yml | 4 ++++ spec/workers/share_worker_spec.rb | 12 ++++++++++++ 9 files changed, 56 insertions(+), 5 deletions(-) create mode 100644 app/models/notification/service_token_expired.rb create mode 100644 app/models/notification/twitter_token_expired.rb create mode 100644 app/models/user/expired_service_connection.rb create mode 100644 app/models/user/expired_twitter_service_connection.rb create mode 100644 app/views/notifications/type/_expiredtwitterserviceconnection.html.haml diff --git a/app/javascript/styles/components/_notifications.scss b/app/javascript/styles/components/_notifications.scss index 9ab69bc3..07ce4e23 100644 --- a/app/javascript/styles/components/_notifications.scss +++ b/app/javascript/styles/components/_notifications.scss @@ -8,6 +8,7 @@ &__user, &__text { margin-bottom: 0; + white-space: normal; } &__icon { @@ -36,7 +37,7 @@ &-dropdown { min-width: 400px; - & .dropdown-item:hover, + & .dropdown-item:hover, & .dropdown-item:active { background: transparent; color: RGB(var(--body-text)); @@ -55,4 +56,4 @@ margin-bottom: 0; } } -} \ No newline at end of file +} diff --git a/app/models/notification/service_token_expired.rb b/app/models/notification/service_token_expired.rb new file mode 100644 index 00000000..bc61a820 --- /dev/null +++ b/app/models/notification/service_token_expired.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class Notification::ServiceTokenExpired < Notification +end diff --git a/app/models/notification/twitter_token_expired.rb b/app/models/notification/twitter_token_expired.rb new file mode 100644 index 00000000..9ddb8447 --- /dev/null +++ b/app/models/notification/twitter_token_expired.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class Notification::TwitterTokenExpired < Notification::ServiceTokenExpired +end diff --git a/app/models/user/expired_service_connection.rb b/app/models/user/expired_service_connection.rb new file mode 100644 index 00000000..110dd896 --- /dev/null +++ b/app/models/user/expired_service_connection.rb @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +# stub model for notifying about expired service connections +class User::ExpiredServiceConnection < User +end diff --git a/app/models/user/expired_twitter_service_connection.rb b/app/models/user/expired_twitter_service_connection.rb new file mode 100644 index 00000000..bb39250e --- /dev/null +++ b/app/models/user/expired_twitter_service_connection.rb @@ -0,0 +1,4 @@ +# frozen_string_literal: true + +class User::ExpiredTwitterServiceConnection < User::ExpiredServiceConnection +end diff --git a/app/views/notifications/type/_expiredtwitterserviceconnection.html.haml b/app/views/notifications/type/_expiredtwitterserviceconnection.html.haml new file mode 100644 index 00000000..36014390 --- /dev/null +++ b/app/views/notifications/type/_expiredtwitterserviceconnection.html.haml @@ -0,0 +1,8 @@ +.media.notification + .notification__icon + %i.fa.fa-2x.fa-fw.fa-twitter + .media-body + %h6.media-heading.notification__user + = t(".heading") + .notification__text + = t(".text_html", settings_sharing: link_to(t(".settings_services"), services_path)) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 9f4ebd15..2709c074 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -9,8 +9,7 @@ class ShareWorker # @param answer_id [Integer] the user id # @param service [String] the service to post to def perform(user_id, answer_id, service) - service_type = "Services::#{service.camelize}" - user_service = User.find(user_id).services.find_by(type: service_type) + user_service = find_service(user_id, service) user_service.post(Answer.find(answer_id)) rescue ActiveRecord::RecordNotFound @@ -23,11 +22,21 @@ class ShareWorker logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the account is suspended." rescue Twitter::Error::Unauthorized # User's Twitter token has expired or been revoked - # TODO: Notify user if this happens (https://github.com/Retrospring/retrospring/issues/123) logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has exired or been revoked." + user_service = find_service(user_id, service) + user_service.destroy + + Object.const_get("Notification::ServiceTokenExpired").create( + target_id: user_id, + target_type: "User::Expired#{service.camelize}ServiceConnection", + recipient_id: user_id, + new: true + ) rescue => e logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" Sentry.capture_exception(e) raise end + + def find_service(user_id, service) = User.find(user_id).services.find_by(type: "Services::#{service.camelize}") end diff --git a/config/locales/views.en.yml b/config/locales/views.en.yml index ad5532c3..7d427b81 100644 --- a/config/locales/views.en.yml +++ b/config/locales/views.en.yml @@ -352,6 +352,10 @@ en: link_text: "your comment" follow: heading_html: "followed you %{time} ago" + expiredtwitterserviceconnection: + heading: "Twitter connection expired" + text_html: "If you would like to continue automatically sharing your answers to Twitter, head to %{settings_sharing} and re-connect your account." + settings_services: "Sharing Settings" services: index: title: "Service Settings" diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index e9c6d23e..b98337e3 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -59,6 +59,18 @@ describe ShareWorker do expect(Sidekiq.logger).to have_received(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has exired or been revoked.") end + it "revokes the service connection when Twitter::Error::Unauthorized is raised" do + allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized) + subject + expect { ShareWorker.drain }.to change { Services::Twitter.count }.by(-1) + end + + it "sends the user a notification when Twitter::Error::Unauthorized is raised" do + allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized) + subject + expect { ShareWorker.drain }.to change { Notification::ServiceTokenExpired.count }.by(1) + end + it "handles Twitter::Error::Forbidden" do allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Forbidden) subject From baffd05d6fd8f4d858bada7cb27e255bfa7c2f1f Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 1 Jan 2023 23:52:46 +0100 Subject: [PATCH 02/10] Appease the dog overlords --- app/workers/share_worker.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 2709c074..8b5cab83 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -23,6 +23,14 @@ class ShareWorker rescue Twitter::Error::Unauthorized # User's Twitter token has expired or been revoked logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has exired or been revoked." + revoke_and_notify(user_id, service) + rescue => e + logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" + Sentry.capture_exception(e) + raise + end + + def revoke_and_notify(user_id, service) user_service = find_service(user_id, service) user_service.destroy @@ -32,10 +40,6 @@ class ShareWorker recipient_id: user_id, new: true ) - rescue => e - logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" - Sentry.capture_exception(e) - raise end def find_service(user_id, service) = User.find(user_id).services.find_by(type: "Services::#{service.camelize}") From d320a74045454f16aac14b8f7ce494ad3ff20651 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 1 Jan 2023 23:58:21 +0100 Subject: [PATCH 03/10] Mark notification as read when visiting service settings --- app/controllers/services_controller.rb | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb index bcf5aae2..deaa39a0 100644 --- a/app/controllers/services_controller.rb +++ b/app/controllers/services_controller.rb @@ -2,6 +2,7 @@ class ServicesController < ApplicationController before_action :authenticate_user! + before_action :mark_notifications_as_read, only: %i[index] def index @services = current_user.services @@ -59,4 +60,10 @@ class ServicesController < ApplicationController def omniauth_hash request.env["omniauth.auth"] end + + def mark_notifications_as_read + Notification::ServiceTokenExpired + .where(recipient: current_user, new: true) + .update_all(new: false) # rubocop:disable Rails/SkipsModelValidations + end end From 02bcfb3c9ee61444d0180c1549238f0c77ed78f3 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:09:29 +0100 Subject: [PATCH 04/10] Move user service into an instance variable Co-authored-by: nilsding --- app/workers/share_worker.rb | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 8b5cab83..a0fd3fd4 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -9,9 +9,9 @@ class ShareWorker # @param answer_id [Integer] the user id # @param service [String] the service to post to def perform(user_id, answer_id, service) - user_service = find_service(user_id, service) + @user_service = User.find(user_id).services.find_by(type: "Services::#{service.camelize}") - user_service.post(Answer.find(answer_id)) + @user_service.post(Answer.find(answer_id)) rescue ActiveRecord::RecordNotFound logger.info "Tried to post answer ##{answer_id} for user ##{user_id} to #{service.titleize} but the user/answer/service did not exist (likely deleted), will not retry." # The question to be posted was deleted @@ -31,8 +31,7 @@ class ShareWorker end def revoke_and_notify(user_id, service) - user_service = find_service(user_id, service) - user_service.destroy + @user_service.destroy Object.const_get("Notification::ServiceTokenExpired").create( target_id: user_id, @@ -41,6 +40,4 @@ class ShareWorker new: true ) end - - def find_service(user_id, service) = User.find(user_id).services.find_by(type: "Services::#{service.camelize}") end From 0f01177c67adcce01f1399af213bd7b27b5317b8 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:12:55 +0100 Subject: [PATCH 05/10] Reformat services controller spec --- spec/controllers/services_controller_spec.rb | 52 ++++++++++---------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/spec/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb index 7c91817d..ee6e4038 100644 --- a/spec/controllers/services_controller_spec.rb +++ b/spec/controllers/services_controller_spec.rb @@ -1,17 +1,19 @@ -require 'rails_helper' +# frozen_string_literal: true + +require "rails_helper" describe ServicesController, type: :controller do - context 'successful Twitter sign in' do + context "successful Twitter sign in" do let(:user) { FactoryBot.create(:user) } before do sign_in user OmniAuth.config.test_mode = true OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new({ - 'provider' => 'twitter', - 'uid' => '12', - 'info' => { 'nickname' => 'jack' }, - 'credentials' => { 'token' => 'AAAA', 'secret' => 'BBBB' } + "provider" => "twitter", + "uid" => "12", + "info" => { "nickname" => "jack" }, + "credentials" => { "token" => "AAAA", "secret" => "BBBB" } }) request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter] end @@ -20,58 +22,58 @@ describe ServicesController, type: :controller do OmniAuth.config.mock_auth[:twitter] = nil end - subject { get :create, params: { provider: 'twitter' } } + subject { get :create, params: { provider: "twitter" } } - context 'no services connected' do - it 'creates a service integration' do + context "no services connected" do + it "creates a service integration" do expect { subject }.to change { Service.count }.by(1) end end - context 'a user has a service connected' do + context "a user has a service connected" do let(:other_user) { FactoryBot.create(:user) } let!(:service) { Services::Twitter.create(user: other_user, uid: 12) } - it 'shows an error when trying to attach a service account which is already connected' do + it "shows an error when trying to attach a service account which is already connected" do subject expect(flash[:error]).to eq("The Twitter account you are trying to connect is already connected to another #{APP_CONFIG['site_name']} account. If you are unable to disconnect the account yourself, please send us a Direct Message on Twitter: @retrospring.") end end end - context '#update' do - subject { patch :update, params: params } + context "#update" do + subject { patch :update, params: } - context 'not signed in' do + context "not signed in" do let(:params) { { id: 1 } } - it 'redirects to sign in page' do + it "redirects to sign in page" do subject expect(response).to redirect_to(new_user_session_path) end end - context 'user with Twitter connection' do + context "user with Twitter connection" do before { sign_in user } let(:user) { FactoryBot.create(:user) } - let(:service) { Services::Twitter.create(user: user, uid: 12) } - let(:params) { { id: service.id, service: { post_tag: post_tag } } } + let(:service) { Services::Twitter.create(user:, uid: 12) } + let(:params) { { id: service.id, service: { post_tag: } } } - context 'tag is valid' do - let(:post_tag) { '#askaraccoon' } + context "tag is valid" do + let(:post_tag) { "#askaraccoon" } - it 'updates a service connection' do - expect { subject }.to change { service.reload.post_tag }.to('#askaraccoon') + it "updates a service connection" do + expect { subject }.to change { service.reload.post_tag }.to("#askaraccoon") expect(response).to redirect_to(services_path) expect(flash[:success]).to eq("Service updated successfully.") end end - context 'tag is too long' do - let(:post_tag) { 'a' * 21 } # 1 character over the limit + context "tag is too long" do + let(:post_tag) { "a" * 21 } # 1 character over the limit - it 'shows an error' do + it "shows an error" do subject expect(response).to redirect_to(services_path) expect(flash[:error]).to eq("Unable to update service.") From f80d4ce9351a308c20ea5b6971250e399f391c92 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:15:51 +0100 Subject: [PATCH 06/10] Move create test into a describe block --- spec/controllers/services_controller_spec.rb | 62 ++++++++++---------- 1 file changed, 32 insertions(+), 30 deletions(-) diff --git a/spec/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb index ee6e4038..413009e0 100644 --- a/spec/controllers/services_controller_spec.rb +++ b/spec/controllers/services_controller_spec.rb @@ -3,45 +3,47 @@ require "rails_helper" describe ServicesController, type: :controller do - context "successful Twitter sign in" do - let(:user) { FactoryBot.create(:user) } - - before do - sign_in user - OmniAuth.config.test_mode = true - OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new({ - "provider" => "twitter", - "uid" => "12", - "info" => { "nickname" => "jack" }, - "credentials" => { "token" => "AAAA", "secret" => "BBBB" } - }) - request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter] - end - - after do - OmniAuth.config.mock_auth[:twitter] = nil - end - + describe "#create" do subject { get :create, params: { provider: "twitter" } } - context "no services connected" do - it "creates a service integration" do - expect { subject }.to change { Service.count }.by(1) + context "successful Twitter sign in" do + let(:user) { FactoryBot.create(:user) } + + before do + sign_in user + OmniAuth.config.test_mode = true + OmniAuth.config.mock_auth[:twitter] = OmniAuth::AuthHash.new({ + "provider" => "twitter", + "uid" => "12", + "info" => { "nickname" => "jack" }, + "credentials" => { "token" => "AAAA", "secret" => "BBBB" } + }) + request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter] end - end - context "a user has a service connected" do - let(:other_user) { FactoryBot.create(:user) } - let!(:service) { Services::Twitter.create(user: other_user, uid: 12) } + after do + OmniAuth.config.mock_auth[:twitter] = nil + end - it "shows an error when trying to attach a service account which is already connected" do - subject - expect(flash[:error]).to eq("The Twitter account you are trying to connect is already connected to another #{APP_CONFIG['site_name']} account. If you are unable to disconnect the account yourself, please send us a Direct Message on Twitter: @retrospring.") + context "no services connected" do + it "creates a service integration" do + expect { subject }.to change { Service.count }.by(1) + end + end + + context "a user has a service connected" do + let(:other_user) { FactoryBot.create(:user) } + let!(:service) { Services::Twitter.create(user: other_user, uid: 12) } + + it "shows an error when trying to attach a service account which is already connected" do + subject + expect(flash[:error]).to eq("The Twitter account you are trying to connect is already connected to another #{APP_CONFIG['site_name']} account. If you are unable to disconnect the account yourself, please send us a Direct Message on Twitter: @retrospring.") + end end end end - context "#update" do + describe "#update" do subject { patch :update, params: } context "not signed in" do From 7766c9bd5e75d124956db683eff2fd510f7c8c36 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:30:38 +0100 Subject: [PATCH 07/10] Add tests for services settings page --- spec/controllers/services_controller_spec.rb | 43 ++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/spec/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb index 413009e0..5e9582ab 100644 --- a/spec/controllers/services_controller_spec.rb +++ b/spec/controllers/services_controller_spec.rb @@ -3,6 +3,49 @@ require "rails_helper" describe ServicesController, type: :controller do + describe "#index" do + subject { get :index } + + context "user signed in" do + let(:user) { FactoryBot.create(:user) } + + before { sign_in user } + + it "renders the services settings page with no services" do + subject + expect(response).to render_template("index") + expect(controller.instance_variable_get(:@services)).to be_empty + end + + context "user has a service token expired notification" do + let(:notification) do + Notification::ServiceTokenExpired.create( + target_id: user.id, + target_type: "User::ExpiredTwitterServiceConnection", + recipient_id: user.id, + new: true + ) + end + + it "marks the notification as read" do + expect { subject }.to change { notification.reload.new }.from(true).to(false) + end + end + + context "user has Twitter connected" do + before do + Services::Twitter.create(user:, uid: 12) + end + + it "renders the services settings page" do + subject + expect(response).to render_template("index") + expect(controller.instance_variable_get(:@services)).not_to be_empty + end + end + end + end + describe "#create" do subject { get :create, params: { provider: "twitter" } } From 0241a02e299700030f2e090679b03682cac2f0df Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:30:51 +0100 Subject: [PATCH 08/10] Remove unnecessary `Object.const_get` --- app/workers/share_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index a0fd3fd4..321d0614 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -33,7 +33,7 @@ class ShareWorker def revoke_and_notify(user_id, service) @user_service.destroy - Object.const_get("Notification::ServiceTokenExpired").create( + Notification::ServiceTokenExpired.create( target_id: user_id, target_type: "User::Expired#{service.camelize}ServiceConnection", recipient_id: user_id, From 873d6a2c88b114a6ccc6429fdcdeffb5882aa1e6 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:36:28 +0100 Subject: [PATCH 09/10] Fix typo in share worker --- app/workers/share_worker.rb | 2 +- spec/workers/share_worker_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 321d0614..6f6b39b8 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -22,7 +22,7 @@ class ShareWorker logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the account is suspended." rescue Twitter::Error::Unauthorized # User's Twitter token has expired or been revoked - logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has exired or been revoked." + logger.info "Tried to post answer ##{answer_id} from user ##{user_id} to Twitter but the token has expired or been revoked." revoke_and_notify(user_id, service) rescue => e logger.info "failed to post answer #{answer_id} to #{service} for user #{user_id}: #{e.message}" diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index b98337e3..c8003258 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -56,7 +56,7 @@ describe ShareWorker do allow_any_instance_of(Services::Twitter).to receive(:post).with(answer).and_raise(Twitter::Error::Unauthorized) subject ShareWorker.drain - expect(Sidekiq.logger).to have_received(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has exired or been revoked.") + expect(Sidekiq.logger).to have_received(:info).with("Tried to post answer ##{answer.id} from user ##{user.id} to Twitter but the token has expired or been revoked.") end it "revokes the service connection when Twitter::Error::Unauthorized is raised" do From 81c9870af4b2d8ba8a3f0ee7166c7b2fad7ccfea Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Mon, 2 Jan 2023 09:41:14 +0100 Subject: [PATCH 10/10] Ignore Metrics/AbcSize in share worker --- app/workers/share_worker.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/workers/share_worker.rb b/app/workers/share_worker.rb index 6f6b39b8..e0385ca1 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -8,7 +8,7 @@ class ShareWorker # @param user_id [Integer] the user id # @param answer_id [Integer] the user id # @param service [String] the service to post to - def perform(user_id, answer_id, service) + def perform(user_id, answer_id, service) # rubocop:disable Metrics/AbcSize @user_service = User.find(user_id).services.find_by(type: "Services::#{service.camelize}") @user_service.post(Answer.find(answer_id))