Skip to content
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

Jump to bottom

[Threescale 7522] Fix CDN urls for fonts assets #3072

Closed
wants to merge 17 commits into from
Closed

[Threescale 7522] Fix CDN urls for fonts assets #3072

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2c3e4a8
THREESCALE-7522: Update webpacker to 5.4.3
jlledom Oct 6, 2022
e0010c3
THREESCALE-7522: Update file-loader to 6.2.0
jlledom Oct 6, 2022
295a331
THREESCALE-7522: Decouple CDN url from assets compilation
jlledom Oct 6, 2022
6e9c99d
Folllorw Red Hat style guidelines on environment.js
jlledom Oct 11, 2022
f29bb2d
THREESCALE-7522: Set the CDN URL in all layouts
jlledom Oct 13, 2022
468c0db
THREESCALE-7522: Remove TODOs
jlledom Oct 13, 2022
44c1e54
THREESCALE-7522: Add unit tests for rails_asset_host_url
jlledom Oct 18, 2022
19f3fc0
THREESCALE-7522: Add cucumbers to ensure assets are loaded from the CDN
jlledom Oct 20, 2022
401fb66
THREESCALE-7522: Remove exception for SaaS
jlledom Oct 20, 2022
3f9cdf0
THREESCALE-7522: Prevent compiling assets w/ asset host
jlledom Oct 21, 2022
9d05346
Revert "THREESCALE-7522: Update webpacker to 5.4.3"
jlledom Oct 24, 2022
727a33c
Asset host config: use a self-explanatory variable name
jlledom Dec 19, 2022
f67119c
CDN Url: Webpacker config: Make code clearer
jlledom Dec 19, 2022
13398a2
CDN Cucumbers: Use only one javascript tag
jlledom Dec 19, 2022
a9a771f
Refactor CDN asset host tests
jlledom Dec 19, 2022
90fb76b
Asset host tests: mock rails config
jlledom Dec 19, 2022
8a79e58
Rake task: check_asset_host: use present?
jlledom Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions app/helpers/application_helper.rb
View file
Open in desktop
Expand Up @@ -364,4 +364,17 @@ def has_out_of_date_configuration?(service)
return unless service
service.pending_affecting_changes?
end

def rails_asset_url_tag
javascript_tag("window.rails_asset_host = '#{rails_asset_host_url}'", { type: "text/javascript"})
end

def rails_asset_host_url
asset_host_enabled = Rails.configuration.asset_host.present?
jlledom marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@mayorova mayorova Oct 21, 2022 •
edited

Choose a reason for hiding this comment

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

Not sure I get it - there are two asset_host values in different places in config tree? 🤔

UPDATE: Didn't scroll enough 😅

If I understand correctly Rails.configuration.asset_host is coming from config/environments/{env}.rb, and Rails.configuration.three_scale.asset_host comes from settings.yml.

From the change and the comment in webpack config, it seems to me that we always assume that this asset_host should be empty on compilation time?
If so, shouldn't we force it to be empty, so we don't have double-host issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly Rails.configuration.asset_host is coming from config/environments/{env}.rb, and Rails.configuration.three_scale.asset_host comes from settings.yml.

Yes.

From the change and the comment in webpack config, it seems to me that we always assume that this asset_host should be empty on compilation time?

Yes.

If so, shouldn't we force it to be empty, so we don't have double-host issues?

Do you mean force it empty during compilation time? It's the same @akostadinov said. Yes, I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a callback that returns an error during assets compilation if the asset host is set. I couldn't find an easy way to just unset the asset host transparently. That would need more time but I can try if you guys think it's needed.

asset_host_url = Rails.configuration.three_scale.asset_host.presence
Copy link
Contributor

Choose a reason for hiding this comment

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

It is still not clear for me why we are using both settings...
Rails.configuration.asset_host and Rails.configuration.three_scale.asset_host
Shouldn't we use just one?...

I guess Rails.configuration.asset_host is the one that should define the actual value, as it's env-specific, and will use the value in Rails.configuration.three_scale.asset_host, if present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The relevant piece of code is here: /config/environments/production.rb#L109-L121

