-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[backend/frontend] Add Import from XTM Hub button in Custom Dashboards #10100
Conversation
@@ -123,6 +127,17 @@ const WorkspaceCreation = ({ paginationOptions, type }) => { | |||
|
|||
const createDashboardButton = FAB_REPLACED ? (props) => ( | |||
<> | |||
<GradientButton |
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.
Personally I find it better to always have this button and no speeddial
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.
Hmm.
Does that mean we're going to change the UI all the time?
I admit that I don't really know how feature flags are managed.
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.
I changed the behavior to always show the button on top of the datatable
withShadow?: boolean; | ||
} | ||
|
||
const GradientButton = styled(Button, { |
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.
It's better to have a Button with style props I think. You could check with Landry
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.
This is the regular way to customize MUI button.
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.
@lndrtrbn any thoughts on this ?
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.
We have decided in OCTI to not use this because even if it is categorized in their doc as the way to do Reusable component
, this syntax is horrible to read.
We can achieve exactly the same (i.e. creating reusable component) with the first bloc of the documentation you shared using sx props
which is better in terms of readability (imo)
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.
Oki doki. I'll change that
lightModeEndColor?: string; | ||
darkModeStartColor?: string; | ||
darkModeEndColor?: string; | ||
withShadow?: boolean; |
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.
I made it for the example in Filigran-UI, we need to choose only one in OCTI and remove that boolean
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.
Done
@@ -337,6 +335,15 @@ const TopBarComponent: FunctionComponent<TopBarProps> = ({ | |||
<Box sx={{ width: '300px', padding: '15px', textAlign: 'center' }}> | |||
<div className={classes.subtitle}>{t_i18n('Filigran eXtended Threat Management')}</div> | |||
<Grid container={true} spacing={3}> | |||
<Grid item xs={12}> | |||
<Tooltip title="XTM Hub"> | |||
<a className={classes.xtmItem} href="https://xtmhub.filigran.io" target="_blank" rel="noreferrer" onClick={handleCloseXtm}> |
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.
hardcoded link ?
not from config ?
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.
Nice catch 😁
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10100 +/- ##
==========================================
+ Coverage 64.30% 64.35% +0.04%
==========================================
Files 656 656
Lines 63046 63086 +40
Branches 6988 7038 +50
==========================================
+ Hits 40541 40597 +56
+ Misses 22505 22489 -16 ☔ View full report in Codecov by Sentry. |
}) => { | ||
const theme = useTheme(); | ||
const isDarkMode = theme.palette.mode === 'dark'; | ||
const startColor = isDarkMode ? darkModeStartColor : lightModeStartColor; |
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.
You should add them in the theme (ThemeDark.ts and ThemeLight.ts) and remove all the props.
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.
Done.
I created a single xtmhub
gradientVariant on the gradientButton
.
In the future we could add some other variants.
@@ -102,6 +106,10 @@ const Workspaces: FunctionComponent<WorkspacesProps> = ({ | |||
const { isFeatureEnable } = useHelper(); | |||
const FAB_REPLACED = isFeatureEnable('FAB_REPLACEMENT'); | |||
|
|||
const theme = useTheme(); | |||
const { settings } = useContext(UserContext); | |||
const importFromHubUrl = isNotEmptyField(settings) ? `${settings.platform_xtmhub_url}/redirect/custom_dashboard?octi_instance_id=${settings.id}`.replaceAll('//', '/') : ''; |
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.
We should decide what to do if the url si not present. Display the button with no href ? Hide the button ? I can't recall what we have decided regarded airgapped platform (that could have a conf with no xtmhub url)
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.
I added this because settings
can be undefined
during the loading time.
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.
Anyway I can only show the button if the value is not empty
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.
Done. The button is hidden if url is empty.
@@ -188,7 +214,8 @@ | |||
"openerm_url": "", | |||
"openrm_token": "", | |||
"openmtd_url": "", | |||
"openmtd_token": "" | |||
"openmtd_token": "", | |||
"xtmhub_url": "https://xtmhub.filigran.io" |
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.
Do we want it easily removable ? I can't recall what we discussed
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.
This value is more to be able to align apps between different environments.
i.e. locahost:3000
> localhost:3001
or my-octi-instance.prod.fr
> xtmhub.filigran.io
> | ||
{t_i18n('Import dashboard')} | ||
<FileUploadOutlined /> |
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.
We need to check that with Product team. @romain-filigran
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.
This is what was requested in Ellyn's brief => #10027
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.
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.
@Kedae : Seen with JB. Ok with a FileUpload icon button once the FAB replacement is merged.
34084c2
to
aaa69d1
Compare
Proposed changes
xtm:xtmhub_url
config valueSettings
GraphQL type to addplatform_xtmhub_url
WorkspaceCreation
component to add the Import from Hub button opening a new tab with a forged URL based onplatform_xtmhub_url
root URI.Related issues
Checklist
Further comments