-
Notifications
You must be signed in to change notification settings - Fork 3
Improvement/artesca 15276 remove dataservice artesca plus veeam #875
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
Improvement/artesca 15276 remove dataservice artesca plus veeam #875
Conversation
Hello jeanmarcmilletscality,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command:
Alternatively, the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
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.
Pull Request Overview
This PR adds a feature flag to hide data service routes and navigation items when Veeam-only mode is enabled.
- Imports and uses a new
useIsVeeamVBROnly
hook - Wraps the “create-dataservice” route in a conditional to exclude it in Veeam-only mode
- Conditionally removes the “Data Services” navigation item in internal routes
Comments suppressed due to low confidence (2)
src/react/Routes.tsx:108
- [nitpick] The variable name
isArtescaPlusVeeamEnabled
is inconsistent with the hook nameuseIsVeeamVBROnly
and is somewhat ambiguous. Consider renaming it toisVeeamVBROnly
for clarity and alignment with the hook.
const isArtescaPlusVeeamEnabled = useIsVeeamVBROnly();
src/react/Routes.tsx:214
- This new conditional rendering for data service routes should have corresponding unit or integration tests to verify that routes are correctly included or excluded based on the Veeam-only flag.
{!isArtescaPlusVeeamEnabled && (
@@ -386,6 +393,7 @@ function InternalRoutes() { | |||
const { isStorageManager } = useAuthGroups(); | |||
const config = useConfig(); | |||
const navigate = useBasenameRelativeNavigate(); | |||
const isArtescaPlusVeeamEnabled = useIsVeeamVBROnly(); |
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.
[nitpick] This hook is called in both PrivateRoutes
and InternalRoutes
, leading to duplication. Consider lifting this logic into a shared parent component or a custom hook wrapper to reduce repetition.
Copilot uses AI. Check for mistakes.
} | ||
/> | ||
{!isArtescaPlusVeeamEnabled && ( | ||
<> |
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.
[nitpick] The React fragment here wraps only a single <Route>
component. You can remove the fragment to simplify the JSX.
Copilot uses AI. Check for mistakes.
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.
Pull Request Overview
This PR introduces conditional rendering of Data Service routes and sidebar entries based on the useIsVeeamVBROnly
feature flag and adds corresponding tests.
- Imported and applied
useIsVeeamVBROnly
inPrivateRoutes
andInternalRoutes
- Wrapped
create-dataservice
anddataservices
routes in a conditional to hide them for Veeam VBR–only users - Updated sidebar configuration to exclude the “Data Services” action when the flag is enabled
- Added tests in
Routes.test.tsx
to cover both route and sidebar behaviors
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/react/Routes.tsx | Added useIsVeeamVBROnly import; conditionally hide DataService routes and sidebar entry |
src/react/Routes.test.tsx | New tests mocking useIsVeeamVBROnly to verify route and sidebar visibility |
Comments suppressed due to low confidence (2)
src/react/Routes.tsx:108
- [nitpick] The variable name
isArtescaPlusVeeamEnabled
is verbose and doesn't align directly with the hookuseIsVeeamVBROnly
. Consider renaming it toisVeeamVBROnly
for consistency and clarity.
const isArtescaPlusVeeamEnabled = useIsVeeamVBROnly();
src/react/Routes.test.tsx:6
- The
debug
import fromjest-preview
is not used in this test file. You can remove it to avoid unused imports.
import { debug } from 'jest-preview';
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
/approve |
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ARTESCA-15276. Goodbye jeanmarcmilletscality. The following options are set: approve |
No description provided.