-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
web: Form validation regressions, consistency fixes #15894
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
Conversation
✅ Deploy Preview for authentik-docs canceled.
|
✅ Deploy Preview for authentik-integrations canceled.
|
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15894 +/- ##
==========================================
- Coverage 92.83% 92.76% -0.07%
==========================================
Files 836 836
Lines 45160 45166 +6
==========================================
- Hits 41924 41900 -24
- Misses 3236 3266 +30
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
authentik PR Installation instructions Instructions for docker-composeAdd the following block to your AUTHENTIK_IMAGE=ghcr.io/goauthentik/dev-server
AUTHENTIK_TAG=gh-31db063e479c1c0ed00fef1f8d9dd22dc8808117
AUTHENTIK_OUTPOSTS__CONTAINER_IMAGE_BASE=ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s Afterwards, run the upgrade commands from the latest release notes. Instructions for KubernetesAdd the following block to your authentik:
outposts:
container_image_base: ghcr.io/goauthentik/dev-%(type)s:gh-%(build_hash)s
global:
image:
repository: ghcr.io/goauthentik/dev-server
tag: gh-31db063e479c1c0ed00fef1f8d9dd22dc8808117 Afterwards, run the upgrade commands from the latest release notes. |
9f9e667
to
27a772a
Compare
6cd91f9
to
fa9e7fc
Compare
a5958a8
to
89b9496
Compare
fa9e7fc
to
bb636e7
Compare
89b9496
to
989c152
Compare
921644b
to
9ca6463
Compare
989c152
to
d2994a6
Compare
50deedb
to
7ab6db0
Compare
"@web/test-runner": "^0.20.2" | ||
"@web/test-runner": "^0.20.2", | ||
"chromedriver": "^139.0.0" |
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.
WebDriver is still plagued with download issues, but this inclusion will let the legacy e2e test suite run when Chromium is manually installed.
} | ||
|
||
/** | ||
* @todo This defaults to true when the form is not yet available |
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.
TOOD: Circle back on this after #16010 gives us a reusable pattern with form internals
if (!this.form) { | ||
throw new TypeError("Form reference is not set"); | ||
} |
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.
Unlikely but this has been technically possible with all uses of @query
public get form(): HTMLFormElement | null { | ||
return this.renderRoot.querySelector("form#applicationform"); | ||
} |
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.
Switch to getter to allow for overriding in application provider steps
@@ -71,7 +72,7 @@ export class ApplicationWizardApplicationStep extends ApplicationWizardStep { | |||
this.errors.set("name", msg("An application name is required")); | |||
} | |||
|
|||
if (!values.metaLaunchUrl || !URL.canParse(values.metaLaunchUrl)) { | |||
if (values.metaLaunchUrl && !URL.canParse(values.metaLaunchUrl)) { |
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.
TODO: This was subtle fix. Circle back on something like an <ak-url-input>
to handle this sort of check.
const providerForm = this.element.form; | ||
|
||
if (!providerForm) { | ||
// TODO: This needs to be removed once all steps can report their validity. |
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.
AFAICT this is only true of the bindings step which has no form. Leaving it in for now to reduce the number of changes.
@@ -16,7 +16,7 @@ import { snakeCase } from "change-case"; | |||
import { CSSResult } from "lit"; | |||
import { property, query } from "lit/decorators.js"; | |||
|
|||
export class ApplicationWizardProviderForm<T extends OneOfProvider> extends AKElement { | |||
export abstract class ApplicationWizardProviderForm<T extends OneOfProvider> extends AKElement { |
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.
ApplicationWizardStep
should also be abstract, but our typing on CustomEmitterElement
makes this difficult to achieve without a lot of detangling.
errors, | ||
errors.provider ?? {}, |
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.
AFAICT not showing field errors in the provider step seems to be a long standing bug we managed to surface when error parsing was introduced.
I suspect that ak-application-wizard-submit-step
also has an expectation of field error name spacing that doesn't apply anymore.
web/src/common/styles/authentik.css
Outdated
.pf-c-form__group { | ||
--pf-c-form--m-horizontal__group-label--md--GridColumnWidth: minmax(max-content, 9.375rem); | ||
column-gap: var(--pf-global--spacer--md); | ||
} | ||
|
||
.pf-c-form__group-label { | ||
display: flex; | ||
user-select: none; | ||
padding-top: var(--pf-c-form--m-horizontal__group-label--md--PaddingTop); | ||
|
||
/* Increase the pressable area of the label. */ | ||
.pf-c-form__label { | ||
display: block; | ||
flex: 1 1 auto; | ||
} | ||
} | ||
|
||
.pf-c-form__label[aria-required] .pf-c-form__label-text::after { | ||
content: "*"; | ||
user-select: none; | ||
margin-left: var(--pf-c-form__label-required--MarginLeft); | ||
font-size: var(--pf-c-form__label-required--FontSize); | ||
color: var(--pf-c-form__label-required--Color); | ||
} | ||
|
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.
Largely moved out of horizontal form for better consistency in inputs across all interfaces.
required?: boolean; | ||
} | ||
|
||
export const AKLabel: LitFC<FormLabelProps> = ( |
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.
See #16119 for how this works with label input association.
37a52d6
to
144d377
Compare
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
1acb244
to
9a37950
Compare
… editing existing entities. - We need to make the component have a better understanding of this concept.
9a37950
to
d01b0b9
Compare
Details
This PR resolves a collection of form validation issues, as well as a partial fix for our WebDriverIO tests to run while Playwright is in review.
Fixed Issues
ErrorProp
s containing strings are now parsed correctly instead of defaulting to a generic network errorform
element getter within AKForm now usesrenderRoot
overshadowRoot
, fixing selector issues when in compatibility mode.See also