Refactor Slack calls into a class

[milesbxf]

This refactors anything that calls slack_client.api_call into a new class at response.slack.SlackClient. There are a few benefits I think we get here:

• We centralise all the logic around actually doing the api_call:
  • we can check for a {"ok": False} response and translate that into a Python exception. Previously, this has been inconsistently applied around the codebase, sometimes making it very difficult to debug Slack errors
  • similar to above, we can add logging in one place to log all Slack API calls for debugging
• Whilst it's fairly straightforward to monkeypatch and mock out slack_client.api_call, our mock had to have lots of extra logic to handle each Slack API call (and record that it had been handled). This means our test code can now go from this:

    mock_slack = MagicMock(spec=SlackClient)
    slack_api_calls = {}
    def handle_slack_api_call(self, slack_api_name, *args, **kwargs):
        slack_api_calls[slack_api_name] = {"args": args, "kwargs": kwargs}
        if slack_api_name == "dialog.open":
            return {"ok": True, "ts": str(datetime.now().timestamp())}
    
    mock_slack.api_call.side_effect = handle_slack_api_call

    # ... run test

    assert 'dialog.open' in slack_api_calls

to this:

mock_slack = MagicMock(spec=SlackClient)

# ... run test
mock_slack.dialog_open.assert_called()

[evnsio]

I wonder if there's a way to avoid having to define this here? It would ideal if it was defined inside response, so people don't need to add this to their settings file.

[milesbxf]

Yes, I've since been wondering this myself actually. We could create a global slack_client object in response.slack.client that is instantiated using settings.SLACK_TOKEN. I'll put that in this afternoon 👍

[milesbxf]

I had a go at this. It's excruciatingly difficult to mock, because of the way Python imports things - I'm running into lots of the issues described in http://alexmarandon.com/articles/python_mock_gotchas/, and looks like we'd need to monkeypatch every single file that slack_client would be imported into.

Definitely agree that this isn't a great approach, but I'm not sure it's something we should spend lots of time solving right now