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.

Sign up for GitHub

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

Jump to bottom

Improve routes handling #1192

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Improve routes handling #1192

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f82a90
refactor(Routes): Remove * wrapping route
gkarat Aug 19, 2024
92b6ad5
chore: Remove deprecated ErrorComponent prop passing
gkarat Aug 19, 2024
fed5d4d
refactor: Replace PermissionRoute with a plain component
gkarat Aug 22, 2024
c7be848
refactor: Use relative paths for routes at Routes.js.
gkarat Aug 29, 2024
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
28 changes: 28 additions & 0 deletions src/AccessWrapper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import PropTypes from 'prop-types';
import { Bullseye, Spinner } from '@patternfly/react-core';
import { NotAuthorized } from '@redhat-cloud-services/frontend-components';
import { usePermissionsWithContext } from '@redhat-cloud-services/frontend-components-utilities/RBACHook';

const AccessWrapper = ({ children }) => {
const { hasAccess, isLoading } = usePermissionsWithContext([
'patch:*:*',
'patch:*:read'
]);

return isLoading ? (
<Bullseye>
<Spinner />
</Bullseye>
) : hasAccess ? (
children
) : (
<NotAuthorized serviceName="patch" />
);
};

AccessWrapper.propTypes = {
children: PropTypes.any
};

export default AccessWrapper;
5 changes: 4 additions & 1 deletion src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { changeGlobalTags, changeProfile, globalFilter } from './store/Actions/A
import { mapGlobalFilters } from './Utilities/Helpers';
import './App.scss';
import Routes from './Routes';
import AccessWrapper from './AccessWrapper';

const App = () => {
const dispatch = useDispatch();
Expand Down Expand Up @@ -46,7 +47,9 @@ const App = () => {
<React.Fragment>
<NotificationPortal />
<RBACProvider appName="patch">
<Routes />
<AccessWrapper>
<Routes />
</AccessWrapper>
</RBACProvider>
</React.Fragment>
);
Expand Down
85 changes: 30 additions & 55 deletions src/Routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,10 @@
import { Bullseye, Spinner } from '@patternfly/react-core';
import { NotAuthorized } from '@redhat-cloud-services/frontend-components/NotAuthorized';
import { usePermissionsWithContext } from '@redhat-cloud-services/frontend-components-utilities/RBACHook';
import AsyncComponent from '@redhat-cloud-services/frontend-components/AsyncComponent';
import axios from 'axios';
import PropTypes from 'prop-types';
import React, { lazy, Suspense, useEffect, useState } from 'react';
import { Navigate, Outlet, Route, Routes } from 'react-router-dom';
import { Navigate, Route, Routes } from 'react-router-dom';
import { NavigateToSystem } from './Utilities/NavigateToSystem';

const PermissionRoute = ({ requiredPermissions = [] }) => {
const { hasAccess, isLoading } = usePermissionsWithContext(requiredPermissions);
if (!isLoading) {
return hasAccess ? <Outlet /> : <NotAuthorized serviceName="patch" />;
} else {
return '';
}
};

PermissionRoute.propTypes = {
requiredPermissions: PropTypes.oneOfType([
PropTypes.arrayOf(PropTypes.string),
PropTypes.string
])
};

const Advisories = lazy(() =>
import(
/* webpackChunkName: "Advisories" */ './SmartComponents/Advisories/Advisories'
Expand Down Expand Up @@ -73,8 +54,8 @@ const TemplateDetail = lazy(() =>
);

const PatchRoutes = () => {
const generalPermissions = ['patch:*:*', 'patch:*:read'];
const [hasSystems, setHasSystems] = useState(null);

const INVENTORY_TOTAL_FETCH_URL = '/api/inventory/v1/hosts';
const RHEL_ONLY_FILTER = '?filter[system_profile][operating_system][RHEL][version][gte]=0';

Expand Down Expand Up @@ -109,41 +90,35 @@ const PatchRoutes = () => {
</Suspense>
) : (
<Routes>
<Route path="/advisories" element={<Advisories />} />
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 know why it work here with absolute paths, but these should be relative. All other applications use relative paths for their routes[0].

However, I think we should try to make these relative as well and also make sure we consistently use the insights navigate stuff, so links resolve properly.

[0] https://github.com/RedHatInsights/compliance-frontend/blob/master/src/Routes.js#L23

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 don't know why it work here with absolute paths

It simply matches the routes starting by the basename (which is / in our case because /insights/patch pathname prefix is cut by chrome/federated modules related reason). Navigating to /advisories will match with the route having a path /advisories. As well as it will match the route with just advisories path. In our case, it doesn't have any disadvantages but inconsistency with other apps, as you mentioned. Though, all apps are inconsistent, Inventory has the same "root slash preference": https://github.com/RedHatInsights/insights-inventory-frontend/blob/master/src/Routes.js.

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 would say these paths starting with a slash are actually more specific, they are fully matching the route. Though, if for example path="advisories" would be by accident placed under some other route, it will lead to the situation where any path that has some slices and ends with "advisories" will match. It's very in theory though, but just imagining.

Do you think it's worth consolidating?

Copy link
Member

Choose a reason for hiding this comment

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

I think this here works maybe only because of some trickery we do in the routing in chrome, but I don't think it is really correct and might cause issues.

We should make these relative and remove the / from the paths (here and in all apps where we have this). These routes are nested within the /insights/patch path and starting the path for them with a / is not correct.

We should also consistently use the InsightsLink and useInsightsNavigate, because these will turn relative paths like "advisories" into "/insights/patch/advisories".

These changes seem unnecessary and everything kinda works at the moment, but this is only because of trickery and luck. What these changes do is make our routing in apps comply with React router conventions in order to reduce the burden on routing.

I also saw some ".." as a to attribute of Link components, these are dangerous, because they can lead to different places under a different context, for example with federated modules.

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 think I get your point, this makes sense to use relative paths in accordance to the react-router documentation. Regarding the cut paths: I don't think it's just of trickery and luck :D IIRC, it's made on purpose that our applications have baseName set to / even though we always have some /insights/abc preceding. Which is made to avoid real base name rewrite or make the navigation work properly.

I will move the things to relative paths now.

<Route
path="/advisories/:advisoryId/:inventoryId"
element={<NavigateToSystem />}
/>
<Route
path="/advisories/:advisoryId"
element={<AdvisoryPage />}
/>
<Route path="/systems" element={<Systems />} />
<Route
path="/systems/:inventoryId"
element={<InventoryDetail />}
/>
<Route path="/packages" element={<PackagesPage />} />
<Route
path="/packages/:packageName"
element={<PackageDetail />}
/>
<Route
path="/packages/:packageName/:inventoryId"
element={<NavigateToSystem />}
/>
<Route path="/templates" element={<Templates />} />
<Route
element={
<PermissionRoute requiredPermissions={generalPermissions} />
}
>
<Route path="/advisories" element={<Advisories />} />
<Route
path="/advisories/:advisoryId/:inventoryId"
element={<NavigateToSystem />}
/>
<Route
path="/advisories/:advisoryId"
element={<AdvisoryPage />}
/>
<Route path="/systems" element={<Systems />} />
<Route
path="/systems/:inventoryId"
element={<InventoryDetail />}
/>
<Route path="/packages" element={<PackagesPage />} />
<Route
path="/packages/:packageName"
element={<PackageDetail />}
/>
<Route
path="/packages/:packageName/:inventoryId"
element={<NavigateToSystem />}
/>
<Route path="/templates" element={<Templates />} />
<Route
path="/templates/:templateName"
element={<TemplateDetail />}
/>
<Route path="*" element={<Navigate to="advisories" />} />
</Route>
path="/templates/:templateName"
element={<TemplateDetail />}
/>
<Route path="*" element={<Navigate to="advisories" />} />
</Routes>
);
};
Expand Down