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

[Integration][Clickup] Add clickup integration #768

Closed
wants to merge 7 commits into from

Conversation

DeeStarks
Copy link

@DeeStarks DeeStarks commented Jul 3, 2024

Description

What - This PR adds an integration for Clickup
Why - This will allow users to connect their workspaces from Clickup to Port
How - This was integrated using the Ocean framework. Documentation on usage is available here

Type of change

Please leave one option from the following and delete the rest:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • New Integration (non-breaking change which adds a new integration)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Non-breaking change (fix of existing functionality that will not change current behavior)
  • Documentation (added/updated documentation)

Screenshots

image

image

image

image

API Documentation

Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

Left some comments. Please go over them and let me know. Overall, great work

integrations/clickup/.dockerignore Outdated Show resolved Hide resolved
integrations/clickup/CHANGELOG.md Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/utils.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

I have added more comments. Please let me know if they are not clear or you have an alternative idea

integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
@DeeStarks DeeStarks requested a review from PeyGis July 4, 2024 10:37
Copy link
Contributor

@PeyGis PeyGis left a comment

Choose a reason for hiding this comment

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

Added more comments

integrations/clickup/clickup/client.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
integrations/clickup/main.py Outdated Show resolved Hide resolved
endDate: .due_date | tonumber / 1000 | todate
totalIssues: .task_count
relations:
team: .__team_id
Copy link
Contributor

Choose a reason for hiding this comment

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

too specific, should have added an option to see all the team object, this will allow extensibility in the future

properties:
url: .url
status: .status.status
assignee: .assignees | map(.email) | first
Copy link
Contributor

Choose a reason for hiding this comment

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

why not all assignees? having all the list will allow seeing the distribution and spread work inside port between the different assignees, while only setting the first, might be misleading as it can be ordered alphabetically

Copy link
Author

Choose a reason for hiding this comment

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

It is a pre-defined blueprint and was advised to not modify. I can adjust it to take in all assignees


### Features

- Added Jira integration with support for projects and issues (0.1.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a clickup integration not Jira integration

Comment on lines 65 to 72
event: str = data.get("event", "")
logger.info(f"Received clickup webhook event: {event}")

if event.startswith("list"):
logger.info(f"Received webhook project event: {data['list_id']}")
project = await client.get_single_project(data["list_id"])

if event.endswith("Deleted"):
Copy link
Contributor

Choose a reason for hiding this comment

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

when starting resolving patterns from strings, its worth to leave some comments, examples and reference to how that kind of thing is expected to be received, it can help to onboard developers to the code and make it more readable

Comment on lines 95 to 98
# because the port-app-config uses the team ID to relate the projects to the teams
# we add the team ID to each project
# also, the clickup returns the datetime as a timestamp, so we convert it to a
# datetime object
Copy link
Contributor

Choose a reason for hiding this comment

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

we thrive to write a generic code, as although we do have in thought how the mapping will be used, we should be aware that users can change it to their needs and therefor make it generic and as extensible as we can.

e.g. instead of returning only the team_id, return the full team object.
also no need to mention the datetime and the transformation if you are not doing anything with it here

Comment on lines 114 to 121
project = next(
(
project
for project in await self.get_projects()
if project["id"] == project_id
),
None,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

as although it is nice it re-uses the same code as in the resync method, it is quite an not efficient way to resolve and get the project, as it is very costly to get list all the teams and then all the spaces and then all the projects just to get a single project.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's true. I'll think of a different approach

Comment on lines +25 to +45
async def create_webhook_events(self, app_host: str) -> None:
for team in await self.get_teams():
await self._create_team_webhook_events(app_host, team["id"])

async def _create_team_webhook_events(self, app_host: str, team_id: int) -> None:
webhook_target_app_host = f"{app_host}/integration/webhook"
webhooks_response = await self.client.get(
f"{self.clickup_url}/team/{team_id}/webhook"
)
webhooks_response.raise_for_status()
webhooks = webhooks_response.json()

existing_webhook = next(
(
webhook
for webhook in webhooks["webhooks"]
if webhook["endpoint"] == webhook_target_app_host
),
None,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

this whole logic, should be outside of the client, and the client should only expost get and create methods

Copy link
Contributor

Choose a reason for hiding this comment

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

the resolution logic shouldn't be inside the client

await ocean.register_raw(ObjectKind.PROJECT, [project])
elif event.startswith("task"):
logger.info(f"Received webhook task event: {data['task_id']}")
issue = await client.get_single_issue(data["task_id"])
Copy link
Contributor

Choose a reason for hiding this comment

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

what will get returned if its deleted event?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we even have what to return

Copy link
Author

Choose a reason for hiding this comment

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

Couldn't find a way to solve this. The webhook only returns the task ID, and digging into the unregister_raw method, it seems to expect the entire object so it can calculate differences.
Would it be okay to call Port's API from within an integration directly? I'm halfway into this but I realized users will need to generate an API token from the dashboard, and I think they shouldn't have to do that

Copy link
Author

Choose a reason for hiding this comment

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

For now, I've removed the blocks that check for deleted issues and projects

@PeyGis PeyGis closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants