-
Notifications
You must be signed in to change notification settings - Fork 365
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
fix: [M3-7083 & M3-7175] MarketPlace regex fix + end to end coverage #9704
Changes from 12 commits
3e6175f
9b7d47c
e3699d0
68a889f
a4d75c7
fd058c0
99b44a1
30ae9f6
7539107
b1cfef5
97e6665
c6e428c
ba729cc
b27898d
dce1afa
cb8ed19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,16 @@ | ||
import { StackScript, UserDefinedField } from '@linode/api-v4/lib/stackscripts'; | ||
import Grid from '@mui/material/Unstable_Grid2'; | ||
import { styled } from '@mui/material/styles'; | ||
import { decode } from 'he'; | ||
import * as React from 'react'; | ||
|
||
import { Box } from 'src/components/Box'; | ||
import { Chip } from 'src/components/Chip'; | ||
import { Divider } from 'src/components/Divider'; | ||
import { Typography } from 'src/components/Typography'; | ||
import { SelectionCardWrapper } from 'src/features/Linodes/LinodesCreate/SelectionCardWrapper'; | ||
|
||
import { handleAppLabel } from './utilities'; | ||
|
||
interface Props { | ||
apps: StackScript[]; | ||
disabled: boolean; | ||
|
@@ -41,24 +43,15 @@ export const AppPanelSection = (props: Props) => { | |
} | ||
|
||
return ( | ||
<> | ||
<Box data-testid={heading}> | ||
<Typography variant="h2">{heading}</Typography> | ||
{heading && heading.length > 0 ? ( | ||
<Divider spacingBottom={16} spacingTop={16} /> | ||
) : null} | ||
{apps.length > 0 ? ( | ||
<AppPanelGrid container spacing={2}> | ||
{apps.map((eachApp) => { | ||
const decodedLabel = decode(eachApp.label); | ||
const isCluster = | ||
decodedLabel.endsWith('Cluster ') && | ||
eachApp.user_defined_fields.some( | ||
(field) => field.name === 'cluster_size' | ||
); | ||
|
||
const label = isCluster | ||
? decodedLabel.split(' Cluster')[0] | ||
: decodedLabel; | ||
const { decodedLabel, isCluster, label } = handleAppLabel(eachApp); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved this to a util so it's reusable and testable in isolation. |
||
|
||
return ( | ||
<SelectionCardWrapper | ||
|
@@ -86,7 +79,7 @@ export const AppPanelSection = (props: Props) => { | |
{`Sorry, no results matching "${searchValue}" were found.`} | ||
</Typography> | ||
)} | ||
</> | ||
</Box> | ||
); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,7 +68,7 @@ import { validatePassword } from 'src/utilities/validatePassword'; | |
import LinodeCreate from './LinodeCreate'; | ||
import { deriveDefaultLabel } from './deriveDefaultLabel'; | ||
import { HandleSubmit, Info, LinodeCreateValidation, TypeInfo } from './types'; | ||
import { getRegionIDFromLinodeID } from './utilities'; | ||
import { filterOneClickApps, getRegionIDFromLinodeID } from './utilities'; | ||
|
||
import type { | ||
CreateLinodeRequest, | ||
|
@@ -175,13 +175,6 @@ const getDisabledClasses = (regionID: string, regions: Region[] = []) => { | |
return disabledClasses; | ||
}; | ||
|
||
const trimOneClickFromLabel = (script: StackScript) => { | ||
return { | ||
...script, | ||
label: script.label.replace('One-Click', ''), | ||
}; | ||
}; | ||
|
||
const nonImageCreateTypes = ['fromStackScript', 'fromBackup', 'fromLinode']; | ||
|
||
const isNonDefaultImageType = (prevType: string, type: string) => { | ||
|
@@ -194,7 +187,6 @@ class LinodeCreateContainer extends React.PureComponent<CombinedProps, State> { | |
componentDidMount() { | ||
// Allowed apps include the base set of original apps + anything LD tells us to show | ||
const newApps = this.props.flags.oneClickApps || []; | ||
const allowedApps = Object.keys({ ...baseApps, ...newApps }); | ||
if (nonImageCreateTypes.includes(this.props.createType)) { | ||
// If we're navigating directly to e.g. the clone page, don't select an image by default | ||
this.setState({ selectedImageID: undefined }); | ||
|
@@ -204,17 +196,12 @@ class LinodeCreateContainer extends React.PureComponent<CombinedProps, State> { | |
this.props.queryClient | ||
.fetchQuery('stackscripts-oca-all', () => getAllOCAsRequest()) | ||
.then((res: StackScript[]) => { | ||
// Don't display One-Click Helpers to the user | ||
// Filter out any apps that we don't have info for | ||
const filteredApps = res.filter((script) => { | ||
return ( | ||
!script.label.match(/helpers/i) && | ||
allowedApps.includes(String(script.id)) | ||
); | ||
const trimmedApps = filterOneClickApps({ | ||
baseApps, | ||
newApps, | ||
queryResults: res, | ||
}); | ||
const trimmedApps = filteredApps.map((stackscript) => | ||
trimOneClickFromLabel(stackscript) | ||
); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. Moved to a util. This helps testing in isolation and re-use as an assertion in the e2e test. |
||
this.setState({ | ||
appInstances: trimmedApps, | ||
appInstancesLoading: false, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import { UserDefinedField } from '@linode/api-v4/lib/stackscripts'; | ||
import { Theme, styled } from '@mui/material/styles'; | ||
import * as React from 'react'; | ||
import { compose } from 'recompose'; | ||
|
@@ -11,11 +10,15 @@ import { AppPanelSection } from 'src/features/Linodes/LinodesCreate/AppPanelSect | |
import { getQueryParamFromQueryString } from 'src/utilities/queryParams'; | ||
|
||
import { Panel } from './Panel'; | ||
import { AppsData } from './types'; | ||
|
||
import type { AppsData } from './types'; | ||
import type { UserDefinedField } from '@linode/api-v4/lib/stackscripts'; | ||
import type { FlagSet } from 'src/featureFlags'; | ||
|
||
interface Props extends AppsData { | ||
disabled: boolean; | ||
error?: string; | ||
flags: FlagSet; | ||
handleClick: ( | ||
id: number, | ||
label: string, | ||
|
@@ -51,6 +54,7 @@ class SelectAppPanel extends React.PureComponent<Props> { | |
appInstancesLoading, | ||
disabled, | ||
error, | ||
flags, | ||
handleClick, | ||
isFiltering, | ||
isSearching, | ||
|
@@ -81,11 +85,9 @@ class SelectAppPanel extends React.PureComponent<Props> { | |
return null; | ||
} | ||
|
||
const newAppsIds = Object.keys(flags.oneClickApps || {}); | ||
const newApps = appInstances.filter((app) => { | ||
return [ | ||
'hashicorp nomad clients cluster', | ||
'hashicorp nomad cluster', | ||
].includes(app.label.toLowerCase().trim()); | ||
return newAppsIds.includes(app.id.toString()); | ||
}); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why we were hardcoding those since they are already returning from the oneClickApps feature flag. This fix also makes the New Apps selection more robust since it references the Id rather than a label. This is the reason for passing the flags from the Class Component to the selection panel. (also a class component so no Confirmed with @HanaXu this was the right thing to do. |
||
const popularApps = appInstances.slice(0, 10); | ||
|
@@ -98,7 +100,10 @@ class SelectAppPanel extends React.PureComponent<Props> { | |
const isFilteringOrSearching = isFiltering || isSearching; | ||
|
||
return ( | ||
<StyledPaper data-qa-tp="Select Image"> | ||
<StyledPaper | ||
data-qa-tp="Select Image" | ||
data-testid="one-click-apps-container" | ||
> | ||
{error && <Notice text={error} variant="error" />} | ||
{!isFilteringOrSearching ? ( | ||
<AppPanelSection | ||
|
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 was necessary to remove an import warning when importing a utility from a
.tsx
extension