-
Notifications
You must be signed in to change notification settings - Fork 987
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
Fixes #37623 - Use html format in react controller to allow for ics domains #10235
Conversation
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.
It took me a while to figure out why it's changing the format because first I thought you meant the Foreman server hostname, but the URL is then https://foreman.example.com/new/hosts/host.example.ics
.
Updated to what @swimlappy used. Screenshot showing it's working: |
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 got a different test failure locally for the webpack, so wonder if it's a transient failure:
Finished tests in 1674.866192s, 0.1128 tests/s, 0.3594 assertions/s.
Error:
HostJSTest::new host details page#test_0004_edit page:
Capybara::Ambiguous: Ambiguous match, found 2 elements matching visible button "Edit" that is not disabled
test/integration/host_js_test.rb:99:in `block (2 levels) in <class:HostJSTest>'
Failure:
SmartProxyJSTest#test_0003_edit page [test/integration/smart_proxy_js_test.rb:29]
Minitest::Assertion: expected "/smart_proxies" to equal "/smart_proxies/980190962-DHCP%20Proxy"
189 tests, 602 assertions, 1 failures, 1 errors, 0 skips
Can you try kicking them again, I don't have perms to do so.
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.
Can you please at least expand the commit message to describe why this is needed, but even better if you write a test for it.
@ekohl I updated the PR title and commit message, I am not having good luck getting a test to work in the |
@ekohl can this one get a review next week? We want to target it for Satellite 6.16, and it would be nice to get in if possible before branching. |
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.
For the commit message I'd recommend reading https://cbea.ms/git-commit/#why-not-how. Using the body to at least describe the use case is really helpful because you a simple git blame
can help you figure out why that was in the future.
As for testing, I can write a very simple reproducer with:
require 'integration_test_helper'
class HostDetailIntegrationTest < ActionDispatch::IntegrationTest
test "host with .ics extension" do
domain = FactoryBot.create(:domain, name: 'example.ics')
host = FactoryBot.create(:host, domain: domain)
visit host_details_page_path(host)
end
end
That shows me in current develop:
Error:
HostDetailIntegrationTest#test_0001_host with .ics extension:
ActionView::MissingTemplate: Missing template react/index with {:locale=>[:en], :formats=>[:ics], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :rabl]}. Searched in:
* "/home/ekohl/dev/foreman/app/views"
* "/home/ekohl/.rbenv/versions/3.0.4/lib/ruby/gems/3.0.0/gems/apipie-dsl-2.6.2/app/views"
* "/home/ekohl/.rbenv/versions/3.0.4/lib/ruby/gems/3.0.0/gems/apipie-rails-1.4.2/app/views"
app/controllers/react_controller.rb:7:in `index'
app/controllers/concerns/foreman/controller/timezone.rb:10:in `set_timezone'
app/models/concerns/foreman/thread_session.rb:32:in `clear_thread'
app/controllers/concerns/foreman/controller/topbar_sweeper.rb:12:in `set_topbar_sweeper_controller'
lib/foreman/middleware/libvirt_connection_cleaner.rb:9:in `call'
lib/foreman/middleware/telemetry.rb:10:in `call'
lib/foreman/middleware/logging_context_session.rb:22:in `call'
lib/foreman/middleware/logging_context_request.rb:11:in `call'
test/integration/host_detail_test.rb:7:in `block in <class:HostDetailIntegrationTest>'
And when I check out your PR it passes.
10797a0
to
ff03c11
Compare
@ekohl thanks for the help with the test, I have added a test and read the link you sent me and made a better commit message. Let me know how it looks when you get time, please. I will start using that when making commit messages to provide better context about my changes in future pr's |
@ekohl how does this look? I didn't see a block that it should fit in but did see another test outside of a block so put it after that one or did you want me to make a block such as view hosts and then put the test inside that? Are you able to rekick the test:katello one that had an issue, I don't have perms to restart the job. Looks like the test failures are unrelated, failing vcr tests with ACS |
This is what I indeed had intended. |
Updated, went with: describe "view host page" do
test "host with .ics extension" do
domain = FactoryBot.create(:domain, name: 'example.ics')
host = FactoryBot.create(:host, domain: domain)
visit host_details_page_path(host)
end
end |
When having a domain with a .ics extension we get a ActionView::MissingTemplate: Missing template react/index error. This PR change forces the react controller to use the HTML format. With this change the host is able to be loaded in the UI.
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.
Thanks!
Credit to @hao-yu for finding the fix. If you have a host with a domain extension .ics the all hosts pages fails to render the host (even the legacy UI) because rails is trying to ics as a format.
Testing steps:
Screenshots before and after:
Based on my understanding, I think it should not break anything because there is only one view in react directory: