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 #37228 - Replace Ansible/Roles page with React component #696

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions app/controllers/ansible_roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,6 @@ class AnsibleRolesController < ::ApplicationController
include Foreman::Controller::AutoCompleteSearch
include ForemanAnsible::Concerns::ImportControllerHelper
include ::ForemanAnsible::AnsibleRolesDataPreparations
def index
@ansible_roles = resource_base.search_for(params[:search],
:order => params[:order]).
paginate(:page => params[:page],
:per_page => params[:per_page])
end

def destroy
if @ansible_role.destroy
process_success
else
process_error
end
end

def import
changed = @importer.import!
@rows = prepare_ansible_import_rows(changed, @variables_importer)
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/ui_ansible_roles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ def index
@ui_ansible_roles = resource_scope_for_index(:permission => :view_ansible_roles)
end

api :DELETE, '/ui_ansible_roles/:id', N_('Deletes Ansible role')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any permission filtering here. Am I missing something?

Also, couldn't this just use the regular REST API to delete roles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I forgot to add the permission.

The table component uses the same endpoint for querying and deleting items, so it's unfortunately not possible to use the API just for deletion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is the right approach? Adding an API call here doesn't seem right to me. I know it might be simpler, but we can still use the existing API call to delete a role. It might require some frontend adjustments, but it will align with the rest of our code.
For reference, this is how we do it on the new hosts page: https://github.com/theforeman/foreman/blame/6aac916fb183f47596e8d543756faac27750a21e/webpack/assets/javascripts/react_app/components/HostsIndex/index.js#L283

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this would require significant changes to TableIndexPage.js and Table.js in core, as the idea is that TableIndexPage represents an interface to a single controller that has the necessary CRUD methods.
One could add another prop, deletionURL to TableIndexPage and Table that would take precedence over the original URL for deletion but I am not sure that is a cleaner solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we add the Delete option to the rowKebabItems action items instead of using the isDeleteable property? From there, we can use the URL of the existing API for the delete action.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we would lose the delete modal that way.

param :id, :identifier, :required => true
def destroy
AnsibleRole.find_by!(id: params[:id]).destroy
end

# restore original method from find_common to ignore resource nesting
def resource_scope(**kwargs)
@resource_scope ||= scope_for(resource_class, **kwargs)
Expand Down
47 changes: 0 additions & 47 deletions app/views/ansible_roles/index.html.erb

This file was deleted.

14 changes: 0 additions & 14 deletions app/views/ansible_roles/welcome.html.erb

This file was deleted.

14 changes: 13 additions & 1 deletion app/views/ui_ansible_roles/show.json.rabl
Original file line number Diff line number Diff line change
@@ -1,3 +1,15 @@
object @ansible_role

extends "api/v2/ansible_roles/show"
attributes :id, :name, :updated_at
code :hostgroups_count do |role|
role.hostgroups.count
end
code :hosts_count do |role|
role.hosts.count
end
code :variables_count do |role|
role.ansible_variables.count
end
code :can_delete do
User.current.can?(:destroy_ansible_roles)
end
5 changes: 3 additions & 2 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
end
end
scope '/ansible' do
match '/ansible_roles' => 'react#index', :via => [:get]
constraints(:id => %r{[^\/]+}) do
resources :hosts, :only => [] do
member do
Expand All @@ -52,15 +53,15 @@
end
end

resources :ansible_roles, :only => [:index, :destroy] do
resources :ansible_roles, :only => [:index] do
collection do
get :import
post :confirm_import
get 'auto_complete_search'
end
end

resources :ui_ansible_roles, :only => [:index]
resources :ui_ansible_roles, :only => [:index, :destroy]

resources :ansible_variables, :except => [:show] do
resources :lookup_values, :only => [:index, :create, :update, :destroy]
Expand Down
4 changes: 2 additions & 2 deletions lib/foreman_ansible/register.rb
Original file line number Diff line number Diff line change
Expand Up @@ -102,8 +102,8 @@
:ui_ansible_roles => [:index] },
:resource_type => 'AnsibleRole'
permission :destroy_ansible_roles,
{ :ansible_roles => [:destroy],
:'api/v2/ansible_roles' => [:destroy, :obsolete] },
{ :'api/v2/ansible_roles' => [:destroy, :obsolete],
:ui_ansible_roles => [:destroy] },
:resource_type => 'AnsibleRole'
permission :import_ansible_roles,
{ :ansible_roles => [:import, :confirm_import],
Expand Down
17 changes: 1 addition & 16 deletions test/functional/ansible_roles_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,4 @@

require 'test_plugin_helper'
# functional tests for AnsibleRolesController
class AnsibleRolesControllerTest < ActionController::TestCase
setup do
@role = FactoryBot.create(:ansible_role)
end

basic_index_test

test 'should destroy role' do
assert_difference('AnsibleRole.count', -1) do
delete :destroy,
:params => { :id => @role.id },
:session => set_session_user
end
assert_redirected_to ansible_roles_url
end
end
class AnsibleRolesControllerTest < ActionController::TestCase; end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's an empty class, is there any point in keeping it at all? I'd also note that there are system tests (https://guides.rubyonrails.org/testing.html#system-testing) that can use a real browser. I'd love to still have some tests.

Copy link
Contributor Author

@Thorben-D Thorben-D Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose I could remove it, but I would like to have a test for the import action at some point.
I will definitely add tests for the code changed in this PR though, just wanted us to agree on the implementation details first.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair.

7 changes: 7 additions & 0 deletions test/functional/ui_ansible_roles_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,11 @@ class UiAnsibleRolesControllerTest < ActionController::TestCase
res = JSON.parse @response.body
assert_equal res['total'], res['results'].size
end

test 'should destroy role' do
delete :destroy,
:params => { :id => @role.id },
:session => set_session_user
assert_response :success
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test to verify that a user without the destroy_ansible_roles permission cannot perform this action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if this is necessary. The permissions should already be tested by a test in core and I didn't really find an existing test that tested this.

Copy link
Contributor

@sbernhard sbernhard Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MariaAga what are your thoughts about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not my area, but I couldnt find a test in core for it in core

end
78 changes: 78 additions & 0 deletions webpack/components/AnsibleRoles/AnsibleRolesPage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import React from 'react';
import { useAPI } from 'foremanReact/common/hooks/API/APIHooks';
import TableIndexPage from 'foremanReact/components/PF4/TableIndexPage/TableIndexPage';
import RelativeDateTime from 'foremanReact/components/common/dates/RelativeDateTime';
import { translate as __ } from 'foremanReact/common/I18n';
import { getDocsURL } from 'foremanReact/common/helpers';
import {
hostgroupsLink,
hostsLink,
variablesLink,
} from './AnsibleRolesPageHelpers';
import { AnsibleRolesImportButtonWrapper } from './components/AnsibleRolesImportButtonWrapper';
import { showToast } from '../../toastHelper';

export const AnsibleRolesPage = () => {
const {
response: { results },
status,
} = useAPI('get', '/api/v2/permissions/current_permissions');

if (status === 'ERROR') {
showToast({
type: 'error',
message: __('There was an error requesting user permissions'),
});
}

return (
<TableIndexPage
header={__('Ansible Roles')}
controller="ansible_roles"
apiUrl="/ansible/ui_ansible_roles"
apiOptions={{ key: 'ANSIBLE_ROLES_API_REQUEST_KEY' }}
hasHelpPage
creatable={false}
isDeleteable
customToolbarItems={
<AnsibleRolesImportButtonWrapper
apiStatus={status}
allPermissions={results}
/>
}
customHelpURL={getDocsURL(
'Managing_Configurations_Ansible',
'Importing_Ansible_Roles_and_Variables_ansible'
)}
columns={{
name: { title: __('Name'), isSorted: true, weight: 0 },
hostgroups_count: {
title: __('Hostgroups'),
weight: 1,
wrapper: ({ name, hostgroups_count: hostgroupsCount }) =>
hostgroupsLink(name, hostgroupsCount, status, results),
},
hosts_count: {
title: __('Hosts'),
weight: 2,
wrapper: ({ name, hosts_count: hostsCount }) =>
hostsLink(name, hostsCount, status, results),
},
variables_count: {
title: __('Variables'),
weight: 3,
wrapper: ({ name, variables_count: variablesCount }) =>
variablesLink(name, variablesCount, status, results),
},
updated_at: {
title: __('Imported at'),
isSorted: true,
weight: 4,
wrapper: ({ updated_at: updatedAt }) => (
<RelativeDateTime date={new Date(updatedAt)} />
),
},
}}
/>
);
};
61 changes: 61 additions & 0 deletions webpack/components/AnsibleRoles/AnsibleRolesPageHelpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import React from 'react';

export const hostgroupsLink = (
roleName,
hostgroupsCount,
apiStatus,
permissions
) => {
if (
apiStatus === 'RESOLVED' &&
permissions.some(perm => perm.name === 'view_hostgroups')
) {
return link(hostgroupsUrl(roleName), hostgroupsCount);
}
return hostgroupsCount;
};

export const hostsLink = (
roleName,
hostgroupsCount,
apiStatus,
permissions
) => {
if (
apiStatus === 'RESOLVED' &&
permissions.some(perm => perm.name === 'view_hosts')
) {
return link(hostsUrl(roleName), hostgroupsCount);
}
return hostgroupsCount;
};

export const variablesLink = (
roleName,
hostgroupsCount,
apiStatus,
permissions
) => {
if (
apiStatus === 'RESOLVED' &&
permissions.some(perm => perm.name === 'view_ansible_variables')
) {
return link(variablesUrl(roleName), hostgroupsCount);
}
return hostgroupsCount;
};

const link = (url, displayText) => (
<a href={url} target="_blank" rel="noreferrer">
{displayText}
</a>
);

const hostgroupsUrl = roleName => `/hostgroups${searchString(roleName)}`;
const hostsUrl = roleName => `/new/hosts${searchString(roleName)}`;
const variablesUrl = roleName => `ansible_variables${searchString(roleName)}`;
const searchString = roleName =>
`?search=${encodeURIComponent(`ansible_role = ${roleName}`)}`;

export const importUrl = (proxyName, proxyId) =>
`ansible_roles/import?proxy=${encodeURIComponent(`${proxyId}-${proxyName}`)}`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
export const testSmartProxies = [
{
created_at: '2024-06-21 14:49:35 +0200',
updated_at: '2024-06-21 14:49:35 +0200',
hosts_count: 0,
name: 'debuggable',
id: 2,
url: 'http://smart-proxy-ansible-capable.example.com:8080',
remote_execution_pubkey: null,
download_policy: 'on_demand',
supported_pulp_types: [],
lifecycle_environments: [],
features: [
{
capabilities: ['ansible-runner', 'single'],
name: 'Dynflow',
id: 17,
},
{
capabilities: ['vcs_clone'],
name: 'Ansible',
id: 20,
},
{
capabilities: [],
name: 'Logs',
id: 13,
},
],
},
];
Loading
Loading