We could just set Rails.configuration.asset_host to Rails.configuration.three_scale.asset_host and it would work. The first was converted into a lambda function here. I can't get too much info from the commit message: what other assets besides ours are we loading through javascript_include_tag? Maybe we can remove the lambda function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I am not sure about the reason for having this function there.
What I mean is: do you think we can we just rely on Rails.configuration.asset_host in this helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need both variables, asset_host_enabled tells us when the CDN is enabled, it can be disabled when DISABLE_DIGEST == 1. In that case we don't care about the value of RAILS_ASSETS: all assets loaded by rails won't include the CDN url, so the ones loaded by webpack shouldn't neither.

It's also possible that asset_host_enabled is true, but there's not provided asset url (asset_host_url = RAILS_ASSET_HOST = <empty>), then the situation is the same, rails won't load any asset and webpack shouldn't neither. So both variables must be true to go ahead and return the CDN url for webpack.

If we rely only on the value of asset_host_enabled, then we need to check for the value of asset_host_url anyway, to decide if we prepend the https:// prefix or not. Now it's done in a way all possibilities are checked in one line:

asset_host_enabled asset_host_url return value
false '' ''
false 'whtaever.cloudfront.net' ''
true '' ''
true 'whtaever.cloudfront.net' 'https://whtaever.cloudfront.net'

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, does it make sense to rely only on asset_host_url, if it is set, then prepend. WDYT? Or is asset_host_enabled a rails thing that is tricky to ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we rely only in asset_host_url, the second row in the table would fail, it would return 'https://whtaever.cloudfront.net/' instead of ''. Since asset_host_enabled == false, it will prepend the URL to webpack assets like fonts and JS files, but it won't use the CDN for all other assets. I think that's not the expected behaviour, if the admin disables the CDN, then it must be disabled for all assets.

Copy link
Contributor

Choose a reason for hiding this comment

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

If admin wants to disable CDN, admin could empty asset_host_url. Or there is some other issue related to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can be disabled in two ways: by DISABLE_DIGEST == 1 or by RAILS_ASSET_HOST == ''. If the admin disables the CDN by leaving RAILS_ASSET_HOST empty, that will work, but if they want to enable it solely by RAILS_ASSET_HOST it might not work because DISABLE_DIGEST could be 1, and then it would only enable the CDN for webpack but not for Sprockets. We need to enforce that no matter how the assets are disabled or enabled, they must be disabled or enabled for both webpack and sprockets.


return '' unless asset_host_enabled && asset_host_url

"#{request.protocol}#{asset_host_url}"
Comment on lines +376 to +378
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's no need to check the string twice. Would this be equivalent?

Suggested change
return '' unless asset_host_enabled && asset_host_url
"#{request.protocol}#{asset_host_url}"
return '' unless asset_host_url = Rails.configuration.three_scale.asset_host.presence
"#{request.protocol}#{asset_host_url}"

Copy link
Contributor Author

@jlledom jlledom Dec 19, 2022 •
edited

Choose a reason for hiding this comment

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

It's not checking the string twice because only asset_host_url is a string, asset_host_enabled is a boolean. This comment explains why both are required.

end
end
1 change: 1 addition & 0 deletions app/views/layouts/error.html.erb
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@
<%- admin_domain = Account.is_admin_domain?(request.internal_host) -%>
<%- master_domain = Account.is_master_domain?(request.internal_host) -%>
<%- site_account ||= Account.master -%>
<%= rails_asset_url_tag %>
<%= stylesheet_link_tag 'error' -%>
<%= render 'provider/analytics' if admin_domain %>
</head>
Expand Down
1 change: 1 addition & 0 deletions app/views/layouts/provider.html.slim
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ html[lang="en" class="pf-m-redhat-font"]
= content_for?(:title) ? yield(:title) : default_title
| | Red Hat 3scale API Management
= csrf_meta_tag
= rails_asset_url_tag
= javascript_pack_tag 'PF4Styles/base'
= render 'provider/theme'
= render 'provider/analytics'
Expand Down
1 change: 1 addition & 0 deletions app/views/layouts/provider/iframe.html.slim
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ html[lang="en"]
base href=base_url
= stylesheet_link_tag "provider/layouts/iframe"
= csrf_meta_tag
= rails_asset_url_tag
= yield :head

