Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions app/forms/frontend_error_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
class FrontendErrorForm
include ActiveModel::Model
include ActionView::Helpers::TranslationHelper

validate :validate_filename_extension
validate :validate_filename_host

attr_reader :filename

def submit(filename:)
@filename = filename

FormResponse.new(success: valid?, errors:, serialize_error_details_only: true)
end

private

def validate_filename_extension
return if File.extname(filename.to_s) == '.js'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we 100% always get the filename? my guess is we'd want to allow a nil filename too just in case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec implies that it must be a string, but that it could (at least by the defaults defined there) be empty. The behavior here matches the behavior on the client-side, which would ignore anything not explicitly .js. Whether we should want to log those, I'd have a mild curiosity what (if anything) would be logged, but generally think we're interested in errors known to be originating from our own scripts.

errors.add(:filename, :invalid_extension, message: t('errors.general'))
end

def validate_filename_host
begin
return if URI(filename.to_s).host == IdentityConfig.store.domain_name
rescue URI::InvalidURIError; end

errors.add(:filename, :invalid_host, message: t('errors.general'))
end
end
2 changes: 2 additions & 0 deletions app/services/frontend_error_logger.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ class FrontendErrorLogger
class FrontendError < StandardError; end

def self.track_error(name:, message:, stack:, filename:)
return unless FrontendErrorForm.new.submit(filename:).success?

NewRelic::Agent.notice_error(
FrontendError.new,
expected: true,
Expand Down
2 changes: 2 additions & 0 deletions spec/controllers/frontend_log_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,8 @@
end

it 'notices the error to NewRelic instead of analytics logger' do
allow_any_instance_of(FrontendErrorForm).to receive(:submit).
and_return(FormResponse.new(success: true))
expect(fake_analytics).not_to receive(:track_event)
expect(NewRelic::Agent).to receive(:notice_error).with(
FrontendErrorLogger::FrontendError.new,
Expand Down
69 changes: 69 additions & 0 deletions spec/forms/frontend_error_form_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
require 'rails_helper'

RSpec.describe FrontendErrorForm do
let(:filename) { 'https://example.com/foo.js' }

subject(:form) { described_class.new }

before do
allow(IdentityConfig.store).to receive(:domain_name).and_return('example.com')
end

describe '#submit' do
subject(:result) { form.submit(filename:) }

context 'with valid filename' do
let(:filename) { 'https://example.com/foo.js' }

it 'is successful' do
expect(result.success?).to eq(true)
expect(result.errors).to eq({})
end
end

context 'without filename' do
let(:filename) { nil }

it 'is unsuccessful' do
expect(result.success?).to eq(false)
expect(result.errors).to eq(filename: [t('errors.general'), t('errors.general')])
end
end

context 'with filename without extension' do
let(:filename) { 'https://example.com/foo' }

it 'is unsuccessful' do
expect(result.success?).to eq(false)
expect(result.errors).to eq(filename: [t('errors.general')])
end
end

context 'with filename having extension other than js' do
let(:filename) { 'https://example.com/foo.txt' }

it 'is unsuccessful' do
expect(result.success?).to eq(false)
expect(result.errors).to eq(filename: [t('errors.general')])
end
end

context 'with filename from a different host' do
let(:filename) { 'https://wrong.example.com/foo.js' }

it 'is unsuccessful' do
expect(result.success?).to eq(false)
expect(result.errors).to eq(filename: [t('errors.general')])
end
end

context 'with filename that cannot be parsed as url' do
let(:filename) { '{' }

it 'is unsuccessful' do
expect(result.success?).to eq(false)
expect(result.errors).to eq(filename: [t('errors.general'), t('errors.general')])
end
end
end
end
26 changes: 24 additions & 2 deletions spec/services/frontend_error_logger_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,13 @@
require 'rails_helper'

RSpec.describe FrontendErrorLogger do
let(:valid) { true }

before do
allow_any_instance_of(FrontendErrorForm).to receive(:submit).
and_return(FormResponse.new(success: valid))
end

describe '.track_event' do
it 'notices an expected error to NewRelic with custom parameters' do
expect(NewRelic::Agent).to receive(:notice_error).with(
Expand All @@ -11,7 +18,7 @@
name: 'name',
message: 'message',
stack: 'stack',
filename: 'filename',
filename: 'filename.js',
},
},
)
Expand All @@ -20,8 +27,23 @@
name: 'name',
message: 'message',
stack: 'stack',
filename: 'filename',
filename: 'filename.js',
)
end

context 'with unsuccessful validation of request parameters' do
let(:valid) { false }

it 'does not notice an error' do
expect(NewRelic::Agent).not_to receive(:notice_error)

FrontendErrorLogger.track_error(
name: 'name',
message: 'message',
stack: 'stack',
filename: 'filename.js',
)
end
end
end
end