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

"Create Product" page #2037

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions portafly/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,14 @@ import { LastLocationProvider } from 'react-router-last-location'
const OverviewPage = React.lazy(() => import('components/pages/Overview'))
const ApplicationsPage = React.lazy(() => import('components/pages/applications/ApplicationsIndexPage'))
const AccountsIndexPage = React.lazy(() => import('components/pages/accounts/AccountsIndexPage'))
const CreateProductPage = React.lazy(() => import('components/pages/product/CreateProductPage'))

const PagesSwitch = () => (
<SwitchWith404>
<LazyRoute path="/" exact render={() => <OverviewPage />} />
<LazyRoute path="/applications" exact render={() => <ApplicationsPage />} />
<LazyRoute path="/accounts" exact render={() => <AccountsIndexPage />} />
<LazyRoute path="/products/new" exact render={() => <CreateProductPage />} />
<Redirect path="/overview" to="/" exact />
</SwitchWith404>
)
Expand Down
4 changes: 4 additions & 0 deletions portafly/src/components/pages/product/CreateProduct.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.system-name-label-text {
display: inline;
margin-right: 5px;
}
197 changes: 197 additions & 0 deletions portafly/src/components/pages/product/CreateProductPage.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
import React, {
useState,
FormEventHandler,
FocusEventHandler,
useEffect
} from 'react'

import {
PageSection,
TextContent,
Text,
Card,
CardBody,
Form,
FormGroup,
TextInput,
TextArea,
ActionGroup,
Button
} from '@patternfly/react-core'
import { useTranslation } from 'i18n/useTranslation'
import { useHistory, Redirect } from 'react-router'
import { createProduct, NewProduct } from 'dal/products'
import { useAsync } from 'react-async'
import { useAlertsContext, useDocumentTitle } from 'components/util'
import { ValidationException, Validator } from 'utils'

type Validations = Record<string, {
validation: 'default' | 'success' | 'error',
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can simplify the API with something like:

Suggested change
validation: 'default' | 'success' | 'error',
validation?: boolean,

thus, if undefined means is not yet validated, and the boolean will define if it was success or error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's given by PF I'm afraid https://www.patternfly.org/v4/documentation/react/components/textinput#props
(minus warning because apparently we don't want to use it)

errors?: string[]
}>

const CreateProductPage = () => {
const { t } = useTranslation('product')
useDocumentTitle(t('create.pagetitle'))
const { goBack } = useHistory()
const { addAlert } = useAlertsContext()

const [name, setName] = useState('')
const [systemName, setSystemName] = useState('')
const [description, setDescription] = useState('')
const [validations, setValidations] = useState<Validations>({
name: { validation: 'default' },
system_name: { validation: 'default' },
description: { validation: 'default' }
})

const {
isPending, error, run, data
} = useAsync({ deferFn: createProduct })

useEffect(() => {
if (error) {
if (Object.prototype.hasOwnProperty.call(error, 'validationErrors')) {
const { validationErrors } = (error as unknown as ValidationException)
const newValidations: Validations = {}
Object.keys(validationErrors).forEach((id) => {
newValidations[id] = {
validation: 'error',
errors: validationErrors[id]
}
})
setValidations({ ...validations, ...newValidations })
} else {
addAlert({ id: String(Date.now()), title: error.message, variant: 'danger' })
}
}
Comment on lines +53 to +67
Copy link
Member

Choose a reason for hiding this comment

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

So validationErrors are not provided in the data object, but within error and not always. We might want to leverage this inside the http module. Then, would be easy to check that if erroris true, data will be validationErrors. Since the parsing of this can't be done without modifying useAsync

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, error can be anything, including a validation error we're gonna need to check for it either way. Consider as if Object.prototype.hasOwnProperty.call(error, 'validationErrors') were typeof error === 'ValidationException'...
There's no way to work around this. It's either check the error for validationErrors or the data, but I think it belongs to error.

}, [error])

if (data) {
const { service } = data as NewProduct
return <Redirect to={`/products/${service.id}`} />
}

const isValid = validations.name.validation === 'success' && validations.system_name.validation !== 'error'

const validator = Validator()
.for('name', () => (name.length > 0 ? 'success' : 'error'))
// eslint-disable-next-line no-nested-ternary
.for('system_name', () => (!systemName ? 'default' : (systemName.length > 0 ? 'success' : 'error')))
.for('description', () => (description.length > 0 ? 'success' : 'default'))
Comment on lines +77 to +81
Copy link
Member

Choose a reason for hiding this comment

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

I like it more than the switch case version, good!

I was thinking that maybe we can pass the Validator factory a validation rules object like:

Suggested change
const validator = Validator()
.for('name', () => (name.length > 0 ? 'success' : 'error'))
// eslint-disable-next-line no-nested-ternary
.for('system_name', () => (!systemName ? 'default' : (systemName.length > 0 ? 'success' : 'error')))
.for('description', () => (description.length > 0 ? 'success' : 'default'))
const validator = Validator({
name: () => (name.length > 0 ? 'success' : 'error'),
'system_name': () => (!systemName ? 'default' : (systemName.length > 0 ? 'success' : 'error')),
description: () => (description.length > 0 ? 'success' : 'default'))
})

then in the implementation you could assert the case with Object.prototype.hasOwnProperty and execute the fn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But isn't that how it works already? Only with a sugar coating on top of it https://github.com/3scale/porta/pull/2037/files#diff-9d8e17e1d9797344cd149b6d58dc6377


const onBlur: FocusEventHandler = (ev) => {
const { name: inputName } = ev.currentTarget as HTMLInputElement

const newValidations = { ...validations }

newValidations[inputName] = {
validation: validator.validate(inputName)
}

setValidations(newValidations)
}

const onSubmit: FormEventHandler = (ev) => {
ev.preventDefault()
const formData = new FormData(ev.currentTarget as HTMLFormElement)
run(formData)
}

return (
<>
<PageSection variant="light">
<TextContent>
<Text component="h1">{t('create.bodytitle')}</Text>
</TextContent>
</PageSection>

<PageSection>
<Card>
<CardBody>
<Form onSubmit={onSubmit}>
<FormGroup
aria-labelledby="name"
label={t('create.name')}
fieldId="name"
helperTextInvalid={validations.name.errors?.flat()}
validated={validations.name.validation}
isRequired
>
<TextInput
validated={validations.name.validation}
id="name"
type="text"
name="name"
value={name}
onChange={setName}
onBlur={onBlur}
isRequired
/>
</FormGroup>

<FormGroup
aria-labelledby="system_name"
label={t('create.system_name.label')}
fieldId="system_name"
helperText={t('create.system_name.helper')}
helperTextInvalid={validations.system_name.errors?.flat()}
validated={validations.system_name.validation}
>
<TextInput
validated={validations.system_name.validation}
id="system_name"
type="text"
name="system_name"
value={systemName}
onChange={setSystemName}
onBlur={onBlur}
placeholder={t('create.system_name.placeholder')}
/>
</FormGroup>

<FormGroup
aria-labelledby="description"
label={t('create.description')}
fieldId="description"
helperTextInvalid={validations.description.errors?.flat()}
validated={validations.description.validation}
>
<TextArea
validated={validations.description.validation}
id="description"
name="description"
value={description}
onChange={setDescription}
onBlur={onBlur}
/>
</FormGroup>

<ActionGroup>
<Button
aria-label={t('shared:shared_elements.create_button')}
type="submit"
isDisabled={isPending || !isValid}
variant="primary"
>
{t('shared:shared_elements.create_button')}
</Button>
<Button
aria-label={t('shared:shared_elements.cancel_button')}
onClick={goBack}
variant="link"
>
{t('shared:shared_elements.cancel_button')}
</Button>
</ActionGroup>
</Form>
</CardBody>
</Card>
</PageSection>
</>
)
}

// Default export needed for React.lazy
// eslint-disable-next-line import/no-default-export
export default CreateProductPage
1 change: 1 addition & 0 deletions portafly/src/components/pages/product/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default as CreateProductPage } from './CreateProductPage'
47 changes: 47 additions & 0 deletions portafly/src/dal/products/createProduct.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import { craftRequest, postData } from 'utils'
import { DeferFn } from 'react-async'

type CreateProductParams = {
name: string
description?: string
deployment_option?: string
backend_version?: string
system_name?: string
}

export interface NewProduct {
service: {
id: number,
name: string,
state: string,
system_name: string,
backend_version: string,
deployment_option: string,
support_email: string,
description: string,
intentions_required: boolean,
buyers_manage_apps: boolean,
buyers_manage_keys: boolean,
referrer_filters_required: boolean,
custom_keys_enabled: boolean,
buyer_key_regenerate_enabled: boolean,
mandatory_app_key: boolean,
buyer_can_select_plan: boolean,
buyer_plan_change_permission: string,
created_at: string,
updated_at: string,
links: Array<{ rel: string, href: string }>
}
}

