Skip to content

Bypass asset_host configuration for icon sprite #5895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 2 commits into from
Feb 2, 2022
Merged

Bypass asset_host configuration for icon sprite #5895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f82e3b
Bypass asset_host configuration for icon sprite
aduth Feb 2, 2022
d881301
[skip changelog]
aduth Feb 2, 2022
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
12 changes: 11 additions & 1 deletion app/components/icon_component.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,16 @@ def initialize(icon:, **tag_options)
def icon_path
# Revert back to using design system image once upstream CSP fix has been patched.
# See: https://github.com/uswds/uswds/pull/4487
[image_path('sprite.svg'), '#', icon].join
[image_path('sprite.svg', host: asset_host), '#', icon].join
end

private

def asset_host
if Rails.env.production?
IdentityConfig.store.domain_name
Comment on lines 269 to 270
Copy link
Copy Markdown
Contributor Author

@aduth aduth Feb 2, 2022

Choose a reason for hiding this comment

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

This comes from the fallback here:

identity-idp/config/environments/production.rb

Line 11 in efa5c49

(IdentityConfig.store.asset_host.presence || IdentityConfig.store.domain_name) if request

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis Feb 2, 2022

Choose a reason for hiding this comment

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

this branch is not covered by tests, I'd consider adding it

if we don't want to stub Rails.env, we could do some dependency injection:

def asset_host(rails_env = Rails.env)
  if rails_env.production?

and then in the test pass in a ActiveSupport::EnvironmentInquirer.new("production") or ActiveSupport::EnvironmentInquirer.new("development") etc

Copy link
Copy Markdown
Contributor Author

@aduth aduth Feb 2, 2022

Choose a reason for hiding this comment

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

this branch is not covered by tests, I'd consider adding it

👍 Added specs in rebased 8f82e3b.

Went for stubbing due to light preference to keep asset_host a private method.

elsif request
request.base_url
Comment on lines 271 to 272
Copy link
Copy Markdown
Contributor Author

@aduth aduth Feb 2, 2022

Choose a reason for hiding this comment

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

...and this matches the default behavior of Rails asset URL calculation:

https://github.com/rails/rails/blob/d35bcc93f791b9be7a41905c6342f9044b24f298/actionview/lib/action_view/helpers/asset_url_helper.rb#L292

end
end
end
24 changes: 23 additions & 1 deletion spec/components/icon_component_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
require 'rails_helper'

RSpec.describe IconComponent, type: :component do
let(:asset_host) { 'http://wrong.example.com' }
let(:domain_name) { 'http://correct.example.com' }

before do
allow(IdentityConfig.store).to receive(:asset_host).and_return(asset_host)
allow(IdentityConfig.store).to receive(:domain_name).and_return(domain_name)
end

it 'renders icon svg' do
rendered = render_inline IconComponent.new(icon: :print)

expect(rendered).to have_css('.usa-icon use[href$=".svg#print"]')
expect(rendered).to have_css(".usa-icon use[href^='#{request.base_url}'][href$='.svg#print']")
end

context 'with invalid icon' do
Expand All @@ -28,4 +36,18 @@
expect(rendered).to have_css('.usa-icon[data-foo="bar"]')
end
end

context 'in production' do
before do
allow(Rails.env).to receive(:production?).and_return(true)
end

it 'bypasses configured asset_host and uses domain_name instead' do
rendered = render_inline IconComponent.new(icon: :print)

href = rendered.css('use').first['href']

expect(href).to start_with(domain_name)
end
end
end