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

Update main nav and UI for local Advisor Engine #932

Open
wants to merge 7 commits into
base: develop
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
6 changes: 6 additions & 0 deletions app/controllers/insights_cloud/settings_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
module InsightsCloud
class SettingsController < ::ApplicationController
def show
if ForemanRhCloud.with_local_advisor_engine?
render json: {
error: _('This Foreman is configured to use Insights on Premise. Syncing to Insights Cloud is disabled.'),
}, status: :unprocessable_entity
return
end
render_setting(:insightsSyncEnabled, :allow_auto_insights_sync)
Comment on lines +8 to 10
Copy link
Member

Choose a reason for hiding this comment

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

Nit: is this easier to follow? I generally try to avoid return and rely on the visual indentation to follow the code.

Suggested change
return
end
render_setting(:insightsSyncEnabled, :allow_auto_insights_sync)
else
render_setting(:insightsSyncEnabled, :allow_auto_insights_sync)
end

Though taking a step back, from an API perspective I wonder why this is even here in the first place. We already have an API to show settings: /api/settings/:id that supports both GET and PUT.

Are there any permission checks on this controller to prevent unauthenticated users to change the setting or can any user change this for any organization?

end

Expand Down
16 changes: 13 additions & 3 deletions lib/foreman_rh_cloud/engine.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,15 @@ def self.register_scheduled_task(task_class, cronline)
Role::SYSTEM_ADMIN => plugin_permissions

# Adding a sub menu after hosts menu
divider :top_menu, caption: N_('RH Cloud'), parent: :configure_menu
menu :top_menu, :inventory_upload, caption: N_('Inventory Upload'), url: '/foreman_rh_cloud/inventory_upload', url_hash: { controller: :react, action: :index }, parent: :configure_menu
menu :top_menu, :insights_hits, caption: N_('Insights'), url: '/foreman_rh_cloud/insights_cloud', url_hash: { controller: :react, action: :index }, parent: :configure_menu
divider :top_menu, caption: N_('Insights'), parent: :configure_menu
menu :top_menu,
:inventory_upload,
caption: N_('Inventory Upload'),
url: '/foreman_rh_cloud/inventory_upload',
url_hash: { controller: :react, action: :index },
parent: :configure_menu,
if: -> { !ForemanRhCloud.with_local_advisor_engine? }
menu :top_menu, :insights_hits, caption: N_('Recommendations'), url: '/foreman_rh_cloud/insights_cloud', url_hash: { controller: :react, action: :index }, parent: :configure_menu

register_facet InsightsFacet, :insights do
configure_host do
Expand Down Expand Up @@ -193,4 +199,8 @@ def self.register_scheduled_task(task_class, cronline)
end
end
end

def self.with_local_advisor_engine?
SETTINGS&.[](:foreman_rh_cloud)&.[](:use_local_advisor_engine) || false
Copy link
Member

Choose a reason for hiding this comment

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

You can use dig on a hash and I think it's safe to assume SETTINGS is a hash (so & is redundant):

Suggested change
SETTINGS&.[](:foreman_rh_cloud)&.[](:use_local_advisor_engine) || false
SETTINGS.dig(:foreman_rh_cloud, :use_local_advisor_engine) || false

end
end
13 changes: 13 additions & 0 deletions test/controllers/insights_sync/settings_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,19 @@

class SettingsControllerTest < ActionController::TestCase
tests InsightsCloud::SettingsController
def setup
ForemanRhCloud.stubs(:with_local_advisor_engine?).returns(false)
end

test 'should return error if insights on premise' do
ForemanRhCloud.stubs(:with_local_advisor_engine?).returns(true)

get :show, session: set_session_user

assert_response :unprocessable_entity
actual = JSON.parse(response.body)
assert_equal 'This Foreman is configured to use Insights on Premise. Syncing to Insights Cloud is disabled.', actual['error']
end

test 'should return allow_auto_insights_sync setting' do
Setting[:allow_auto_insights_sync] = false
Expand Down
Original file line number Diff line number Diff line change
@@ -1,32 +1,49 @@
import React, { Component } from 'react';
import React, { useEffect, useContext } from 'react';
import PropTypes from 'prop-types';
import { translate as __ } from 'foremanReact/common/I18n';
import SwitcherPF4 from '../../../common/Switcher/SwitcherPF4';
import { InsightsConfigContext } from '../../InsightsCloudSync';
import './insightsSettings.scss';