body
Expand Down
2 changes: 1 addition & 1 deletion app/views/layouts/provider/login.html.slim
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@ html[lang="en" class="pf-m-redhat-font"]
meta http-equiv="Content-Type" content="text/html; charset=utf-8"
title 3scale Login
= csrf_meta_tag

= rails_asset_url_tag
= javascript_pack_tag 'PF4Styles/base'
= javascript_pack_tag 'PF4Styles/loginPage'
= javascript_include_tag 'vendor/jquery-1.8.2.min.js', 'vendor/rails.js'
Expand Down
1 change: 1 addition & 0 deletions app/views/layouts/provider/suspended.html.slim
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ html[lang="en"]
title
| Account Suspended | Red Hat 3scale API Management
= csrf_meta_tag
= rails_asset_url_tag
= stylesheet_link_tag "provider/themes/wizard.css"
= render 'provider/analytics'
= javascript_include_tag 'provider/layout/provider'
Expand Down
1 change: 1 addition & 0 deletions app/views/layouts/wizard.html.slim
View file
Open in desktop
Expand Up @@ -6,6 +6,7 @@ html[lang="en" class="pf-m-redhat-font"]
= content_for?(:title) ? yield(:title) : default_title
| | Red Hat 3scale API Management
= csrf_meta_tag
= rails_asset_url_tag
= javascript_pack_tag 'PF4Styles/base'
= stylesheet_link_tag "provider/themes/wizard.css"
= render 'provider/analytics'
Expand Down
10 changes: 10 additions & 0 deletions config/environments/test.rb
View file
Open in desktop
Expand Up @@ -64,6 +64,16 @@

config.assets.compile = ENV.fetch('SKIP_ASSETS', '0') == '0'

config.asset_host = ->(source) do
# does it exist in /public/assets ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what this comment is for. Is it a question or a clarification? If the latter, I think the code is clear enough. However, I would use a self-explanatory name such as exist_in_public_assets before resorting to comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll fix this

full_path = File.join(Rails.public_path, source)
precompiled = File.exist?(full_path)

break unless precompiled

config.three_scale.asset_host.presence
end

config.after_initialize do
::GATEWAY = ActiveMerchant::Billing::BogusGateway.new
end
Expand Down
20 changes: 20 additions & 0 deletions config/webpack/environment.js
View file
Open in desktop
Expand Up @@ -37,4 +37,24 @@ environment.loaders.append('yaml', {
type: 'json'
})

/* The CDN url must be hardcoded at settings.yml:asset_host during assets compilation in order to get static assets
* like CSS files point to the CDN. Otherwise, the assets are generated with relative paths and are not loaded from
* the CDN, even when settings.yml:asset_host is set during runtime. We don't want to have to provide any CDN url when
* building porta container images in order to get the assets correctly precompiled. To avoid that, this trick assumes
* the assets are generated with relative paths (settings.yml:asset_host = null during compilation time). The next code
* is executed by webpack when compiling the assets, and sets the variable `postTransformPublicPath` to an arrow
* function which will prepend the CDN url in runtime.
*
* https://github.com/3scale/porta/pull/3072
*/
const { output } = environment.config;
const { publicPath } = output;
const fileLoader = environment.loaders.get('file');

output.publicPath = '';
Object.assign(fileLoader.use[0].options, {
publicPath,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing. Is this supposed to be the previous or the current value of publicPath? If it's the same value please use the same reference for both. Also for readability's sake I'd suggest not mixing up config and loaders: first update publicPath in one block then get the loader and mutate it in second block. These config files tend to get messy over time if we're not super tidy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done (or at least I tried 😛)

postTransformPublicPath: (p) => `window.rails_asset_host + ${p}`
});

module.exports = environment
32 changes: 32 additions & 0 deletions features/asset_host/asset_host.feature
View file
Open in desktop
@@ -0,0 +1,32 @@
Feature: Asset host
Copy link
Contributor

Choose a reason for hiding this comment

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

Tag can be added at Feature level once.

Suggested change
Feature: Asset host
@javascript
Feature: Asset host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

In order to reduce the network traffic from the web server
As master, provider or developer
I want to load all assets from the configured CDN

Background:
Given a provider "foo.3scale.localhost"
And a buyer "bob" signed up to provider "foo.3scale.localhost"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid naming things unless necessary, to reduce verbosity. Use these instead:

Suggested change
Given a provider "foo.3scale.localhost"
And a buyer "bob" signed up to provider "foo.3scale.localhost"
Given a provider
And the provider has one buyer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK


@javascript
Scenario: Asset host not configured
When I am logged in as master admin on master domain
Copy link
Contributor

Choose a reason for hiding this comment

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

Group Master and Provider scenarios into different Rules to reduce duplication, as each Rule can have its own Background.

On the other hand and related to the comment above, we should avoid "I" steps. This one is also available:

Given master admin is logged in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Then assets shouldn't be loaded from the asset host

@javascript
Scenario: Master dashboard with asset host configured
When the asset host is set to "cdn.3scale.localhost"
And I am logged in as master admin on master domain
Then assets should be loaded from the asset host

@javascript
Scenario: Provider dashboard with asset host configured
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of things here. The correct order should be: given-when-then.

Given the asset host is set to "cdn.3scale.localhost"
And current domain is the admin domain of provider "foo.3scale.localhost"
When I am logged in as provider "foo.3scale.localhost"
Then assets should be loaded from the asset host

Also, we don't use "I" steps anymore (or we should avoid them completely). Some steps have already been refactored accordingly. I think this one is:

-And I am logged in as provider "foo.3scale.localhost"
+And the provider is logged in

I think the step current domain is the admin domain of provider "foo.3scale.localhost" is already included in Given a provider and it doesn't add value to the scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's better now

When the asset host is set to "cdn.3scale.localhost"
Given current domain is the admin domain of provider "foo.3scale.localhost"
And I am logged in as provider "foo.3scale.localhost"
Then assets should be loaded from the asset host

@javascript
Scenario: Developer portal with asset host configured
When the asset host is set to "cdn.3scale.localhost"
Given I log in as "bob" on foo.3scale.localhost
Then javascript assets should be loaded from the asset host
25 changes: 25 additions & 0 deletions features/step_definitions/asset_host_steps.rb
View file
Open in desktop
@@ -0,0 +1,25 @@
# frozen_string_literal: true

Given /^the asset host is set to "(.*)"$/ do |asset_host|
cdn_url = "#{asset_host}:#{Capybara.current_session.server.port}"
Rails.configuration.asset_host = cdn_url
Rails.configuration.three_scale.asset_host = cdn_url
end

Then /^((javascript|font)\s)?assets should be loaded from the asset host$/ do |asset_type|
cdn_url = Rails.configuration.asset_host
js_regexp = Regexp.new("https?://#{cdn_url}/packs.*?\\.js")
font_regexp = Regexp.new("https?://#{cdn_url}/packs.*?\\.eot")

assert_not_nil Capybara.page.source.match js_regexp if ['javascript', nil].include? asset_type
assert_not_nil Capybara.page.source.match font_regexp if ['font', nil].include? asset_type
Comment on lines +14 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get what's being asserted here but it looks to me like convoluted simulated polymorphism. Does it matter whether asset_type is defined?

Or maybe what you meant is:

Then "javascript/fonts assets should be loaded from the asset host" do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If asset_type exists, I only want to assert that type of assets, if not, I want to assert both.

end

Then /^assets shouldn't be loaded from the asset host$/ do
# When no CDN is set, we expect to find relative paths for fonts and js
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using a comment when you can use a step?

