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

Conversation

josemigallas
Copy link
Contributor

@josemigallas josemigallas commented Jul 8, 2020

What this PR does / why we need it:

Adds a page component for Products > New.

Screen Shot 2020-08-12 at 13 14 15

Screen Shot 2020-07-29 at 12 50 04

Screen Shot 2020-07-29 at 12 49 10 1

Which issue(s) this PR fixes
THREESCALE-5636: Component for "Create product" page

Verification steps

Go to /products/new

  1. Check validation works as expected (onblur effects, etc)
  2. Submit wrong data and check errors are shown (no product is created)
  3. Submit correct data and check product is created in porta (also it should navigate to products/:id)

@josemigallas josemigallas added the portafly Issues related to the PortaFly module label Jul 8, 2020
@josemigallas josemigallas self-assigned this Jul 8, 2020
@josemigallas josemigallas force-pushed the portafly/product_create_page branch 2 times, most recently from b426c57 to 01da96f Compare July 14, 2020 10:51
@josemigallas josemigallas changed the title Create Product page for Portafly "Create Product" page Jul 14, 2020
Comment on lines +50 to +67
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' })
}
}
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.

@josemigallas josemigallas marked this pull request as ready for review July 29, 2020 10:46
@josemigallas josemigallas force-pushed the portafly/product_create_page branch 3 times, most recently from 53285d2 to 11e4b7d Compare August 11, 2020 12:03
This was referenced Aug 11, 2020
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)

Comment on lines +77 to +81
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'))
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

@hallelujah hallelujah deleted the portafly/product_create_page branch September 2, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
portafly Issues related to the PortaFly module
Projects
None yet
2 participants