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

Migrate Discourse integration to the new framework #1778

Merged
merged 13 commits into from
Nov 2, 2023

Conversation

garrrikkotua
Copy link
Contributor

@garrrikkotua garrrikkotua commented Oct 26, 2023

Changes proposed ✍️

What

🤖 Generated by Copilot at 35081c1

Refactored the discourse integration logic to use the new service SQS queue and the integrations library. Removed the old node worker SQS queue and the DiscourseIntegrationService class. Added new functions and files to handle the discourse API calls, stream generation, data processing, and webhook processing. Commented out the discourse webhook endpoint temporarily.

🤖 Generated by Copilot at 35081c1

We are the masters of discourse
We integrate with force and skill
We use the service queue and library
We comment out the route until

Why

How

🤖 Generated by Copilot at 35081c1

  • Refactor the webhook processing logic to use a new service SQS queue instead of the node worker SQS queue (link, link, link, link)
  • Remove the discourse integration service and the integration run creation logic as they are no longer needed after the refactoring (link, link, link, link)
  • Implement the integrations library for the discourse integration, which defines the type, member attributes, generate streams, process stream, process data, and post process functions for the integration (link)
  • Implement the functions to fetch the categories, topics, posts, and user from the discourse forum using the discourse API, which are used by the integrations library to process the data for the integration (link, link, link, link, link)
  • Implement the handler functions to generate the streams, process the streams, process the data, and process the webhook streams for the discourse integration, which are used by the integrations library to initiate and execute the integration logic (link, link, link, link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screenshots 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.

@peoray
Copy link
Contributor

peoray commented Oct 26, 2023

Great work.

Quick question: When will the docs be updated so anyone can add new integrations?

cc @garrrikkotua @epipav

@epipav epipav self-assigned this Oct 30, 2023
@epipav epipav added the Bug Created by Linear-GitHub Sync label Oct 30, 2023
@garrrikkotua garrrikkotua added Improvement Created by Linear-GitHub Sync and removed Bug Created by Linear-GitHub Sync labels Oct 30, 2023
Copy link
Collaborator

@epipav epipav left a comment

Choose a reason for hiding this comment

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

I added a few comments

} from '../types'
import { IProcessStreamContext } from '../../../types'

const serializeArrayToQueryString = (params: object) =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: we can call this serializeObjecttoQueryString instead, what do u think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +11 to +14
ctx.log.info({
message: 'Fetching categories from Discourse',
forumHostName: params.forumHostname,
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can update this logging, it's same as getCategories

// wait 5 mins
throw new RateLimitError(5 * 60, 'discourse/gettopics')
}
ctx.log.error({ err, params }, 'Error while getting Discourse categories')
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's update this also

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +10 to +11
// this methods returns ids of posts in a topic
// then we need to parse each topic individually (can be batched)
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's either delete or update the comment, it's from getPostsFromTopic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)
})

// we aslo need to trigger nextPageStream
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@garrrikkotua garrrikkotua merged commit ccc1ca4 into main Nov 2, 2023
8 checks passed
@garrrikkotua garrrikkotua deleted the improve/migrate-discourse branch November 2, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement Created by Linear-GitHub Sync
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants