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

[Merged by Bors] - fix: update voiceflow dtos (PL-844) #743

Closed
wants to merge 2 commits into from

Conversation

DecathectZero
Copy link
Member

@DecathectZero DecathectZero commented Apr 3, 2024

This PR is blocked by:
https://github.com/voiceflow/creator-app/pull/7928

Is this a pure PR? Nah, I cheated a bit and tossed in classification DTOs here:

export const TestClassificationRequestBodyDTO = z
.object({
projectID: z.string().refine((id) => ObjectId.isValid(id), {
message: 'projectID must be a valid ObjectId',
}),
versionID: z.string().refine((id) => ObjectId.isValid(id), {
message: 'versionID must be a valid ObjectId',
}),
utterance: z.string(),
intentClassificationSettings: IntentClassificationSettingsDTO,
})
.strict();
export type TestClassificationRequestBodyDTO = z.infer<typeof TestClassificationRequestBodyDTO>;
export const ClassificationDTO = z.object({
intents: z.array(
z.object({
name: z.string(),
confidence: z.number(),
})
),
});
export const TestClassificationResponseDTO = z
.object({
nlu: ClassificationDTO,
llm: ClassificationDTO,
utterance: z.string(),
})
.strict();
export type TestClassificationResponse = z.infer<typeof TestClassificationResponseDTO>;

If I wanted to abide by even better prinicples, ^ that should be a separate PR.

derived verbatim from https://github.com/voiceflow/general-runtime/pull/736/files

But I made a critical bug fix that would've broken our functions testing:
Screenshot 2024-04-02 at 10 45 39 PM

This code is approved and validated from my end.

Copy link

linear bot commented Apr 3, 2024

@@ -12,14 +13,14 @@ export const TestFunctionRequestBodyDTO = z
inputVars: z.record(
z
.object({
type: z.literal(FunctionVariableType.STRING),
type: z.literal(VariableDatatype.TEXT),
Copy link
Member Author

@DecathectZero DecathectZero Apr 3, 2024

Choose a reason for hiding this comment

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

so the actual raw values here are different, "string" would break if we are matching against "text".

To my knowledge, the FE still sends "string"
We need to allow both otherwise the function testing would break:
creator-app request code

try {
await TestFunctionRequestBodyDTO.parseAsync(req.body);
} catch (err) {
throw new BadRequestException({
message: err instanceof z.ZodError ? formatZodError(err) : err.message,
});
}

@DecathectZero DecathectZero force-pushed the tyler/update-dtos/PL-844 branch 3 times, most recently from d67e7b6 to 4d527f7 Compare April 3, 2024 03:56
Copy link

sonarcloud bot commented Apr 3, 2024

@trs trs mentioned this pull request Apr 3, 2024
2 tasks
@DecathectZero
Copy link
Member Author

bors r+

bors-vf bot pushed a commit that referenced this pull request Apr 3, 2024
This PR is blocked by:
voiceflow/creator-app#7928

Is this a pure PR? Nah, I cheated a bit and tossed in classification DTOs here:
https://github.com/voiceflow/general-runtime/blob/2208a027011298632bc1eaa360da115c124f73a0/lib/controllers/test/interface.ts#L47-L79
If I wanted to abide by even better prinicples, ^ that should be a separate PR.

derived verbatim from https://github.com/voiceflow/general-runtime/pull/736/files

But I made a critical bug fix that would've broken our functions testing:
<img width="1695" alt="Screenshot 2024-04-02 at 10 45 39 PM" src="https://github.com/voiceflow/general-runtime/assets/5643574/7b4d0539-4db5-4659-94bd-e473d8f78013">

This code is approved and validated from my end.

Co-authored-by: Tyler Han <tylerhan97@gmail.com>
@bors-vf
Copy link

bors-vf bot commented Apr 3, 2024

@bors-vf bors-vf bot changed the title fix: update voiceflow dtos (PL-844) [Merged by Bors] - fix: update voiceflow dtos (PL-844) Apr 3, 2024
@bors-vf bors-vf bot closed this Apr 3, 2024
@bors-vf bors-vf bot deleted the tyler/update-dtos/PL-844 branch April 3, 2024 16:01
@DecathectZero DecathectZero restored the tyler/update-dtos/PL-844 branch April 3, 2024 16:22
@DecathectZero DecathectZero deleted the tyler/update-dtos/PL-844 branch April 3, 2024 16:23
@DecathectZero DecathectZero restored the tyler/update-dtos/PL-844 branch April 3, 2024 16:34
DecathectZero added a commit to voiceflow/test-history-rebase that referenced this pull request Jun 27, 2024
We don't actually use `FunctionCompiledVariableDeclarationDTO` or any of it's downstream stuff anywhere in creator-app I believe. We use it purely for types.

We should be careful when modifying DTOs

But we should patch this because the moment someone uses it all functions will break unless we run a migration. All functions today still use `type: "string"`

Related PR:
voiceflow/general-runtime#743

Co-authored-by: Tyler Han <tylerhan97@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants