python-zulip-api icon indicating copy to clipboard operation
python-zulip-api copied to clipboard

zulip_bots: Create Google Calendar Bot.

Open YagoGG opened this issue 7 years ago • 9 comments

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 avatar Dec 06 '17 20:12 YagoGG

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!

YagoGG avatar May 27 '18 21:05 YagoGG

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 avatar May 28 '18 08:05 roberthoenig

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.

roberthoenig avatar May 28 '18 08:05 roberthoenig

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 avatar May 28 '18 10:05 YagoGG

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!

YagoGG avatar May 28 '18 17:05 YagoGG

Hi, what is blocking this PR from being merged?

snowe2010 avatar Jun 28 '19 23:06 snowe2010

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

timabbott avatar Jul 13 '19 18:07 timabbott

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.

zulipbot avatar Apr 07 '20 23:04 zulipbot

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

PIG208 avatar Jun 14 '21 15:06 PIG208