integrations/clickup: Add ClickUp integration script. - #921
integrations/clickup: Add ClickUp integration script.#921sathwikshetty33 wants to merge 1 commit into
Conversation
1d63b18 to
daa1323
Compare
|
@0xthedance tagging you for a review. |
|
@zulipbot add "buddy review" |
0xthedance
left a comment
0xthedance
left a comment
There was a problem hiding this comment.
LGTM. I've run the script for creating and replacing webhooks in my ClickUp app, and it works.
I've included a few slight suggestions, but feel free to skip any that don't feel clarifying
ClickUp payloads contain only entity IDs, so the server needs a stored ClickUp token to resolve them to names. Use a personal API token instead of OAuth: the token must be present in the bot config at bot-creation time, whereas an OAuth flow can only issue one after the bot and webhook URL already exist. Fixes part of zulip/zulip#26529 Co-Authored-By: pieterck <pieterceka123@gmail.com>
daa1323 to
b869dbc
Compare
|
@0xthedance Thanks for the review! |
|
@zulipbot remove "buddy review" |
|
@zulipbot add "mentor review" |
|
Tagging @sbansal1999 and @prah23 for review. |
sbansal1999
left a comment
sbansal1999
left a comment
There was a problem hiding this comment.
Thanks @sathwikshetty33 for working on this one. I have left some comments on this PR; feel free to create a thread in CZO if you feel any of them needs any discussion.
| if response.status != 200: | ||
| print(f"Error : {response.status}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
- This does an abrupt sys.exit call rather than handling the error properly.
- Can this branch be deleted given
urllibraisesHTTPErrorwhen the response status is not 2XX?
| "folder": ("folderCreated", "folderUpdated", "folderDeleted"), | ||
| "space": ("spaceCreated", "spaceUpdated", "spaceDeleted"), | ||
| "goal": ("goalCreated", "goalUpdated", "goalDeleted"), | ||
| "key result": ("keyResultCreated", "keyResultUpdated", "keyResultDeleted"), |
There was a problem hiding this comment.
In the above linked doc it says 'Target' instead of key result.
Can you check if that's the term being used now?
| if ( | ||
| event_code.strip().isdigit() | ||
| and (event := code_to_event_dict.get(int(event_code))) in EVENT_CHOICES | ||
| ): | ||
| if event_code not in exhausted_options: | ||
| events = EVENT_CHOICES[event] |
There was a problem hiding this comment.
We can simplify this be using a code_to_events dict, whose key is the event code and the value will be an array with events for that value.
code_to_events = {i + 1: events for i, events in enumerate(EVENT_CHOICES.values())}| - **Zulip webhook URL** — the URL generated by your Zulip incoming | ||
| webhook bot. |
There was a problem hiding this comment.
Should we add the URL generation doc here; similar to how it's being done in the script's help section.
| user_input: list[str] = re.split(",", input_codes_list) | ||
| input_is_valid: bool = len(user_input) > 0 |
There was a problem hiding this comment.
Should we modify the parsing logic to accept inputs like 2,,5 in them (two commas).
So if the user enters 2,,5 we should consider it as 2,5.
| selected_events: list[str] = [] | ||
| input_codes_list: str = input("EVENT CODE(s): ") | ||
|
|
||
| if "*" in input_codes_list: |
There was a problem hiding this comment.
An in check would register all events for an input like 1,2* which actually is an invalid input and should be flagged accordingly.
| input_is_valid = False | ||
|
|
||
| if not input_is_valid: | ||
| print("Please enter a valid set of options and only select each option once") |
There was a problem hiding this comment.
The error text says "only select each option once", but duplicates in the input actually work.
4 participants
This is a replacement PR for #824 .
The original PR used an OAuth approach. The bot was created without a ClickUp token, and the script ran an OAuth flow to generate one. That token, along with the team ID, was then appended to the bot's webhook URL as query parameters, so the key essentially lived in the URL.
The server side has since moved on. It now reads the ClickUp token from the bot's config data, where it is a required option at bot creation, and uses it to call ClickUp's API and turn the entity IDs in webhook payloads into readable names.
That is what rules OAuth out. The config option needs the token when the bot is created, but an OAuth flow can only return a token after the bot and its webhook URL already exist, so there is no point at which OAuth could provide what bot creation asks for.
This PR therefore drops OAuth entirely. Instead, the user generates a personal API token from the ClickUp UI (Settings > ClickUp API > API Token), enters it when creating the bot, and passes the same token to this script to register the webhook. The script now registers a plain webhook URL with nothing appended, since the server takes the team ID straight from the payload and the token from config.
Fixes part of zulip/zulip#26529
CZO: thread
How did you test this PR?
Manually invoked the script and added unit tests.
Testing video
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: