bots: Enable 'run_bot_message_handler_for_bot' to be used by Embedded…

[abhijeetkaur]

… bots.

External bots are executed by the run.py script, which calls
'run_bot_message_handler_for_bot'. This function has multiple
nested functions that Embedded bots need to use in order to run
smoothly.

Since External bots uses an explicit 'Client' class object to run,
but Embedded bots run without it. Modifications to the function
had to be done along with refactoring.

[abhijeetkaur]

Leaving empty messages unhandled for now, as I saw a PR regarding empty messages by @neiljp.

We can remove merge conflicts as and when any of the PR gets merged. :)

[timabbott]

@abhijeetkaur I think it might make sense to add docstring notes for the functions shared between the embedded and external bot systems explaining that they're part of the interface used by zulip/zulip. Otherwise, there's risk of someone doing the opposite refactoring and breaking things :).

[abhijeetkaur]

@timabbott Done!
But I am not sure if such descriptive message is appropriate or not. I just felt that if I do that for one function, it doesn't make sense not to do for the others.

I can modify it as per your suggestion. :)

[timabbott]

I'd just have the docstrings for all but the first such function just point to the first one's.

Can you rebase so the tests pass?

[abhijeetkaur]

@timabbott Sure! Even I thought of pointing to the first one, but then I thought eventually the ordering of the functions might also change. So, it may lose it meaning.

Rebasing...

[abhijeetkaur]

I wonder why the test is failing here, it is getting passed in my local env.

[abhijeetkaur]

I checked each test log, all of them are passing. But due to environment variable not being set, it shows that travis tests have failed.

[timabbott]

The issue is the linter is failing:

Line too long (146) at zulip_bots/zulip_bots/lib.py line 269:         message_content_if_bot_is_called = get_message_content_if_bot_is_called(message=message, at_mention_bot_name=restricted_client.full_name)

@derAnfaenger we may want to make the linter its own separate suite from the tests...

[roberthoenig]

@timabbott I'm not sure I understand you correctly; are you thinking of something like a second Travis run that just runs the linter?

[timabbott]

Yes, I'm thinking of moving the linter to a separate job, like do things in the server repository.

[zulipbot]

Heads up @abhijeetkaur, 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.

[timabbott]

OK, for this PR, @abhijeetkaur you need to split out your changes to be more reviewable. It is basically impossible to review a change where you both move some functions around and edit their content in the same commit. I split out a chunk of moving a couple functions as part of reviewing this, and merge that commit. Reading this, I also notice some other things need significant work:

• The is_private function is just broken. We should never be checking things using full_name; use the user_id instead. Names aren't guaranteed unique; user IDs are :). Can you do a quick PR with a commit or two that cleans up that function to work based on user IDs and has an interface that will work for embedded bots (and no other changes)?
• And then you can do a commit or two doing the same thing with extract_query_without_mention. I think there, the check has 2 parts: (1) We should check the message flags in the message dict to see whether the mentioned flag is set; that's the authoritative source for whether this use was mentioned. And then I guess we need the full name for checking whether the mention is at the start of the message.

Once those are done, we can pick up the other two functions, which are simpler. Sound good?

[abhijeetkaur]

@timabbott I have opened #81 for is_private function.

[abhijeetkaur]

@timabbott For the second point raised by you above, we already are doing the checks. Only when mentioned flag is set, only then are we extracting the at-mention string.
I guess the commit in PR has too much for 1 commit; I would have created a separate PR for all the functions individually (as that would be easy to review) but all the functions are interlinked, modifying one is dependent on the other.

Adding further commits for extract_query_without_mention function to the PR raised above #81.

[abhijeetkaur]

@timabbott With you already have merged the "Extraction of nested functions to top-level", what I started with an endeavour to create a separate PR for each function; Since all functions were dependent, I ended up adding atomic commits in #81.

This PR can be closed as all the rest of the work apart from refactoring is reproduced in #81. :)

[timabbott]

Closing, thanks @abhijeetkaur!