#8067: fix unresolved schemas in integration configuration validation #8212
Conversation
| @@ -67,4 +69,35 @@ describe("IntegrationConfigEditorModal", () => { | |||
| ).toBeVisible(); | |||
| } | |||
| }); | |||
|
|
|||
| test("displays user-friendly pattern validation message", async () => { | |||
There was a problem hiding this comment.
Restored this test because now we're resolving the schema correctly. This should've been a sign to me that something was wrong, but I didn't know how to replicate the problem in the UI.
| ); | ||
|
|
||
| const { errors } = validator.validate(values); | ||
| const { errors } = await resolveSchemaAndValidate(schema, values); |
There was a problem hiding this comment.
This is the actual fix
src/validators/schemaValidator.ts
Outdated
| @@ -249,7 +249,7 @@ export async function dereference( | |||
| * | |||
| * To avoid secret leakage, does not validate the secret `$ref`s of properties accepting an integration configuration. | |||
| */ | |||
| export async function validateBrickInputOutput( | |||
| export async function resolveSchemaAndValidate( | |||
There was a problem hiding this comment.
Open to other names, but this function can resolve and schema and validate the values passed in, not just bricks
There was a problem hiding this comment.
@grahamlangford I don't think this is the correct method to use for validating configuration?
This method has very particular behavior around how integrations are resolved: https://github.com/pixiebrix/pixiebrix-extension/pull/8212/files#diff-59aee84744090336263733bc3d1d8f1da1945e4e3728dcf9052ae3e093112890R276
I would recommend we make a "validateIntegrationConfiguration" method that correctly passes passes sanitize: false. (We'll also want to use this method in the Admin Console for validation)
There was a problem hiding this comment.
On second look, I think you're right. I need to dig deeper into how the $RefParser works.
There was a problem hiding this comment.
@BLoe and I both have context on this -- I'd recommend pairing with one of us vs. starting the investigation from scratch
There was a problem hiding this comment.
@twschiller those failures were actually due to a change I made in the previous PR. Agree on wanting to be able to set sanitize: false. I would appreciate a quick sync later though.
| @@ -204,7 +204,7 @@ const builtInSchemaResolver: ResolverOptions = { | |||
| * Should generally be set to true when using for runtime validation, but false when using for UI entry validation. | |||
| * @see $RefParser.dereference | |||
| */ | |||
| export async function dereference( | |||
| export async function dereferenceForYup( | |||
| const dereferenced = await dereference( | ||
| integrationWithOptionalField as Schema, | ||
| { | ||
| sanitizeIntegrationDefinitions: false, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
No need to actually dereference and ref resolution is now handled in the modal
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
| @@ -52,7 +52,7 @@ export class LocalIntegrationsPage { | |||
| .getByPlaceholder("Start typing to find results") | |||
| .fill(integrationName); | |||
|
|
|||
| await this.page.getByText(integrationName).hover(); | |||
| await this.page.getByText(integrationName).first().hover(); | |||
There was a problem hiding this comment.
NIT: why is first necessary? Seems like it appears 2x on the page?
There was a problem hiding this comment.
Depending on which integration, the text can appear several times. Ran into an issue with the Open AI test.
There was a problem hiding this comment.
Clarifying which screen affordance is being hovered in that case would be beneficial
twschiller
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8212 +/- ##
==========================================
- Coverage 73.42% 73.42% -0.01%
==========================================
Files 1319 1319
Lines 40825 40832 +7
Branches 7578 7578
==========================================
+ Hits 29976 29980 +4
- Misses 10849 10852 +3 ☔ View full report in Codecov by Sentry. |
What does this PR do?
Reviewer Tips
Checklist