When the asset host is not set # no CDN is set
Then assets should be loaded # assert js/font using relative paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That comment is only to explain the regular expressions below it: we particularly look for relative paths. It can't be replaced by any step.

js_regexp = Regexp.new('src="/packs.*?\.js"')
font_regexp = Regexp.new('url\(/packs.*?\.eot\)')

assert_not_nil Capybara.page.source.match js_regexp
assert_not_nil Capybara.page.source.match font_regexp
end
13 changes: 13 additions & 0 deletions lib/tasks/webpacker.rake
View file
Open in desktop
@@ -0,0 +1,13 @@
# frozen_string_literal: true

namespace :webpacker do

task :check_asset_host do
if Rails.configuration.three_scale.asset_host.presence
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if Rails.configuration.three_scale.asset_host.presence
if Rails.configuration.three_scale.asset_host.present?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the same, presence returns nil or the original value, present? return a boolean.

https://stackoverflow.com/questions/19637499/what-is-the-point-of-objectpresence-in-rails

Copy link
Contributor

Choose a reason for hiding this comment

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

Still looks better wit h the ? IMO :)

Copy link
Contributor Author

@jlledom jlledom Dec 19, 2022 •
edited

Choose a reason for hiding this comment

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

Well, now I think about it, maybe it should return a boolean...

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly noticed that but was too lazy to comment on it :) Thought it was too small of a detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied

STDERR.puts "*** Asset host must be null during compilation time. See https://github.com/3scale/porta/pull/3072 ***"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the message should be more practical/meaningful. What about we say:

*** Asset host is not supported. Run <command> and try again. ***

With some instruction to remove it? My reasoning is not everyone will be familiar with rake, webpack or with the assets pipeline or they will simply don't what to know about it and will just want to run the app.
And pointing to the PR in a comment, aimed to developers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's not true that Asset host is not supported, it only needs to be empty during the compilation time, but it can have a value in runtime.

return false
end
end

Rake::Task['webpacker:compile'].enhance [:check_asset_host]
end
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -44,7 +44,7 @@
"eslint-plugin-promise": "^4.1.1",
"eslint-plugin-react": "^7.13.0",
"eslint-plugin-standard": "^4.0.0",
"file-loader": "^1.1.6",
"file-loader": "^6.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

That is a bold move 😄 No breaking changes or caveats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't you trust me? 😢

Copy link
Contributor Author

@jlledom jlledom Dec 19, 2022 •
edited

Choose a reason for hiding this comment

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

I didn't try this thoroughly, I only used it in my development environment and my dev cluster and noticed no incidences, also all tests pass. So how could I be sure I'm not breaking anything?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably read through the changelog for any breaking changes I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the list of breaking changes:

  • use md4 by default for hashing (chore(deps): update webpack-contrib/file-loader#369) (ad39022)
  • minimum required nodejs version is 10.13.0
  • rename the esModules option to esModule
  • switch to ES modules by default (the option esModule is true by default)
  • removed the useRelativePath option. It is dangerously and break url when you use multiple entry points.
  • drop support for webpack < 4

Only the nodejs version seems relevant for me. Which one is installed in SaaS?

"flow-bin": "^0.152.0",
"flow-typed": "^3.3.1",
"hosted-git-info": "^2.7.1",
Expand Down
36 changes: 36 additions & 0 deletions test/unit/helpers/application_helper_test.rb
View file
Open in desktop
Expand Up @@ -22,6 +22,42 @@ def test_css_class
assert_equal 'one two three', css_class('one', ['two'], three: true)
end

class AssetHostTest < ApplicationHelperTest
setup do
@request = ActionDispatch::TestRequest.create
@asset_host = 'cdn.3scale.test.localhost'
end

attr_reader :request
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
attr_reader :request
attr_reader :request, :asset_host

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?


test 'asset host is not configured' do
Rails.configuration.asset_host = nil
Rails.configuration.three_scale.asset_host = @asset_host
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Rails.configuration.asset_host = nil
Rails.configuration.three_scale.asset_host = @asset_host
Rails.configuration.expects(:asset_host).returns(nil)
Rails.configuration.three_scale.expects(:asset_host).returns(@asset_host)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this, also for other tests


result = rails_asset_host_url

assert_equal '', result
end

test "asset host is configured but it's value is empty" do
Rails.configuration.asset_host = -> {}
Rails.configuration.three_scale.asset_host = ''

result = rails_asset_host_url

assert_equal '', result
end

test 'asset host is configured and has a proper value' do
Rails.configuration.asset_host = -> {}
Rails.configuration.three_scale.asset_host = @asset_host

result = rails_asset_host_url

assert_equal "#{request.protocol}#{@asset_host}", result
end
end

private

def ability
Expand Down
42 changes: 24 additions & 18 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -1732,6 +1732,11 @@
resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-7.0.8.tgz#edf1bf1dbf4e04413ca8e5b17b3b7d7d54b59818"
integrity sha512-YSBPTLTVm2e2OoQIDYx8HaeWJ5tTToLH67kXR7zYNGupXMEHa2++G8k+DczX2cFVgalypqtyZIcU19AFcmOpmg==

"@types/json-schema@^7.0.8":
version "7.0.11"
resolved "https://registry.yarnpkg.com/@types/json-schema/-/json-schema-7.0.11.tgz#d421b6c527a3037f7c84433fd2c4229e016863d3"
integrity sha512-wOuvG1SN4Us4rez+tylwwwCV1psiNVOkJeM3AUWUNWg/jDQY2+HE/444y5gc+jBmRqASOm2Oeh5c1axHobwRKQ==

"@types/json5@^0.0.29":
version "0.0.29"
resolved "https://registry.yarnpkg.com/@types/json5/-/json5-0.0.29.tgz#ee28707ae94e11d2b827bcbe5270bcea7f3e71ee"
Expand Down Expand Up @@ -2039,7 +2044,7 @@ ajv-keywords@^3.1.0, ajv-keywords@^3.4.1, ajv-keywords@^3.5.2:
resolved "https://registry.yarnpkg.com/ajv-keywords/-/ajv-keywords-3.5.2.tgz#31f29da5ab6e00d1c2d329acf7b5929614d5014d"
integrity sha512-5p6WTN0DdTGVQk6VjcEju19IgaHudalcfabD7yhDGeA6bcQnmL+CpveLJq/3hvfwd1aof6L386Ougkx6RfyMIQ==

ajv@^6.1.0, ajv@^6.10.0, ajv@^6.10.2, ajv@^6.12.3, ajv@^6.12.4, ajv@^6.7.0:
ajv@^6.1.0, ajv@^6.10.0, ajv@^6.10.2, ajv@^6.12.3, ajv@^6.12.4, ajv@^6.12.5, ajv@^6.7.0:
version "6.12.6"
resolved "https://registry.yarnpkg.com/ajv/-/ajv-6.12.6.tgz#baf5a62e802b07d977034586f8c3baf5adf26df4"
integrity sha512-j3fVLgvTo527anyYyJOGTYJbG+vnnQYvE0m5mmkc1TK+nxAppkCLMIL0aZ4dblVCNoGShhm+kzE4ZUykBoMg4g==
Expand Down Expand Up @@ -5424,14 +5429,6 @@ file-entry-cache@^5.0.1:
dependencies:
flat-cache "^2.0.1"

file-loader@^1.1.6:
version "1.1.11"
resolved "https://registry.yarnpkg.com/file-loader/-/file-loader-1.1.11.tgz#6fe886449b0f2a936e43cabaac0cdbfb369506f8"
integrity sha512-TGR4HU7HUsGg6GCOPJnFk06RhWgEWFLAGWiT6rcD+GRC2keU3s9RGJ+b3Z6/U73jwwNb2gKLJ7YCrp+jvU4ALg==
dependencies:
loader-utils "^1.0.2"
schema-utils "^0.4.5"

file-loader@^4.2.0:
version "4.3.0"
resolved "https://registry.yarnpkg.com/file-loader/-/file-loader-4.3.0.tgz#780f040f729b3d18019f20605f723e844b8a58af"
Expand All @@ -5440,6 +5437,14 @@ file-loader@^4.2.0:
loader-utils "^1.2.3"
schema-utils "^2.5.0"

file-loader@^6.2.0:
version "6.2.0"
resolved "https://registry.yarnpkg.com/file-loader/-/file-loader-6.2.0.tgz#baef7cf8e1840df325e4390b4484879480eebe4d"
integrity sha512-qo3glqyTa61Ytg4u73GultjHGjdRyig3tG6lPtyX/jOEJvHif9uB0/OCI2Kif6ctF3caQTW2G5gym21oAsI4pw==
dependencies:
loader-utils "^2.0.0"
schema-utils "^3.0.0"

file-selector@^0.1.8:
version "0.1.19"
resolved "https://registry.yarnpkg.com/file-selector/-/file-selector-0.1.19.tgz#8ecc9d069a6f544f2e4a096b64a8052e70ec8abf"
Expand Down Expand Up @@ -7847,7 +7852,7 @@ loader-runner@^2.4.0:
resolved "https://registry.yarnpkg.com/loader-runner/-/loader-runner-2.4.0.tgz#ed47066bfe534d7e84c4c7b9998c2a75607d9357"
integrity sha512-Jsmr89RcXGIwivFY21FcRrisYZfvLMTWx5kOLc+JTxtpBOG6xML0vzbc6SEQG2FO9/4Fc3wW4LVcB5DmGflaRw==

loader-utils@^1.0.1, loader-utils@^1.0.2, loader-utils@^1.1.0, loader-utils@^1.2.3, loader-utils@^1.4.0:
loader-utils@^1.0.1, loader-utils@^1.1.0, loader-utils@^1.2.3, loader-utils@^1.4.0:
version "1.4.0"
resolved "https://registry.yarnpkg.com/loader-utils/-/loader-utils-1.4.0.tgz#c579b5e34cb34b1a74edc6c1fb36bfa371d5a613"
integrity sha512-qH0WSMBtn/oHuwjy/NucEgbx5dbxxnxup9s4PVXJUDHZBQY+s0NWA9rJf53RBnQZxfch7euUui7hpoAPvALZdA==
Expand Down Expand Up @@ -11468,14 +11473,6 @@ scheduler@^0.19.1:
loose-envify "^1.1.0"
object-assign "^4.1.1"

schema-utils@^0.4.5:
version "0.4.7"
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-0.4.7.tgz#ba74f597d2be2ea880131746ee17d0a093c68187"
integrity sha512-v/iwU6wvwGK8HbU9yi3/nhGzP0yGSuhQMzL6ySiec1FSrZZDkhm4noOSWzrNFo/jEc+SJY6jRTwuwbSXJPDUnQ==
dependencies:
ajv "^6.1.0"
ajv-keywords "^3.1.0"

schema-utils@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-1.0.0.tgz#0b79a93204d7b600d4b2850d1f66c2a34951c770"
Expand All @@ -11494,6 +11491,15 @@ schema-utils@^2.5.0, schema-utils@^2.6.5, schema-utils@^2.6.6, schema-utils@^2.7
ajv "^6.12.4"
ajv-keywords "^3.5.2"

schema-utils@^3.0.0:
version "3.1.1"
resolved "https://registry.yarnpkg.com/schema-utils/-/schema-utils-3.1.1.tgz#bc74c4b6b6995c1d88f76a8b77bea7219e0c8281"
integrity sha512-Y5PQxS4ITlC+EahLuXaY86TXfR7Dc5lw294alXOq86JAHCihAIZfqv8nNCWvaEJvaC51uN9hbLGeV0cFBdH+Fw==
dependencies:
"@types/json-schema" "^7.0.8"
ajv "^6.12.5"
ajv-keywords "^3.5.2"

scss-tokenizer@^0.2.3:
version "0.2.3"
resolved "https://registry.yarnpkg.com/scss-tokenizer/-/scss-tokenizer-0.2.3.tgz#8eb06db9a9723333824d3f5530641149847ce5d1"
Expand Down