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

Update zapier trigger payload #8464

Merged
merged 3 commits into from
Nov 12, 2024
Merged

Update zapier trigger payload #8464

merged 3 commits into from
Nov 12, 2024

Conversation

martmull
Copy link
Contributor

  • fixes zapier tests

@martmull martmull marked this pull request as ready for review November 12, 2024 15:40
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR updates the Zapier integration's trigger payload structure and improves JWT authentication handling for better consistency and reliability.

  • Changed webhook operation format from create.company to company.created in /packages/twenty-zapier/src/test/triggers/trigger_record.test.ts
  • Added new getOperationFromDatabaseEventAction function in /packages/twenty-zapier/src/creates/crud_record.ts to properly map database actions to GraphQL mutations
  • Modified trigger payload structure to consistently wrap data under record property across all operations
  • Improved JWT validation in /packages/twenty-server/src/engine/core-modules/jwt/services/jwt-wrapper.service.ts by adding payload existence check
  • Standardized error handling by using AuthException consistently for JWT validation failures

7 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 233 to 235
expect(firstCompany).toBeDefined();
expect(firstCompany.id).toBeDefined();
expect(firstCompany.record.id).toBeDefined();
expect(Object.keys(firstCompany).length).toEqual(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Inconsistent object structure check - other tests check firstCompany.record but this one checks firstCompany directly before checking firstCompany.record.id

Comment on lines +165 to +166
expect(firstCompany.record).toBeDefined();
expect(firstCompany.updatedFields).toBeDefined();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Test only checks that updatedFields exists but not its content or structure. Consider adding more specific assertions

@@ -1,7 +1,5 @@
export type InputData = { [x: string]: any };
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using a more specific type than { [x: string]: any } for InputData to improve type safety

Comment on lines +93 to +95
updatedFields: [
Object.keys(result).filter((key) => key !== 'id')?.[0],
] || ['updatedField'],
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: This logic will only ever return the first non-id field as the updated field, which may not be accurate if multiple fields were actually updated

@martmull martmull merged commit 269eaf4 into main Nov 12, 2024
19 checks passed
@martmull martmull deleted the update-zapier-trigger-payload branch November 12, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants