diff --git a/app/models/user.rb b/app/models/user.rb index 75579290e..fe36da178 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -39,6 +39,10 @@ class User < ApplicationRecord unless: ->(user) { user.sufficient_eligible_prs? } end + event :deactivate do + transition all => :inactive + end + state all - [:new] do validates :terms_acceptance, acceptance: true validates :email, presence: true @@ -52,7 +56,7 @@ class User < ApplicationRecord validates :sticker_coupon, absence: true end - state all - %i[new registered waiting] do + state all - %i[new registered waiting inactive] do validates :receipt, presence: true end @@ -90,6 +94,16 @@ def win in: [false], message: 'user has too many sufficient eligible prs' } end + state :inactive do + validates :last_error, inclusion: { + in: ['GithubPullRequestService::UserNotFoundOnGithubError', + 'Octokit::AccountSuspended'], + message: "user's last_error must be + UserNotFoundOnGithubError or AccountSuspended" } + + validates :state_before_inactive, presence: true + end + before_transition do |user, _transition| UserPullRequestSegmentUpdaterService.call(user) end @@ -102,6 +116,10 @@ def win user.receipt = user.scoring_pull_requests end + before_transition to: :inactive do |user, transition| + user.update_state_before_inactive(transition) + end + after_transition to: :waiting, do: :update_waiting_since after_transition to: :completed do |user, _transition| @@ -148,6 +166,10 @@ def update_waiting_since )) end + def update_state_before_inactive(transition) + self.state_before_inactive = transition.from + end + def waiting_for_week? return false if waiting_since.nil? diff --git a/app/services/github_error_handler.rb b/app/services/github_error_handler.rb new file mode 100644 index 000000000..4c842621b --- /dev/null +++ b/app/services/github_error_handler.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module GithubErrorHandler + module_function + + HANDLED_ERRORS = [ + Octokit::AccountSuspended, + GithubPullRequestService::UserNotFoundOnGithubError + ].freeze + + def with_error_handling(user) + yield + rescue *HANDLED_ERRORS => e + process_github_error(e, user) + end + + def process_github_error(error, user) + # rubocop:disable Rails/SkipsModelValidations + user.update_attribute(:last_error, error.class) + # rubocop:enable Rails/SkipsModelValidations + case error + when Octokit::AccountSuspended + process_user_missing_error(error, user) + when GithubPullRequestService::UserNotFoundOnGithubError + process_user_missing_error(error, user) + else + raise error + end + end + + def process_user_missing_error(_error, user) + user.deactivate + + return unless (user_stat = UserStat.where(user_id: user.id).first) + + user_stat.destroy + end +end diff --git a/app/services/import_pr_metadata_service.rb b/app/services/import_pr_metadata_service.rb index efa5f8fc5..51c82bd12 100644 --- a/app/services/import_pr_metadata_service.rb +++ b/app/services/import_pr_metadata_service.rb @@ -6,17 +6,12 @@ module ImportPrMetadataService def call(user) pr_service = PullRequestService.new(user, randomize_token: true) - begin + GithubErrorHandler.with_error_handling(user) do pr_data = pr_service.all - rescue GithubPullRequestService::UserNotFoundOnGithubError - user_stat = UserStat.where(user_id: user.id).first - user_stat.destroy - return - end - - pr_data.map do |pr| - ImportOnePrMetadataJob.perform_async(pr.url) + pr_data.map do |pr| + ImportOnePrMetadataJob.perform_async(pr.url) + end end end end diff --git a/app/services/import_repos_metadata_service.rb b/app/services/import_repos_metadata_service.rb index c7c524a40..f52e3847b 100644 --- a/app/services/import_repos_metadata_service.rb +++ b/app/services/import_repos_metadata_service.rb @@ -6,18 +6,13 @@ module ImportReposMetadataService def call(user) pr_service = PullRequestService.new(user, randomize_token: true) - begin + GithubErrorHandler.with_error_handling(user) do pr_data = pr_service.all - rescue GithubPullRequestService::UserNotFoundOnGithubError - user_stat = UserStat.where(user_id: user.id).first - user_stat.destroy - return - end - - pr_data.map do |pr| - repo_id = pr.github_pull_request.repo_id - ImportRepoMetadataJob.perform_async(repo_id) + pr_data.map do |pr| + repo_id = pr.github_pull_request.repo_id + ImportRepoMetadataJob.perform_async(repo_id) + end end end end diff --git a/app/services/try_user_transition_service.rb b/app/services/try_user_transition_service.rb index 32430cf7b..382cc963e 100644 --- a/app/services/try_user_transition_service.rb +++ b/app/services/try_user_transition_service.rb @@ -2,13 +2,15 @@ module TryUserTransitionService def self.call(user) - case user.state - when 'registered' - TryUserTransitionFromRegisteredService.call(user) - when 'waiting' - TryUserTransitionFromWaitingService.call(user) - when 'completed' - TryUserTransitionFromCompletedService.call(user) + GithubErrorHandler.with_error_handling(user) do + case user.state + when 'registered' + TryUserTransitionFromRegisteredService.call(user) + when 'waiting' + TryUserTransitionFromWaitingService.call(user) + when 'completed' + TryUserTransitionFromCompletedService.call(user) + end end end end diff --git a/app/services/user_state_transition_segment_service.rb b/app/services/user_state_transition_segment_service.rb index 7e9770d5d..465cdbec6 100644 --- a/app/services/user_state_transition_segment_service.rb +++ b/app/services/user_state_transition_segment_service.rb @@ -15,6 +15,7 @@ def call(user, transition) when :incomplete then incomplete(user) when :ineligible then ineligible(user) when :won then won(user, transition) + when :deactivate then deactivate(user) end end @@ -61,6 +62,11 @@ def won(user, transition) end end + def deactivate(user) + segment(user).track('user_inactive') + segment(user).identify(state: 'inactive') + end + def segment(user) SegmentService.new(user) end diff --git a/db/migrate/20191107151321_add_last_error_to_user.rb b/db/migrate/20191107151321_add_last_error_to_user.rb new file mode 100644 index 000000000..582820ba4 --- /dev/null +++ b/db/migrate/20191107151321_add_last_error_to_user.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddLastErrorToUser < ActiveRecord::Migration[5.2] + def change + add_column :users, :last_error, :string + end +end diff --git a/db/migrate/20191111195936_add_state_before_inactive_to_user.rb b/db/migrate/20191111195936_add_state_before_inactive_to_user.rb new file mode 100644 index 000000000..5e32b7ef7 --- /dev/null +++ b/db/migrate/20191111195936_add_state_before_inactive_to_user.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddStateBeforeInactiveToUser < ActiveRecord::Migration[5.2] + def change + add_column :users, :state_before_inactive, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 26a9c8975..e735e05d0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,11 +10,19 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_10_28_180239) do +ActiveRecord::Schema.define(version: 2019_11_11_195936) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" + create_table "daily_pr_counts", force: :cascade do |t| + t.date "date", null: false + t.integer "count", default: 0 + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["date"], name: "index_daily_pr_counts_on_date", unique: true + end + create_table "issues", force: :cascade do |t| t.string "title", limit: 191, null: false t.bigint "number", null: false @@ -104,6 +112,7 @@ t.integer "user_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "deleted" end create_table "users", force: :cascade do |t| @@ -119,6 +128,8 @@ t.string "state" t.datetime "waiting_since" t.jsonb "receipt" + t.string "last_error" + t.string "state_before_inactive" end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index bb3ddab1e..e411c21b6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -559,5 +559,49 @@ end end end + + describe '#deactivate' do + before do + allow(UserStateTransitionSegmentService) + .to receive(:call).and_return(true) + + allow_any_instance_of(User) + .to receive(:last_error).and_return('Octokit::AccountSuspended') + end + + context 'the user is in any state' do + let(:user) { FactoryBot.create(:user) } + context 'the state_before_inactive gets set successfully' do + before { user.deactivate } + + it 'transitions the user to the inactive state' do + expect(user.state).to eq('inactive') + end + + it 'saves the previous state as state_before_inactive' do + expect(user.state_before_inactive).to eq('registered') + end + end + + context 'the state_before_inactive does not get set' do + before do + allow(user).to receive(:state_before_inactive).and_return(nil) + end + + it 'does not transition the user to the inactive state' do + user.deactivate + + expect(user.state).to_not eq('inactive') + end + + it 'applies the correct errors to the user object' do + user.deactivate + + expect(user.errors.messages[:state_before_inactive].first) + .to eq("can't be blank") + end + end + end + end end end diff --git a/spec/services/github_error_handler_spec.rb b/spec/services/github_error_handler_spec.rb new file mode 100644 index 000000000..2cd5e2937 --- /dev/null +++ b/spec/services/github_error_handler_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe GithubErrorHandler do + let(:user) { FactoryBot.create(:user) } + + # stub API call that will happen due to User#deactivate + before do + allow(UserStateTransitionSegmentService) + .to receive(:call).and_return(true) + end + + describe '.process_github_error' do + context 'The error is UserNotFoundOnGithubError' do + let(:error) { GithubPullRequestService::UserNotFoundOnGithubError.new } + + it 'calls process_user_missing_error' do + expect(GithubErrorHandler) + .to receive(:process_user_missing_error).with(error, user) + + GithubErrorHandler.process_github_error(error, user) + end + + it 'correctly updates the last_error column on the user' do + GithubErrorHandler.process_github_error(error, user) + + expect(user.last_error).to eq(error.class.to_s) + end + end + + context 'The error is Octokit::AccountSuspended' do + let(:error) { Octokit::AccountSuspended.new } + + it 'calls process_user_missing_error' do + expect(GithubErrorHandler) + .to receive(:process_user_missing_error).with(error, user) + + GithubErrorHandler.process_github_error(error, user) + end + + it 'correctly updates the last_error column on the user' do + GithubErrorHandler.process_github_error(error, user) + + expect(user.last_error).to eq(error.class.to_s) + end + end + + context 'The error is an unhandled error' do + let(:error) { StandardError.new } + + it 'raises the error' do + expect { GithubErrorHandler.process_github_error(error, user) } + .to raise_error(StandardError) + end + end + end + + describe '.process_user_missing_error' do + let(:error) { GithubPullRequestService::UserNotFoundOnGithubError.new } + it 'destroys the UserStat' do + UserStat.create(user_id: user.id, data: user) + + GithubErrorHandler.process_user_missing_error(error, user) + + expect(UserStat.count).to eq(0) + end + + it 'deactivates the user' do + # to pass user#deactivate validations + allow(user) + .to receive(:last_error).and_return('Octokit::AccountSuspended') + + GithubErrorHandler.process_user_missing_error(error, user) + + expect(user.state).to eq('inactive') + end + end +end diff --git a/spec/services/import_repos_metadata_service_spec.rb b/spec/services/import_repos_metadata_service_spec.rb index 1a5560abf..b74b0bb0a 100644 --- a/spec/services/import_repos_metadata_service_spec.rb +++ b/spec/services/import_repos_metadata_service_spec.rb @@ -5,6 +5,13 @@ RSpec.describe ImportReposMetadataService do describe '.call' do let(:user) { FactoryBot.create(:user) } + + # stub API call that will happen due to User#deactivate + before do + allow(UserStateTransitionSegmentService) + .to receive(:call).and_return(true) + end + context 'The user has no PRs' do before do allow_any_instance_of(PullRequestService)