-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
fix: over-fetching data in workflows when getting all workflows for eventtype #16879
Conversation
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
@@ -781,6 +780,63 @@ export const getEventTypeWorkflows = async ( | |||
userId: number, | |||
eventTypeId: number | |||
): Promise<z.infer<typeof ZWorkflows>> => { | |||
const rawEventType = await EventTypeRepository.findById({ id: eventTypeId, userId }); | |||
return rawEventType?.workflows; | |||
const workflows = await prisma.workflow.findMany({ |
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.
@CarinaWolli i tested this and im pretty sure this works as expected - this query has been changed a bit from the one in eventTypes repo.
Targeting the workflow table directly instead of the event types. This generates some nicer SQL for what we needed. Also doesnt over fetch the data outside of eventTypes.workflows
that was un-needed in this call
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.
I think this would be a good opportunity to add as a method in the workflow repository.
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.
I have a separate issue for this! Gonna be extracting a lot of the current workflow repository to a service and handling that migration there. Mind if we pick that up there?
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.
Excited to hear! I'm ok with that.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (09/30/24)1 reviewer was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (09/30/24)1 label was added to this PR based on Keith Williams's automation. |
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.
LGTM
Test cases
Looks like this is only used to fetch active workflows on the event type pages so I tested the following
- Personal workflows render properly on personal event type pages
- Team workflows render properly on team event type pages
- Org level workflows render properly on team event type pages
E2E results are ready! |
What does this PR do?
fixes: CAL-4433
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Test workflows is working as expected
Checklist