-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
integrations: Add ClickUp integration. #29390
base: main
Are you sure you want to change the base?
Conversation
@zulipbot add "area: integrations" "documentation review" "maintainer review" |
Hello @zulip/server-integrations members, this pull request was labeled with the "area: integrations" label, so you may want to check it out! |
0875d71
to
640ea93
Compare
640ea93
to
3da05cc
Compare
3da05cc
to
5a79644
Compare
fbbc4cf
to
69ca20f
Compare
Update: added more tests to 100% coverage. @timabbott |
15c479d
to
b0efba5
Compare
Update: Adjusted the doc format to better follow changes in #29592 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PieterCK - Thanks for all the work on this integration! I did a review of the documentation part of these changes only.
Let me know if you have any questions about my feedback comments. Overall it looks pretty good and follows our new structure for integration docs.
One general change I'd make is to reorder the instructions to follow an order more like the one we use in the Trello integration doc:
- Set up the Zulip bot/URL
- Get the ClickUp information
- Download the python script and run it
And as far as formatting in these files, we usually leave an empty line after headers and include brackets in the documentation pages, so instead of ...
![](image_location)
# Header
* text
{tip}
it would be ...
![](image_location)
# Header
* text
{tip}
b0efba5
to
cf939e6
Compare
@laurynmm thanks for the feedbacks! Here's what the doc currently looks like: If there are any more changes you'd like to suggest, feel free to leave another comment! |
@PieterCK Looks like there are some test failures to fix for this PR to be ready for the next round of review. |
cf939e6
to
3738ed2
Compare
Fixed it. Looking forward for your review 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PieterCK - I did another pass on the documentation updates for this new integration. Let me know if you have any questions!
@timabbott - Let me know if you want me to do a review of the integration implementation.
@@ -0,0 +1,60 @@ | |||
# Zulip ClickUp integration | |||
!!! tip "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a line break here for readability.
zerver/webhooks/clickup/doc.md
Outdated
|
||
{start_tabs} | ||
|
||
1. {!create-stream.md!} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file was renamed to create-channel.md
.
zerver/webhooks/clickup/doc.md
Outdated
|
||
1. Collect your ClickUp **Client ID** and **Client Secret** : | ||
|
||
- Go to <https://app.clickup.com/settings/team/clickup-api> and click **Create an App** button. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd drop "button" at the end of this sentence.
zerver/webhooks/clickup/doc.md
Outdated
|
||
- Go to <https://app.clickup.com/settings/team/clickup-api> and click **Create an App** button. | ||
|
||
- You will be prompted for **Redirect URL(s)**. Enter the URL for your Zulip organization. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you want a comma at the end of this line instead of a period.
zerver/webhooks/clickup/doc.md
Outdated
|
||
{!congrats.md!} | ||
|
||
![](/static/images/integrations/clickup/002.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason two screenshots of messages are included in these changes? We can probably just have one for the documentation.
3738ed2
to
3c81c0d
Compare
return [item.value for item in cls] | ||
|
||
|
||
class EventItemType(ConstantVariable): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant variables are declared in clickup/__init__.py
and not in view.py
to avoid circular import because api_endpoints.py
also uses a common constant variable; EventItemType
.
Creates an incoming webhook integration for ClickUp. The main use case is getting notifications when new ClickUp items such as task, list, folder, space, goals are created, updated or deleted. Fixes zulip#26529.
3c81c0d
to
a39b48d
Compare
Thank you for dropping another set of reviews for the doc page here, Lauryn. I've addressed them and also implemented some of the changes you suggested from the New Relic PR, where applicable. |
@kennethnrk Hello Kenneth, could you please review this PR? Thank you! @zulipbot label "buddy review" |
@PieterCK I'm unable to manually test this integration, I keep getting a response saying Is it because I don't have a valid clickup |
Yeah, this integration is a bit different from the other ones because ClickUp doesn't provide complete human-readable data in most of their payload. To make things worse, they don't have a user-friendly UI or workflow to set up webhooks. Instead, you have to make API calls to their API endpoints to register the webhook integration ( Zulip endpoint URL & which events). To get the API keys to access their APIs, you must go through their OAuth flow. This is the purpose of the helper script I submitted at this PR. It lives in the python-zulip-api repo and is required to integrate Zulip with ClickUp. For reference, our Trello integration follows a similar procedure to be integrated, and I largely followed that process for this script. In addition to registering our webhook endpoint with ClickUp, this script has another function: appending a couple of variables to the user's webhook URL query. One of these variables is the That is why our current tool wasn't able to test the integration because the webhook URL it uses doesn't have the This whole PR is actually the first PR that I've submitted to Zulip, it is quite hefty and complicated. If I were to re-do it, I'd split it into a few commits the first one being a very rudimentary integration without callbacks and thus simpler notifications just so that it is easier to be tested and partially merged, then add the more complicated features like callbacks and detailed notifications in separate commits following that. This is also a suggestion my mentor @sbansal1999 endorsed. I think I will do it, but given the cost of such refactoring and the priority of this PR, this task would find itself near the bottom of my TODO list... For now, I don't think you have to go through all that hassle to test the integration. It would already be very helpful if I could get a second pair of eyes to review the code styling and overall logic of the PR. @kennethnrk @sbansal1999 what do you guys think? |
Creates an incoming webhook integration for ClickUp. The main use case is getting notifications when new ClickUp items such as task, list, folder, space, goals are created, updated, or deleted.
Fixes #26529.
CZO thread
Notes:
zulip_clickup.py
for now. similar to that of Trello's.zulip_clickup.py
script. This method of storing third party API KEY for integration callbacks is subject to change (Add support for incoming webhooks with a callback to the third-party server #10907).Screenshots and screen captures:
zulip_clickup.py
walkthrough:Screencast from 21-03-24 22:36:35.webm
PR for the integration script: PR#824
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: