-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add timeline to physical server #2272
Conversation
@miq-bot add_label enhancement |
506f24a
to
76b0fca
Compare
Looks like just a couple of tests need passing. |
@juliancheal this PR depends on core PR 16084, and that's why the tests are failing. I saw your restarted its build, which had failed, and everything looks fine now. |
@lucashsilva ah thanks, I'd missed that in the first comment that it relied on another PR, thanks! |
@juliancheal @AparnaKarve everything looks fine now :) |
This pull request is not mergeable. Please rebase and repush. |
@agrare could you please take a look at this? |
ping @AparnaKarve |
tl_build_timeline | ||
drop_breadcrumb(:name => _("Timelines"), :url => show_link(@record, :refresh => "n", :display => "timeline")) | ||
session[:tl_record_id] = @record.id | ||
javascript_redirect(polymorphic_path(@record, :display => 'timeline').sub!('.', '/show/')) |
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.
@lucashsilva Since you have defined the mixin Mixins::MoreShowActions
at the top, could you simply call the method show_timeline
instead of writing this block?
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.
@AparnaKarve I figured out I didn't use anything from this mixin and that the model I followed didn't either. Sorry for that. I also tried to do what you said but the user is not redirected to the timelines page and nothing is shown. If you look at this, you'll see a similar code, which was the way I found to redirect the user to the timelines page. How should I proceed?
Is it necessary to execute the lines 51 to 57 if the user will be redirected to a new page?
I could get the button to work and redirect correctly with lines 52 and 58 only and without the mixin.
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.
@lucashsilva I think it would be better to use the mixin.
The eventual goal would be to make use of the mixin in the entire app and remove duplication.
Can you give this a try -
if params[:pressed] == "physical_server_timeline"
@record = find_record_with_rbac(ManageIQ::Providers::PhysicalInfraManager::PhysicalServer, params[:id])
show_timeline
javascript_redirect :action => 'show', :id => @record.id, :display => 'timeline'
end
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.
@AparnaKarve I tried it and it works. I already pushed the change.
@lucashsilva Overall the code looks good. |
Checked commits lucashsilva/manageiq-ui-classic@287a65b~...dabe446 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 |
@AparnaKarve can you give it another look? Thanks! |
Reviewed by @Fryguy and @blomquisg -- merging. |
This PR consists in adding the monitoring button group with timelines button to the physical server page and making the timeline page available when this button is clicked.
Physical server page with monitoring button group
Disabled when there are no events for the physical server
Enabled when there are events for the physical server
Timeline page before filtering
Timeline page after filtering
This PR depends on core PR 16084.