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.
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
Make #admin_domain private and fix DomainSubstitution deprecation warnings #3001
Make #admin_domain private and fix DomainSubstitution deprecation warnings #3001
Conversation
f217251
to
2871f97
Compare
cdd4b66
to
bbd1a6c
Compare
test/unit/account_test.rb
Outdated
@@ -361,6 +361,7 @@ def setup | |||
|
|||
assert_equal "amazing-name-123.#{ThreeScale.config.superdomain}", account.domain | |||
assert_equal "amazing-name-123-admin.#{ThreeScale.config.superdomain}", account.internal_admin_domain | |||
assert_equal "amazing-name-123-admin.#{ThreeScale.config.superdomain}", account.external_admin_domain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lines |
e94c2e0
to
b534f4c
Compare
- #external_self_domain - #internal_self_domain - #self_domain - #admin_domain - #domain in favor of: - internal_admin_domain - external_admin_domain - internal_domain - external_domain
b534f4c
to
47c5d55
Compare
@@ -1,4 +1,4 @@ | |||
<% unless request.host == site_account.admin_domain %> | |||
<% unless request.host == site_account.external_admin_domain%> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt in this case, as I am not sure if request.host
is supposed to be before or after substitution...
But on the other hand, I couldn't even find where this partial is used, I guess it might not be used anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
If the domain substitution feature is ever going to be used in non-dev environment, it might be possible we will find some edge case that was not treated properly. But as there are no plans at the moment, I think it's good enough!
…nings (#3001) [domain_substitution] makes #admin_domain private and stop using self_domain/domain in favor of internal/external versions (THREESCALE-8523)
…nings (#3001) [domain_substitution] makes #admin_domain private and stop using self_domain/domain in favor of internal/external versions (THREESCALE-8523)
…nings (#3001) [domain_substitution] makes #admin_domain private and stop using self_domain/domain in favor of internal/external versions (THREESCALE-8523)
TLDR (too long don't review)
admin_domain -> internal_admin_domain / external_admin_domain
self_domain -> internal_admin_domain / external_admin_domain
match_internal_self_domain? -> match_internal_admin_domain?
domain -> internal_domain / external_domain
"internal" domain reads directly from DB (internal use) whereas "external" domain is meant to be exposed in UI and API responses (external use)