Skip to content

zulip_bots: Create Google Calendar Bot. #204

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

YagoGG
Copy link
Member

@YagoGG YagoGG commented Dec 6, 2017

This addresses #161, by implementing my (now almost 2 year old) Google Calendar bot.

Feel free to be extremely nitpicky when reviewing: this was one of my very first contributions, expect to find funky stuff. I bet you can't beat this ;)

@YagoGG YagoGG force-pushed the bot-google-calendar branch from 4220de1 to 921a3b2 Compare May 27, 2018 21:05
@YagoGG YagoGG force-pushed the bot-google-calendar branch 4 times, most recently from 83d58fa to 1516180 Compare May 27, 2018 21:47
@YagoGG
Copy link
Member Author

YagoGG commented May 27, 2018

Updated the PR with some fixes and minor improvements.

I haven't added a unit tests because the bot needs a decent amount of setup in order to run, which cannot be prepared with the current testing framework. If we were to make quite a decent amount of mocking in order to test it, I could offer to make a followup PR.

Cheers!

@roberthoenig
Copy link
Collaborator

Huge thanks for resurrecting this @YagoGG! I just spend some time setting it up and testing it; it works.

With that said, there are a few things that should be resolved before offering this bot to users:

  • The doc.md file doesn't cover all of the setup. In particular, I found it missing some steps regarding the setup of the project in the Google Developer Console (e.g. one needs to enable the Google Calendar API).

  • For the oauth.py script, some parameters like credential_path are confusing and should probably be better explained and omitted in the simple example commands.

  • The bot seems to have some basic message parsing tests, but they are in the wrong place (directly in google_calendar.py). These should be moved to a proper test file or otherwise the test runner will probably fail.

@roberthoenig
Copy link
Collaborator

Nonetheless, I feel this PR has been stuck for too long and should just get merged and then iterated upon.

However, in light of especially the last bullet point, I wouldn't recommend directly adding this bot to the main directory. Instead, I'd add a commit to move the google_calendar bot to bots_unmaintained and then merge it. Then, we can close this PR, improve the bot, and eventually move it to the proper bots directory.

@YagoGG
Copy link
Member Author

YagoGG commented May 28, 2018

Thanks for reviewing, @roberthoenig!

I don't mind waiting a bit more in order to have this merged properly. I'll update the PR in a few hours :)

Regarding the parser tests, I forgot to make some comments about that: I tried to export that to a separate testing file, but I noticed the following:

  • As soon as you create the test file, the 3 standard checks that are built into the testing system are run (for instance, sending the bot an empty message). This causes some problems, since it involves starting the server —which can't be done due to the complex setup I mentioned earlier. A possible fix would be overriding those predefined tests.
  • Moving the stuff in test() to a separate file would require importing the bot's code from the new test file. Yesterday, when I played around with that idea, I had to do the trick of adding the bot's directory to the interpreter's path. Not sure if this would end up being necessary after some iterations, but just something worth keeping in mind.

@YagoGG YagoGG force-pushed the bot-google-calendar branch from 1516180 to fff5106 Compare May 28, 2018 17:27
@YagoGG
Copy link
Member Author

YagoGG commented May 28, 2018

I addressed for now the first two comments @roberthoenig made.

Let me know what you think about the tests and if you consider that further improvements are needed.

Thanks!

@snowe2010
Copy link

Hi, what is blocking this PR from being merged?

@timabbott
Copy link
Member

It would benefit from being rebased and tested by a new person, just to make sure it still works.

@zulipbot
Copy link
Member

zulipbot commented Apr 7, 2020

Heads up @YagoGG, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@PIG208
Copy link
Member

PIG208 commented Jun 14, 2021

I think this is worth picking up. We might need to do some extra testings on this to see if everything still works fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants