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 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..e0385ca1 100644 --- a/app/workers/share_worker.rb +++ b/app/workers/share_worker.rb @@ -8,11 +8,10 @@ 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) - service_type = "Services::#{service.camelize}" - user_service = User.find(user_id).services.find_by(type: service_type) + 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)) + @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 @@ -23,11 +22,22 @@ 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." + 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}" Sentry.capture_exception(e) raise end + + def revoke_and_notify(user_id, service) + @user_service.destroy + + Notification::ServiceTokenExpired.create( + target_id: user_id, + target_type: "User::Expired#{service.camelize}ServiceConnection", + recipient_id: user_id, + new: true + ) + end 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/controllers/services_controller_spec.rb b/spec/controllers/services_controller_spec.rb index 7c91817d..5e9582ab 100644 --- a/spec/controllers/services_controller_spec.rb +++ b/spec/controllers/services_controller_spec.rb @@ -1,77 +1,124 @@ -require 'rails_helper' +# frozen_string_literal: true + +require "rails_helper" describe ServicesController, type: :controller do - context 'successful Twitter sign in' do - let(:user) { FactoryBot.create(:user) } + describe "#index" do + subject { get :index } - 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 + context "user signed in" do + let(:user) { FactoryBot.create(:user) } - after do - OmniAuth.config.mock_auth[:twitter] = nil - end + before { sign_in user } - 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) - 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 + it "renders the services settings page with no services" 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.") + 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 - context '#update' do - subject { patch :update, params: params } + describe "#create" do + subject { get :create, params: { provider: "twitter" } } - context 'not signed 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" } + }) + request.env["omniauth.auth"] = OmniAuth.config.mock_auth[:twitter] + end + + after do + OmniAuth.config.mock_auth[:twitter] = nil + end + + 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 + + describe "#update" do + subject { patch :update, params: } + + 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.") diff --git a/spec/workers/share_worker_spec.rb b/spec/workers/share_worker_spec.rb index e9c6d23e..c8003258 100644 --- a/spec/workers/share_worker_spec.rb +++ b/spec/workers/share_worker_spec.rb @@ -56,7 +56,19 @@ 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 + 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