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

Fix Discourse webhooks #1405

Merged
merged 2 commits into from
Aug 31, 2023
Merged

Fix Discourse webhooks #1405

merged 2 commits into from
Aug 31, 2023

Conversation

garrrikkotua
Copy link
Contributor

@garrrikkotua garrrikkotua commented Aug 31, 2023

Changes proposed ✍️

What

🤖 Generated by Copilot at a53e0c5

Improved the webhook integration with Discourse by using a new database query method and enhancing the error handling and logging in discourse.ts and integrationRepository.ts.

🤖 Generated by Copilot at a53e0c5

To handle the Discourse events
We refactored the webhook contents
We query by platform and tenant
With a new method that's elegant
And improved the error and logging segments

Why

How

🤖 Generated by Copilot at a53e0c5

  • Extract logic to find active integration by platform and tenant ID to a new method in IntegrationRepository (link, link, link)
  • Handle errors when finding the integration and respond with a 200 status and a log message in discourse.ts (link)
  • Remove unused import of TenantRepository from discourse.ts (link)
  • Change log message to include tenant ID instead of tenant object in discourse.ts (link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

where: {
platform,
tenantId,
deletedAt: null,
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK if we have integration sequelize model set to paranoid you don't have to specify deletedAt: null because it's automatic:
https://sequelize.org/docs/v6/core-concepts/paranoid/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, didn't know

@garrrikkotua garrrikkotua merged commit 1336bb7 into main Aug 31, 2023
8 checks passed
@garrrikkotua garrrikkotua deleted the fix/discourse-webhooks branch August 31, 2023 11:43
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.

None yet

2 participants