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.

Sign up for GitHub

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

Jump to bottom

Migrate Twitter integration to the new framework #1744

Merged
merged 17 commits into from
Oct 30, 2023
Merged

Migrate Twitter integration to the new framework #1744

merged 17 commits into from
Oct 30, 2023

Conversation

garrrikkotua
Copy link
Contributor

@garrrikkotua garrrikkotua commented Oct 20, 2023 •
edited by ghost

Changes proposed ✍️

What

🤖 Generated by Copilot at 39a2c88

This pull request adds support for the integration with Twitter as a new platform type. It modifies the integration data, run, and stream services and repos to handle the token and refresh token for the integrations. It also adds the platform settings and the integration library files for the Twitter platform, using the existing interface and types.
​

🤖 Generated by Copilot at 39a2c88

Twitter platform
integrations need tokens
refresh them often

Why

How

🤖 Generated by Copilot at 39a2c88

  • Add support for integration with Twitter as a new platform type that uses OAuth 2.0 for authentication
  • Define the platform settings for Twitter, such as the client id, the client secret, and the callback url, in the config files and the TwitterPlatformSettings interface (link, link, link, link)
  • Export the integration descriptor for Twitter, which includes the handler functions for generating, processing, and validating streams and data, in the index.ts file (link)
  • Implement the handler function for generating streams for Twitter, which uses the axios library and the platform settings to refresh the token and the refresh token for the integration, and passes them to the integration service interface (link, link)
  • Add stubs for the handler functions for processing and validating streams and data for Twitter, which will contain the logic for interacting with the Twitter API and transforming the data into the common format (link, link)
  • Add the refresh token for the integration to the data interfaces and the database queries for the integration data, integration run, and integration stream services (link, link, link, link, link, link)
  • Add the methods for updating the token and the refresh token for the integration to the repo classes and the service classes for the integration data, integration run, and integration stream services, and handle any errors or logging (link, link, link, link, link, link)
  • Add the refresh token and the update functions to the objects that are passed to the integration library functions for generating, processing, and validating streams and data, in the integration data, integration run, and integration stream services (link, link, link, link, 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.

epipav
epipav requested changes Oct 26, 2023
View reviewed changes
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.

Included some nitpicks and comments

services/apps/data_sink_worker/src/service/member.service.ts Show resolved Hide resolved
services/libs/integrations/src/integrations/twitter/generateStreams.ts Outdated Show resolved Hide resolved
services/libs/integrations/src/integrations/twitter/generateStreams.ts Outdated Show resolved Hide resolved
services/libs/integrations/src/integrations/twitter/processStream.ts Outdated
Comment on lines 107 to 130
await processPaginated<ReachSelection>(
async (page) => {
const result = await db.any(
`
SELECT
m."memberId" as id,
m.username as username
FROM
"memberIdentities" m
WHERE
m."tenantId"= (select "tenantId" from integrations where id = $(integrationId) )
and m.platform = $(platform)
ORDER BY
m."memberId"
LIMIT $(perPage)
OFFSET $(offset)
`,
{
integrationId: ctx.integration.id,
platform: PlatformType.TWITTER,
perPage,
offset: (page - 1) * perPage,
},
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: let's keep the queries in repo layer - 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.

Yeah, but there is no repo layer in integrations folder - I can put this query just in a separate file inside integration if it makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we at least keep the query in a separate function with a name to what it does it'll be cleaner. Not a strong opinion tho

services/libs/integrations/src/integrations/twitter/processStream.ts Show resolved Hide resolved
@garrrikkotua garrrikkotua merged commit dcba0f9 into main Oct 30, 2023
8 checks passed
@garrrikkotua garrrikkotua deleted the improve/migrate-twitter branch October 30, 2023 15:27
sausage-todd pushed a commit that referenced this pull request Nov 9, 2023
@garrrikkotua @sausage-todd
Migrate Twitter integration to the new framework (#1744)
023d132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@themarolt themarolt themarolt left review comments

@epipav epipav epipav approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@garrrikkotua @epipav @themarolt