class InsightsSettings extends Component {
componentDidMount() {
const { getInsightsSyncSettings } = this.props;
getInsightsSyncSettings();
}
const InsightsSettings = ({
insightsSyncEnabled,
getInsightsSyncSettings,
setInsightsSyncEnabled,
}) => {
const { isLocalInsightsAdvisor, setIsLocalInsightsAdvisor } = useContext(
InsightsConfigContext
);
useEffect(() => {
async function fetchData() {
try {
await getInsightsSyncSettings();
} catch (err) {
if (err.cause?.response?.status === 422) {
setIsLocalInsightsAdvisor(true);
} else {
throw err;
}
}
}
fetchData();
}, [getInsightsSyncSettings, setIsLocalInsightsAdvisor]);

render() {
const { insightsSyncEnabled, setInsightsSyncEnabled } = this.props;
return (
<div className="insights_settings">
<SwitcherPF4
id="insights_sync_switcher"
label={__('Sync automatically')}
tooltip={__(
'Enable automatic synchronization of Insights recommendations from the Red Hat cloud'
)}
isChecked={insightsSyncEnabled}
onChange={() => setInsightsSyncEnabled(!insightsSyncEnabled)}
/>
</div>
);
}
}
if (isLocalInsightsAdvisor) return null;

return (
<div className="insights_settings">
<SwitcherPF4
id="insights_sync_switcher"
label={__('Sync automatically')}
tooltip={__(
'Enable automatic synchronization of Insights recommendations from the Red Hat cloud'
)}
isChecked={insightsSyncEnabled}
onChange={() => setInsightsSyncEnabled(!insightsSyncEnabled)}
/>
</div>
);
};

InsightsSettings.propTypes = {
insightsSyncEnabled: PropTypes.bool.isRequired,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ export const getInsightsSyncSettings = () => async dispatch => {
},
},
});
} catch ({ message }) {
} catch (err) {
const { message } = err;
if (err?.response?.status === 422) {
Copy link
Member

Choose a reason for hiding this comment

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

Should this catch any non-2xx code? Any network connection can fail so it could return 5xx.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since I send myself the 422 specifically for this check, I thought it made sense to check for it specifically as well. But I may end up re-architecting this whole thing anyway, stand by..

Copy link
Member

Choose a reason for hiding this comment

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

In that case, perhaps it should raise a more specific exception than Error?

throw new Error(message, { cause: err });
}
dispatch(
addToast({
sticky: true,
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable react-hooks/exhaustive-deps */
import React, { useEffect } from 'react';
import React, { useEffect, useContext } from 'react';
import PropTypes from 'prop-types';
import { Table, TableHeader, TableBody } from '@patternfly/react-table';
import { useForemanSettings } from 'foremanReact/Root/Context/ForemanContext';
Expand All @@ -12,6 +12,7 @@ import TableEmptyState from '../../../common/table/EmptyState';
import { modifySelectedRows, getSortColumnIndex } from './InsightsTableHelpers';
import Pagination from './Pagination';
import './table.scss';
import { InsightsConfigContext } from '../../InsightsCloudSync';

const InsightsTable = ({
page,
Expand All @@ -37,6 +38,7 @@ const InsightsTable = ({
const perPage = urlPerPage || appPerPage;
const [rows, setRows] = React.useState([]);
const [columns, setColumns] = React.useState(defaultColumns);
const { isLocalInsightsAdvisor } = useContext(InsightsConfigContext);

// acts as componentDidMount
useEffect(() => {
Expand All @@ -45,7 +47,13 @@ const InsightsTable = ({

useEffect(() => {
setRows(
modifySelectedRows(hits, selectedIds, showSelectAllAlert, hideHost)
modifySelectedRows(
hits,
selectedIds,
showSelectAllAlert,
hideHost,
isLocalInsightsAdvisor
)
);

if (hideHost) setColumns(getColumnsWithoutHostname());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@ export const hasPlaybookFormatter = ({ title: hasPlaybook }) => ({
});

export const actionsFormatter = (props, { rowData = {} }) => {
const { recommendationUrl, accessRHUrl } = rowData;
const { recommendationUrl, accessRHUrl, isLocalInsightsAdvisor } = rowData;
const dropdownItems = [];

recommendationUrl &&
!isLocalInsightsAdvisor &&
dropdownItems.push(
<DropdownItem key="recommendation-url">
<a href={recommendationUrl} target="_blank" rel="noopener noreferrer">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ export const modifySelectedRows = (
hits,
selectedIds,
showSelectAllAlert,
hideHost
hideHost,
isLocalInsightsAdvisor = false
) => {
if (hits.length === 0) return [];

Expand Down Expand Up @@ -34,6 +35,7 @@ export const modifySelectedRows = (
selected: selectedIds[id] || (disableCheckbox && showSelectAllAlert),
recommendationUrl: results_url,
accessRHUrl: solution_url,
isLocalInsightsAdvisor,
};
}
);
Expand Down
11 changes: 9 additions & 2 deletions webpack/InsightsCloudSync/Components/ToolbarDropdown.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,23 @@
import React, { useState } from 'react';
import React, { useState, useContext } from 'react';
import PropTypes from 'prop-types';
import { translate as __ } from 'foremanReact/common/I18n';
import { Dropdown, DropdownItem, KebabToggle } from '@patternfly/react-core';
import { ExternalLinkAltIcon } from '@patternfly/react-icons';
import { redHatAdvisorSystems } from '../InsightsCloudSyncHelpers';
import { InsightsConfigContext } from '../InsightsCloudSync';

const ToolbarDropdown = ({ onRecommendationSync }) => {
const [isDropdownOpen, setIsDropdownOpen] = useState(false);
const dropdownItems = [
const { isLocalInsightsAdvisor } = useContext(InsightsConfigContext);
const baseItems = [
<DropdownItem
key="recommendation-manual-sync"
onClick={onRecommendationSync}
>
{__('Sync recommendations')}
</DropdownItem>,
];
const cloudItems = [
<DropdownItem key="cloud-advisor-systems-link">
<a
href={redHatAdvisorSystems()}
Expand All @@ -26,6 +30,9 @@ const ToolbarDropdown = ({ onRecommendationSync }) => {
</a>
</DropdownItem>,
];
const dropdownItems = isLocalInsightsAdvisor
? baseItems
: baseItems.concat(cloudItems);
return (
<Dropdown
className="title-dropdown"
Expand Down
31 changes: 19 additions & 12 deletions webpack/InsightsCloudSync/InsightsCloudSync.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import PageLayout from 'foremanReact/routes/common/PageLayout/PageLayout';
import InsightsHeader from './Components/InsightsHeader';
Expand All @@ -13,7 +13,10 @@ import Pagination from './Components/InsightsTable/Pagination';
import ToolbarDropdown from './Components/ToolbarDropdown';
import InsightsSettings from './Components/InsightsSettings';

export const InsightsConfigContext = React.createContext();

const InsightsCloudSync = ({ syncInsights, query, fetchInsights }) => {
const [isLocalInsightsAdvisor, setIsLocalInsightsAdvisor] = useState(false);
const onRecommendationSync = () => syncInsights(fetchInsights, query);
const toolbarButtons = (
<>
Expand All @@ -29,18 +32,22 @@ const InsightsCloudSync = ({ syncInsights, query, fetchInsights }) => {

return (
<div className="rh-cloud-insights">
<InsightsSettings />
<PageLayout
searchable
searchProps={INSIGHTS_SEARCH_PROPS}
onSearch={nextQuery => fetchInsights({ query: nextQuery, page: 1 })}
header={INSIGHTS_SYNC_PAGE_TITLE}
toolbarButtons={toolbarButtons}
searchQuery={query}
beforeToolbarComponent={<InsightsHeader />}
<InsightsConfigContext.Provider
value={{ isLocalInsightsAdvisor, setIsLocalInsightsAdvisor }}
>
<InsightsTable />
</PageLayout>
<InsightsSettings />
<PageLayout
searchable
searchProps={INSIGHTS_SEARCH_PROPS}
onSearch={nextQuery => fetchInsights({ query: nextQuery, page: 1 })}
header={INSIGHTS_SYNC_PAGE_TITLE}
toolbarButtons={toolbarButtons}
searchQuery={query}
beforeToolbarComponent={<InsightsHeader />}
>
<InsightsTable />
</PageLayout>
</InsightsConfigContext.Provider>
</div>
);
};
Expand Down
Loading
Loading