From 8d2ca2adc62c91244511fe05f3b5d8e1341566b8 Mon Sep 17 00:00:00 2001 From: "Patrick J. Cherry" Date: Mon, 31 Oct 2022 12:40:29 +0000 Subject: [PATCH 1/4] Add Sentry, and refactor operations to use it --- Gemfile | 1 + Gemfile.lock | 6 ++++ app/concepts/project/operation/create.rb | 23 ++++++++---- .../project/operation/create_remix.rb | 14 ++++---- app/concepts/project/operation/update.rb | 36 +++++++++---------- config/initializers/sentry.rb | 9 +++++ 6 files changed, 58 insertions(+), 31 deletions(-) create mode 100644 config/initializers/sentry.rb diff --git a/Gemfile b/Gemfile index 6f9eeb837..69b7428cb 100644 --- a/Gemfile +++ b/Gemfile @@ -15,6 +15,7 @@ gem 'pg', '~> 1.1' gem 'puma', '~> 5.6' gem 'rack-cors' gem 'rails', '~> 7.0.0' +gem 'sentry-rails', '~> 5.5.0' gem 'sprockets-rails' gem 'stimulus-rails' gem 'turbo-rails' diff --git a/Gemfile.lock b/Gemfile.lock index 0c4c14262..7580d75dc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -247,6 +247,11 @@ GEM rubocop (~> 1.19) ruby-progressbar (1.11.0) ruby2_keywords (0.0.5) + sentry-rails (5.5.0) + railties (>= 5.0) + sentry-ruby (~> 5.5.0) + sentry-ruby (5.5.0) + concurrent-ruby (~> 1.0, >= 1.0.2) shoulda-matchers (5.0.0) activesupport (>= 5.2.0) simplecov (0.21.2) @@ -308,6 +313,7 @@ DEPENDENCIES rubocop rubocop-rails rubocop-rspec + sentry-rails (~> 5.5.0) shoulda-matchers (~> 5.0) simplecov sprockets-rails diff --git a/app/concepts/project/operation/create.rb b/app/concepts/project/operation/create.rb index 2fa9da926..2bd6c3e3c 100644 --- a/app/concepts/project/operation/create.rb +++ b/app/concepts/project/operation/create.rb @@ -1,13 +1,22 @@ # frozen_string_literal: true +require 'operation_response' + class Project module Operation class Create - require 'operation_response' - - DEFAULT_COMPONENT = { name: 'main', extension: 'py', default: true, index: 0 }.freeze - DEFAULT_PROJECT = { type: 'python', name: 'Untitled project', components: [DEFAULT_COMPONENT], - image_list: [] }.freeze + DEFAULT_COMPONENT = { + name: 'main', + extension: 'py', + default: true, + index: 0 + }.freeze + DEFAULT_PROJECT = { + type: 'python', + name: 'Untitled project', + components: [DEFAULT_COMPONENT], + image_list: [] + }.freeze class << self def call(user_id:, params:) @@ -26,8 +35,8 @@ def call(user_id:, params:) response[:project] = new_project response[:project].save! response - rescue StandardError - # TODO: log error + rescue StandardError => e + Sentry.capture_exception(e) response[:error] = 'Error creating project' response end diff --git a/app/concepts/project/operation/create_remix.rb b/app/concepts/project/operation/create_remix.rb index 42cea7ac2..4530a3c1f 100644 --- a/app/concepts/project/operation/create_remix.rb +++ b/app/concepts/project/operation/create_remix.rb @@ -1,16 +1,19 @@ # frozen_string_literal: true +require 'operation_response' class Project module Operation class CreateRemix - require 'operation_response' - class << self def call(params:, user_id:, original_project:) response = OperationResponse.new validate_params(response, params, user_id, original_project) - remix_project(response, params, user_id, original_project) + remix_project(response, params, user_id, original_project) if response.success? + response + rescue StandardError => e + Sentry.capture_exception(e) + response[:error] = I18n.t('errors.project.remixing.cannot_save') response end @@ -22,11 +25,9 @@ def validate_params(response, params, user_id, original_project) end def remix_project(response, params, user_id, original_project) - return if response[:error] - response[:project] = create_remix(original_project, params, user_id) - response[:error] = I18n.t('errors.project.remixing.cannot_save') unless response[:project].save + response[:project].save! response end @@ -43,6 +44,7 @@ def create_remix(original_project, params, user_id) params[:components].each do |x| remix.components.build(x.slice(:name, :extension, :content, :index)) end + remix end end diff --git a/app/concepts/project/operation/update.rb b/app/concepts/project/operation/update.rb index cfde67f70..3d2e48787 100644 --- a/app/concepts/project/operation/update.rb +++ b/app/concepts/project/operation/update.rb @@ -1,22 +1,25 @@ # frozen_string_literal: true +require 'operation_response' + class Project module Operation class Update - require 'operation_response' - - def self.call(params:, project:) - response = setup_response(project) - - setup_deletions(response, params) - update_project_attributes(response, params) - update_component_attributes(response, params) - persist_changes(response) + class << self + def call(params:, project:) + response = setup_response(project) - response - end + setup_deletions(response, params) + update_project_attributes(response, params) + update_component_attributes(response, params) + persist_changes(response) + response + rescue StandardError => e + Sentry.capture_exception(e) + response[:error] ||= 'Error persisting changes' + response + end - class << self private def setup_response(project) @@ -41,13 +44,13 @@ def validate_deletions(response) end def update_project_attributes(response, params) - return if response[:error] + return if response.failure? response[:project].assign_attributes(params.slice(:name)) end def update_component_attributes(response, params) - return if response[:error] + return if response.failure? params[:components].each do |component_params| if component_params[:id].present? @@ -60,14 +63,11 @@ def update_component_attributes(response, params) end def persist_changes(response) - return if response[:error] + return if response.failure? ActiveRecord::Base.transaction do response[:project].save! response[:project].components.where(id: response[:component_ids_to_delete]).destroy_all - rescue StandardError - # TODO: log error? - response[:error] = 'Error persisting changes' end end end diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb new file mode 100644 index 000000000..22f7cf011 --- /dev/null +++ b/config/initializers/sentry.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +Sentry.init do |config| + config.dsn = ENV.fetch('SENTRY_DSN', nil) if Rails.env.production? + config.breadcrumbs_logger = [:active_support_logger] + config.environment = ENV.fetch('SENTRY_CURRENT_ENV', nil) || ENV.fetch('RAILS_ENV', nil) + + config.traces_sample_rate = 0.5 +end From 0bb5cf2e940920fde9b1780472c9b64961837cc3 Mon Sep 17 00:00:00 2001 From: "Patrick J. Cherry" Date: Mon, 31 Oct 2022 13:25:14 +0000 Subject: [PATCH 2/4] Rubocop --- app/concepts/project/operation/create_remix.rb | 1 + config/initializers/auth.rb | 2 +- lib/operation_response.rb | 4 +--- spec/concepts/project/operation/create_remix_spec.rb | 8 ++++---- spec/concepts/project/operation/create_spec.rb | 6 +++--- .../project/operation/update_default_component_spec.rb | 2 +- .../project/operation/update_delete_components_spec.rb | 2 +- spec/concepts/project/operation/update_invalid_spec.rb | 2 +- spec/concepts/project/operation/update_spec.rb | 4 ++-- spec/lib/operation_response_spec.rb | 8 ++++---- spec/models/component_spec.rb | 4 ++-- spec/request/projects/index_spec.rb | 2 +- 12 files changed, 22 insertions(+), 23 deletions(-) diff --git a/app/concepts/project/operation/create_remix.rb b/app/concepts/project/operation/create_remix.rb index 4530a3c1f..0f49cd77a 100644 --- a/app/concepts/project/operation/create_remix.rb +++ b/app/concepts/project/operation/create_remix.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'operation_response' class Project diff --git a/config/initializers/auth.rb b/config/initializers/auth.rb index cd170d065..9b97cd4e3 100644 --- a/config/initializers/auth.rb +++ b/config/initializers/auth.rb @@ -1,4 +1,4 @@ # frozen_string_literal: true BYPASS_AUTH = ENV['BYPASS_AUTH'] == 'true' -AUTH_USER_ID = ENV['AUTH_USER_ID'] if BYPASS_AUTH +AUTH_USER_ID = ENV.fetch('AUTH_USER_ID', nil) if BYPASS_AUTH diff --git a/lib/operation_response.rb b/lib/operation_response.rb index 024e33f97..53b0bdb90 100644 --- a/lib/operation_response.rb +++ b/lib/operation_response.rb @@ -2,9 +2,7 @@ class OperationResponse < Hash def success? - return false unless self[:error].nil? - - true + fetch(:error, nil).nil? end def failure? diff --git a/spec/concepts/project/operation/create_remix_spec.rb b/spec/concepts/project/operation/create_remix_spec.rb index 07e74d268..17dae5db7 100644 --- a/spec/concepts/project/operation/create_remix_spec.rb +++ b/spec/concepts/project/operation/create_remix_spec.rb @@ -33,7 +33,7 @@ it 'returns success' do result = create_remix - expect(result.success?).to eq(true) + expect(result.success?).to be(true) end it 'creates new project' do @@ -104,7 +104,7 @@ it 'returns failure' do result = create_remix - expect(result.failure?).to eq(true) + expect(result.failure?).to be(true) end it 'returns error message' do @@ -122,7 +122,7 @@ it 'returns failure' do result = create_remix - expect(result.failure?).to eq(true) + expect(result.failure?).to be(true) end it 'returns error message' do @@ -143,7 +143,7 @@ end it 'returns failure' do - expect(create_remix.failure?).to eq(true) + expect(create_remix.failure?).to be(true) end it 'sets error message' do diff --git a/spec/concepts/project/operation/create_spec.rb b/spec/concepts/project/operation/create_spec.rb index afd19fa09..ba824bbb1 100644 --- a/spec/concepts/project/operation/create_spec.rb +++ b/spec/concepts/project/operation/create_spec.rb @@ -14,7 +14,7 @@ describe '.call' do it 'returns success' do - expect(create_project.success?).to eq(true) + expect(create_project.success?).to be(true) end it 'creates a new project' do @@ -57,7 +57,7 @@ end it 'returns success' do - expect(create_project_with_content.success?).to eq(true) + expect(create_project_with_content.success?).to be(true) end it 'returns project with correct component content' do @@ -74,7 +74,7 @@ end it 'returns failure' do - expect(create_project.failure?).to eq(true) + expect(create_project.failure?).to be(true) end it 'returns error message' do diff --git a/spec/concepts/project/operation/update_default_component_spec.rb b/spec/concepts/project/operation/update_default_component_spec.rb index 4c55207bc..e876abf55 100644 --- a/spec/concepts/project/operation/update_default_component_spec.rb +++ b/spec/concepts/project/operation/update_default_component_spec.rb @@ -18,7 +18,7 @@ end it 'returns failure? true' do - expect(update.failure?).to eq(true) + expect(update.failure?).to be(true) end it 'returns error message' do diff --git a/spec/concepts/project/operation/update_delete_components_spec.rb b/spec/concepts/project/operation/update_delete_components_spec.rb index ad08160f3..c9b25c82e 100644 --- a/spec/concepts/project/operation/update_delete_components_spec.rb +++ b/spec/concepts/project/operation/update_delete_components_spec.rb @@ -34,7 +34,7 @@ it 'deletes the correct component' do update - expect(Component.find_by(id: component_to_delete.id)).to eq(nil) + expect(Component.find_by(id: component_to_delete.id)).to be_nil end end end diff --git a/spec/concepts/project/operation/update_invalid_spec.rb b/spec/concepts/project/operation/update_invalid_spec.rb index 449faa2fd..6ec0968b6 100644 --- a/spec/concepts/project/operation/update_invalid_spec.rb +++ b/spec/concepts/project/operation/update_invalid_spec.rb @@ -28,7 +28,7 @@ describe '.call' do context 'when updated project component is invalid' do it 'returns failure? true' do - expect(update.failure?).to eq(true) + expect(update.failure?).to be(true) end it 'does not amend any project properties' do diff --git a/spec/concepts/project/operation/update_spec.rb b/spec/concepts/project/operation/update_spec.rb index a0ea8a655..4fcc6277f 100644 --- a/spec/concepts/project/operation/update_spec.rb +++ b/spec/concepts/project/operation/update_spec.rb @@ -30,7 +30,7 @@ let(:component_params) { [default_component_params, edited_component_params] } it 'returns success? true' do - expect(update.success?).to eq(true) + expect(update.success?).to be(true) end it 'updates project properties' do @@ -54,7 +54,7 @@ it 'creates component with correct properties' do update created_component = project.components.find_by(**new_component_params) - expect(created_component).not_to eq(nil) + expect(created_component).not_to be_nil end end end diff --git a/spec/lib/operation_response_spec.rb b/spec/lib/operation_response_spec.rb index faa9b59ca..7cf83903e 100644 --- a/spec/lib/operation_response_spec.rb +++ b/spec/lib/operation_response_spec.rb @@ -8,7 +8,7 @@ context 'when :error not present' do it 'returns true' do response = described_class.new - expect(response.success?).to eq(true) + expect(response.success?).to be(true) end end @@ -16,7 +16,7 @@ it 'returns false' do response = described_class.new response[:error] = 'An error' - expect(response.success?).to eq(false) + expect(response.success?).to be(false) end end end @@ -25,7 +25,7 @@ context 'when :error not present' do it 'returns false' do response = described_class.new - expect(response.failure?).to eq(false) + expect(response.failure?).to be(false) end end @@ -33,7 +33,7 @@ it 'returns true' do response = described_class.new response[:error] = 'An error' - expect(response.failure?).to eq(true) + expect(response.failure?).to be(true) end end end diff --git a/spec/models/component_spec.rb b/spec/models/component_spec.rb index 9fd7812d9..ebb180b12 100644 --- a/spec/models/component_spec.rb +++ b/spec/models/component_spec.rb @@ -17,7 +17,7 @@ describe 'validations' do it 'returns valid? false when name changed' do component.name = 'updated' - expect(component.valid?).to eq(false) + expect(component.valid?).to be(false) end it 'sets error message when name changed' do @@ -29,7 +29,7 @@ it 'returns valid? false when extension changed' do component.extension = 'txt' - expect(component.valid?).to eq(false) + expect(component.valid?).to be(false) end it 'sets error message when extension changed' do diff --git a/spec/request/projects/index_spec.rb b/spec/request/projects/index_spec.rb index 8bd7d526d..7c51c672d 100644 --- a/spec/request/projects/index_spec.rb +++ b/spec/request/projects/index_spec.rb @@ -30,7 +30,7 @@ it 'returns users projects' do get '/api/projects' returned = JSON.parse(response.body) - expect(returned.all? { |proj| proj['user_id'] == user_id }).to eq(true) + expect(returned.all? { |proj| proj['user_id'] == user_id }).to be(true) end end From b4a42affd47741fe01dc409e0db85b3802ac3878 Mon Sep 17 00:00:00 2001 From: "Patrick J. Cherry" Date: Mon, 31 Oct 2022 13:49:43 +0000 Subject: [PATCH 3/4] Add Sentry call test on create --- spec/concepts/project/operation/create_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/concepts/project/operation/create_spec.rb b/spec/concepts/project/operation/create_spec.rb index ba824bbb1..2151a4165 100644 --- a/spec/concepts/project/operation/create_spec.rb +++ b/spec/concepts/project/operation/create_spec.rb @@ -71,6 +71,7 @@ mock_project = instance_double(Project) allow(mock_project).to receive(:components).and_raise('Some error') allow(Project).to receive(:new).and_return(mock_project) + allow(Sentry).to receive(:capture_exception) end it 'returns failure' do @@ -80,6 +81,11 @@ it 'returns error message' do expect(create_project[:error]).to eq('Error creating project') end + + it 'sents the exception to Sentry' do + create_project + expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) + end end end end From ca2fa1d835a81f800e0b5833b6d84399d7a5ee95 Mon Sep 17 00:00:00 2001 From: Patrick Cherry Date: Mon, 31 Oct 2022 15:27:52 +0000 Subject: [PATCH 4/4] Update create_spec.rb --- spec/concepts/project/operation/create_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/concepts/project/operation/create_spec.rb b/spec/concepts/project/operation/create_spec.rb index 2151a4165..72e10a211 100644 --- a/spec/concepts/project/operation/create_spec.rb +++ b/spec/concepts/project/operation/create_spec.rb @@ -82,7 +82,7 @@ expect(create_project[:error]).to eq('Error creating project') end - it 'sents the exception to Sentry' do + it 'sent the exception to Sentry' do create_project expect(Sentry).to have_received(:capture_exception).with(kind_of(StandardError)) end