Fixes #23163 - Fix breadcrumbs in template invocation details #355
Conversation
adamruzicka
commented
May 31, 2018
adamruzicka
commented
| @@ -0,0 +1,46 @@ | |||
| module Api | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
There was a problem hiding this comment.
@houndci-bot neither @rubocop is happy about this
|
CC @sharvit could you take a look please? |
| N_('List template invocations belonging to job invocation') | ||
| param_group :search_and_pagination, ::Api::V2::BaseController | ||
| param :id, :identifier, :required => true | ||
| def template_invocations |
There was a problem hiding this comment.
why not calling it index, which is the standard name for method listing all the resources in rails?
There was a problem hiding this comment.
Perhaps it was moved from JobInvocationController?
There was a problem hiding this comment.
Is it still an index when it is by default scoped by another resource? I would expect index to return all template invocations (with possible search), not only template invocations belonging to a certain job invocation
|
|
||
| before_action :find_job_invocation, :only => %w{template_invocations} | ||
|
|
||
| api :GET, '/job_invocations/:id/template_invocations', |
There was a problem hiding this comment.
In nested resources, we would usually do :job_invocation_id instead of :id, as the :id usually refers to the resource of the controller itself (which is TemplateInvocation in this case`)
There was a problem hiding this comment.
This leads to a slightly confusing situation where the API description states it is job_invocation_id, but in the controller it is accessible as params[:id]
| module V2 | ||
| class TemplateInvocationsController < ::Api::V2::BaseController | ||
| include ::Api::Version2 | ||
| include ::Foreman::Renderer |
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
One thing I forgot to ask yesterday: could you cover it by some controller tests? |
There was a problem hiding this comment.
I think it looks good! Thanks @adamruzicka 👍
Just one small comment from my side...
|
|
||
| <% breadcrumbs(:resource_url => template_invocations_api_job_invocation_path(@template_invocation.job_invocation_id), | ||
| :name_field => 'host_name', | ||
| :switcher_item_url => '/template_invocations/:id', |
There was a problem hiding this comment.
Is it possible to use rails helper to create the switcher_item_url?
Example:
https://github.com/theforeman/foreman/blob/833e2aef24d6f3cb6eae90213ea198dd354a5709/app/views/provisioning_templates/edit.html.erb#L2
| @@ -0,0 +1,31 @@ | |||
| require 'test_plugin_helper' | |||
There was a problem hiding this comment.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
| @@ -0,0 +1,45 @@ | |||
| # frozen_string_literal: true | |||
| module Api | |||
There was a problem hiding this comment.
Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
adamruzicka
force-pushed
the
fix/breadcrumbs-23163
branch
from
4023152
to
dc13ac6
Compare
|
Re-tested and works like a charm. Thanks @adamruzicka |