export type CreateProductValidationErrors = {
name?: string[],
system_name?: string[]
}

const createProduct: DeferFn<NewProduct> = async ([data]) => {
const request = craftRequest('/admin/api/services.json')
return postData(request, data)
}

export { createProduct }
1 change: 1 addition & 0 deletions portafly/src/dal/products/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './createProduct'
3 changes: 3 additions & 0 deletions portafly/src/i18n/locales/en/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import accountSettingsUsersListing from 'i18n/locales/en/account_settings/users/
import analyticsUsage from 'i18n/locales/en/analytics/usage.yml'
// Integration
import integrationConfiguration from 'i18n/locales/en/integration/configuration.yml'
// Products
import product from 'i18n/locales/en/product.yml'

const accounts = {
personal: {
Expand Down Expand Up @@ -48,5 +50,6 @@ export {
analytics,
accounts,
integration,
product,
login
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
import React from 'react'

import { fireEvent, waitFor } from '@testing-library/react'
import { render } from 'tests/custom-render'
import { CreateProductPage } from 'components/pages/product'
import { useAsync, AsyncState } from 'react-async'
import { IProduct } from 'types'
import { useAlertsContext } from 'components/util/AlertsContext'

jest.mock('react-async')
jest.mock('components/util/AlertsContext')

const addAlert = jest.fn();
(useAlertsContext as jest.Mock).mockReturnValue({ addAlert })

const setup = (asyncState: Partial<AsyncState<IProduct[]>>) => {
(useAsync as jest.Mock).mockReturnValue(asyncState)
const wrapper = render(<CreateProductPage />)
const inputs = {
nameInput: wrapper.getByRole('textbox', { name: /create.name/ }),
systemNameInput: wrapper.getByRole('textbox', { name: /create.system_name.label/ }),
createButton: wrapper.getByRole('button', { name: 'shared:shared_elements.create_button' }),
cancelButton: wrapper.getByRole('button', { name: 'shared:shared_elements.cancel_button' })
}

return { ...wrapper, ...inputs }
}

it('button is disabled as long as request is pending', () => {
const { createButton } = setup({ isPending: true })
})

it('should render an alert if there is an error', async () => {
const error = {
name: 'SomeError',
message: 'ERROR'
}
setup({ error })

expect(addAlert).toHaveBeenCalledWith(expect.objectContaining({
title: error.message
}))
})

it('should render inline errors', async () => {
const error = {
name: '',
message: '',
validationErrors: {
name: ['Invalid name', 'duplicated name'],
system_name: ['Invalid system name']
}
}
const { getByText } = setup({ error })

await waitFor(() => expect(getByText(/Invalid name/)).toBeInTheDocument())
expect(getByText(/duplicated name/)).toBeInTheDocument()
expect(getByText(/Invalid system name/)).toBeInTheDocument()
})

it('button is disabled as long as it is invalid', () => {
const { createButton, nameInput, systemNameInput } = setup({ isPending: false })
expect(createButton.getAttribute('disabled')).not.toBeNull()

// Only name is good
fireEvent.change(nameInput, { target: { value: 'My API' } })
fireEvent.blur(nameInput)
expect(createButton.getAttribute('disabled')).toBeNull()

// Both name and systemName is good
fireEvent.change(systemNameInput, { target: { value: 'my-api' } })
fireEvent.blur(systemNameInput)
expect(createButton.getAttribute('disabled')).toBeNull()

// No name, no good
fireEvent.change(nameInput, { target: { value: '' } })
fireEvent.blur(nameInput)
expect(createButton.getAttribute('disabled')).not.toBeNull()
})
18 changes: 18 additions & 0 deletions portafly/src/tests/utils/validator.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { Validator } from 'utils'

const validInput = 'name'
const invalidInput = 'wrong'
const inexistentInput = 'nope'

const validator = Validator()
.for(validInput, () => 'success')
.for(invalidInput, () => 'error')

it('should return default when validation does not exist', () => {
expect(validator.validate(inexistentInput)).toBe('default')
})

it('should validate', () => {
expect(validator.validate(validInput)).toBe('success')
expect(validator.validate(invalidInput)).toBe('error')
})
4 changes: 4 additions & 0 deletions portafly/src/types/product.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export interface IProduct {
id: number
name: string
}
1 change: 1 addition & 0 deletions portafly/src/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './auth'
export * from './http'
export * from './reducers'
export * from './validator'