python-zulip-api
python-zulip-api copied to clipboard
Update the Google Calendar integration doc.
Follow-up to the Google Calendar integration updates.
Moved from #32783 following the migration of the Python API integration docs.
Performs a complete re-write of the doc, and is quite different from the version in the previous PR.
Screenshots
Self-review checklist
- [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
- [ ] Explains differences from previous plans (e.g., issue description).
- [ ] Highlights technical choices and bugs encountered.
- [ ] Calls out remaining decisions and concerns.
- [ ] Automated tests verify logic where appropriate.
Individual commits are ready for review (see commit discipline).
- [x] Each commit is a coherent idea.
- [x] Commit message(s) explain reasoning and motivation for changes.
Completed manual review and testing of the following:
- [x] Visual appearance of the changes.
- [ ] Responsiveness and internationalization.
- [ ] Strings and tooltips.
- [ ] End-to-end functionality of buttons, interactions and flows.
- [ ] Corner cases, error conditions, and easily imagined bugs.
@laurynmm Could you please review this?
Wow, that's a pretty thorough review, thanks a lot! You didn't miss even a hyphen discrepancy :) Reviewer goals!
I'm formatting my comments and updates in a single block, because I've clubbed multiple questions with single replies, and also so that these comments don't get submerged as the original comments get outdated.
I've absorbed all the phrasing improvements you've suggested. Thank you!
Screenshots
The macro
So, for Zulip Cloud, this step isn't necessary, correct? Or is this integration only available for self-hosted servers?
Also, just confirming that this can be done on any machine.
Ah, you're right. I just used the macro that was used in the other integrations. This integration can be run from any system, doesn't have to necessarily be the Zulip server. Thank you for catching that. I've removed the macro. I assume it would be fine to keep the installation step. But, I've moved it to the right section.
Config file example and command arguments example
If the Google Meet link is missing from the payload, the join call link would be broken, correct? I wonder if we want that in the message template if so...
Does this need to be included in the file configuration if it's blank?
Do these need to be included in this example template? I think the options should be listed somewhere, but I imagine most folks aren't going to change these file location defaults.
My intention was to include all the options in the sample config file and the sample command. I didn't intend for it to be copy pasted, since we have better default values (like the Google meet link case you mentioned), but I can see how users are likely to do that if we provide a full config file. So, I've now made the examples leaner.
And I've moved the format_message sample to the configuration options section, because we might want to show the sample without the users using that exact one. The code logic constructs the message template conditionally, which wouldn't be possible with a fixed template.
Location of configuration options section
someone implementing this is definitely going to want to review all of that before running the script.
I think the usual approach would be to getting it running first, and then configuring it as desired. So, it makes sense to keep the optional configuration options at the end, after the instructions for setting it up. The default values should be robust enough that they wouldn't need a config file or need to pass any parameters, unless they really want to.
Google Cloud instructions
When I followed this link, I had to create a project before I could access that page... Also, I had no idea what Google Cloud was and I imagine a general Google email user wouldn't either.
Oh, you're absolutely right, I've added a new instruction now. But, it's got multiple steps. I didn't want to break it down into individual steps, because they reduce readability, it's helpful to keep the steps for this instruction together?
I've not included a link to the Google Cloud console because it redirects to a very generic welcome page. The specific pages have links though, which are the final URLs for each step.
I've also added the generic link to that in the related docs section.
Calendar ID
Where would one find the ID for a Google calendar?
I'd addressed this here in this comment last year :melting_face: "I've removed the instructions on how to get the --calendar value, as they've changed over time and are hence better googled. There's no particular Google documentation to link to either."
I've now made that option the penultimate option, so that it's not the first option that users come across.
For you to test, I'm sharing how I got the calendar ID - Go to Settings, select a calendar (other than "primary" and "birthdays"). In the "Calendar settings" tab for that particular calendar, there's an Integrate calendar section that contains the Calendar ID.
Tokens-file option
When would it make sense to specify this
tokens-fileoption? Does running the script ever update the tokens?
Yes, the script needs to refresh the token every hour. The token expires in an hour. I mentioned "first run" because the file doesn't exist until then. Inspired by your question, I've now added that it gets rewritten every hour.
@Niloth-p - Thanks for making those updates and that comprehensive summary note! I think this is much easier to read and understand now.
A few notes I have after reading through the screenshot of the doc above:
- I think we can remove the "Configure the integration" header and just put all of that under "Configuration options".
### Configuration options
The integration can be configured by:
- Passing command line arguments to the integration script.
- Editing the `google-calendar` section of the `zuliprc` file.
The configuration settings in `zuliprc` will be overridden by the
corresponding command line options, if both are used.
The integration script accepts the following configuration options:
- `interval`: How many minutes in advance you want reminders delivered.
The default value is 30 minutes.
-
Question about steps 3 and 4 in the "Run the integration script" section. Does it matter if you run the script without the command line arguments and then run it again with them? I assume that one could run the script with various arguments to test how one wants to eventually configure the
zuliprcfile? Maybe step 4 should say "reconfigure" vs "configure". Or it could be formatted as a bullet point under step 3. I'm not sure what would be the best/clearest. -
I think that instances of "commandline" should be "command line", correct?
-
I assume if the terminal session is closed/ended, then the user only needs to rerun the script in a new session to get the integration running again. If so, it might be worth noting that in step 5 of running the integration script.
Thanks for the fast review! Sorry for my delay in updating the PR.
-
Haha, sure, I started off with all of that under "Configuration options", but then moved the ways to set them outside thinking you'd prefer to keep the config options section to only have the options info.
-
The state is not stored. So, each run is independent. So, yes, one could run the script with various arguments to test how one wants to eventually configure the zuliprc file. I went with the switch of "configure" to "reconfigure". I think using a bullet point may be awkward since we'll have just the one point?
-
Apparently, the correct usage is "command line" as two words when used as a noun, and "command-line" with a hyphen when used as an adjective. I've updated all instances now, thanks for pointing that out.
-
Added a line that the user only needs to rerun the script to get the integration running again, as suggested.
Screenshots
@Niloth-p - Thanks for making those updates! I think these instructions are much clearer, but I haven't gone through the process of setting up the integration to test. I believe that @alya wanted to do that as the next step in the review process, so probably pinging her on CZO for the next round of review makes sense.
Before the "Google Auth Platform not configured yet" UI, I needed to create a project:
So I guess we should document that as a thing that might happen.
Once I clicked "Create", the json file wasn't downloaded automatically. From what I'm seeing currently, there needs to be a dedicated download step, and an instruction to "OK" out of the dialog. Leaving out the data, I see:
Maybe we do this for other integrations too, but I got pretty grumpy when the instructions told me to save files in ~/. I don't want stuff in my home directory! Does it seriously have to go there?
For the secrets file, I guess the client-secret-file config means it could be elsewhere, but perhaps there's some better default for it?
This is what happened when I tried to rename the zuliprc file to .zuliprc:
Am I doing something wrong here?
alya@MacBook-Pro-5 python-zulip-api % python /usr/local/share/zulip/integrations/google --provision
/Library/Developer/CommandLineTools/usr/bin/python3: can't find '__main__' module in '/usr/local/share/zulip/integrations/google'
OK, that's my feedback so far, and I think I'm stuck for the moment.
FWIW, I felt like doing the Google-side config before setting up the bot, I think because it felt higher-risk.