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.

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 #24149 - use new template renderer #7496

Merged
merged 1 commit into from
Aug 7, 2018

Conversation

timogoebel
Copy link
Contributor

@timogoebel timogoebel commented Jul 3, 2018

This needs theforeman/foreman#5683 and theforeman/foreman_remote_execution#363.

I don't have a katello development environment at hand and don't know the codebase well enough. This is a shot in the dark. Please help me with testing this.

@theforeman-bot
Copy link

Issues: #24149

@jlsherrill
Copy link
Member

I still need to test this, but the update seems sane 👍

@timogoebel
Copy link
Contributor Author

Updated to match the latest changes in the core PR.

@johnpmitsch
Copy link
Contributor

@timogoebel running into the issue I mentioned on the mailing list (this was before I saw this PR), the subscriptions page isn't loading.

@timogoebel
Copy link
Contributor Author

I can't tell how this could be related. The commit you mention just changed stuff in the template rendering. notification_recipients_path should be defined by rails based on the defined routes.

Can you look into this a bit more?

My gut feeling is that Foreman::ForemanUrlRenderer was mixed into some controller (as templates were rendered in the controller context) and included this that made the url available. But I don't have a katello dev env to verify.

@akofink
Copy link
Member

akofink commented Jul 31, 2018

This only seems to be an issue on all our react pages in Katello (RH Repos and Subscriptions). rake routes shows the route there, so it must just be the missing helper method. The actual trace shows the error coming from mount_react_component in foreman.

@johnpmitsch
Copy link
Contributor

I'm really not sure where to start looking, maybe someone more familiar with the templating change can weigh in. Git bisect narrows it down to the template rendering change so something must have changed that we are missing.

@ares @kamils-iRonin @tbrisker @tstrachota @lzap Do you have any ideas on the error we are seeing on the Katello React pages?

subs page

@akofink
Copy link
Member

akofink commented Jul 31, 2018

IDK if this is helpful, but I checked in the console with Katello loaded, and the following works fine:

> include Rails.application.routes.url_helpers
> notification_recipients_path
=> "/notification_recipients"

@timogoebel
Copy link
Contributor Author

Can you try changing this to something like:

render layout: 'katello/layouts/react'

and defining an empty view for the index action?

@tbrisker
Copy link
Member

tbrisker commented Aug 1, 2018

ForemanUrlRenderer that loads the urls was indeed previously included by Foreman::Renderer which was removed here theforeman/foreman@f715a7c#diff-6632a0077c474a44423343eb597f9d72L13
I haven't managed to pinpoint exactly where katello loaded the renderer from and why it broke now on the react pages, but perhaps adding include Rails.application.routes.url_helpers to the react controller will fix this issue.

@timogoebel
Copy link
Contributor Author

@tbrisker: I had similar thoughts. I guess the methods need to be available in the helpers and not in the controller, correct?
But I believe a katello maintainer needs to look at this. I expect them to know katello and the codebase better then I do.

@akofink
Copy link
Member

akofink commented Aug 1, 2018

Yes, it appears adding include Rails.application.routes.url_helpers to the ReactController fixes the issue. I also noticed that we do this already in the BastionController, so it's not without precedent.

@timogoebel
Copy link
Contributor Author

A fix for the url issue is in #7589.

@jlsherrill
Copy link
Member

[test katello]

@jlsherrill jlsherrill self-assigned this Aug 6, 2018
@jlsherrill
Copy link
Member

@timogoebel works well for me! I've opened an issue to add tests to all the untested extensions here: https://projects.theforeman.org/issues/24549

and will be opening a PR to add those, but thanks for your updates here.

@jlsherrill jlsherrill merged commit 9596c68 into Katello:master Aug 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants