Add Mattermost chat integration.

Review Request #9361 — Created Nov. 10, 2017 and submitted

Information

rbintegrations
master
5c9dd88...

Reviewers

This change integrates Mattermost with Review Board, so that when there
is activity on review requests, a message will be sent to the configured
Mattermost channel with the details, just like Slack.

All existing rbintegrations unit tests pass for SlackIntegration. Added
similar unit tests for MattermostIntegration, which also pass.

Manually tested navigating to the Integrations page on the UI. The new
form for Mattermost is displayed. Mattermost logo has been added, but
icon@2x needs to be created/uploaded still.

Description From Last Updated

I think you may have missed adding some files - I don't see the MattermostIntegration code in here, for example. …

mike_conleymike_conley

We also need an icon@2x.png file.

daviddavid

All the files inside .idea shouldn't be part of your commit.

daviddavid

You've still got this .idea file in the change.

daviddavid

We should include module docstrings at the top of each new file (something like """Forms for chat integrations""")

daviddavid

I don't think it was before, but this string should be wrapped in _()

daviddavid

I don't think it was before, but this string should be wrapped in _()

daviddavid

Module docstring.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack.

daviddavid

These could all go on the same line.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

Hmm. This is probably something that we should have an abstract method for in basechat and specialize per subclass.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

Logging here shouldn't mention slack.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

Shouldn't mention slack here.

daviddavid

We can just skip this entirely because the base class for all integrations defines it.

daviddavid

Module docstring.

daviddavid

Too many blank lines here.

daviddavid

Not too many blank lines enough here.

daviddavid

Because this is a property, we can document it like a property (just a single line `"""The icons used for …

daviddavid

Module docstring

daviddavid

Class docstring

daviddavid

Don't need this blank line.

daviddavid

Should include a docstring.

daviddavid

Looks like we can just get rid of this file, no?

daviddavid

Maybe mention that this is oriented towards Slack, but broken out of the integration class because other services (like Mattermost) …

daviddavid

This argument should be called integration instead of self

daviddavid

Perhaps just "configured channels"? I think it's definitely possible that other slack clones will pop up in the future.

daviddavid

E265 block comment should start with '# '

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

I think an argument needs a default value in order for it to be optional, I'm not seeing a default …

bleblan2bleblan2

This import is coming from the same location as the line above, this can be updated to a single line …

bleblan2bleblan2

There might be a good reason why this line was moved away from the django imports, but having the line …

bleblan2bleblan2
mike_conley
  1. 
      
  2. Show all issues

    I think you may have missed adding some files - I don't see the MattermostIntegration code in here, for example. Are you sure you used git add before committing?

    1. I did, sorry! I'll add them.

  3. 
      
giuliacm
david
  1. Most of the stuff in your new "basechat" implementation is pretty Slack specific. This works because Mattermost tries to duplicate Slack's formatting very carefully, but that won't generalize to more services. I suspect what we want is to have the basechat integration set up all the handlers and create some kind of intermediate data structures, which the individual subclasses can then format into whatever is necessary. Because Mattermost borrows a lot from Slack, we could have utility code for that in the slack/ module that Mattermost imports.

  2. .idea/misc.xml (Diff revision 2)
     
     
    Show all issues

    All the files inside .idea shouldn't be part of your commit.

    1. Can I delete all the files in .idea or should I keep them and just remove them from my commit?

  3. 
      
giuliacm
giuliacm
david
  1. 
      
  2. Show all issues

    We also need an icon@2x.png file.

    1. I changed my description to explain that I still need to add the icon2@x.png, but I might have some questions about how to do this.

  3. .idea/workspace.xml (Diff revision 3)
     
     
    Show all issues

    You've still got this .idea file in the change.

    1. I know :( I thought I got rid of them, but looks like there's still one left.

  4. rbintegrations/basechat/forms.py (Diff revision 3)
     
     
    Show all issues

    We should include module docstrings at the top of each new file (something like """Forms for chat integrations""")

  5. rbintegrations/basechat/forms.py (Diff revision 3)
     
     
    Show all issues

    I don't think it was before, but this string should be wrapped in _()

    1. I made the change, but why does it have the underscore?

    2. At the top of the file we have this:

      from django.utils.translation import ugettext_lazy as _
      

      so _('What to Post') is the same as ugettext_lazy('What to Post'). This means we'll be able to (eventually) show the text in the user's native language, if they use something other than English.

    3. thanks!

  6. rbintegrations/basechat/forms.py (Diff revision 3)
     
     
    Show all issues

    I don't think it was before, but this string should be wrapped in _()

  7. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Module docstring.

  8. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  9. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack.

  10. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
    Show all issues

    These could all go on the same line.

  11. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  12. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  13. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
     
     
     
     
    Show all issues

    Hmm. This is probably something that we should have an abstract method for in basechat and specialize per subclass.

    1. Should I raise a NotImplementedError for format_link in basechat, and do something similar to notify's utility function in slack.integration (since Mattermost would use this implementation as well)?

    2. Yeah, that seems appropriate.

  14. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  15. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  16. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  17. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Logging here shouldn't mention slack.

  18. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  19. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  20. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  21. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
    Show all issues

    Shouldn't mention slack here.

  22. rbintegrations/basechat/integration.py (Diff revision 3)
     
     
     
     
    Show all issues

    We can just skip this entirely because the base class for all integrations defines it.

    1. So I can delete this completely from basechat? I thought I should still define it, but raise a NotImplementedError.

    2. Yep, no need to have it. If a subclass doesn't provide it then it'll just not show an icon.

  23. Show all issues

    Module docstring.

  24. rbintegrations/mattermost/integration.py (Diff revision 3)
     
     
     
    Show all issues

    Too many blank lines here.

  25. rbintegrations/mattermost/integration.py (Diff revision 3)
     
     
     
    Show all issues

    Not too many blank lines enough here.

    1. Should I add more lines or take some away?

    2. Add a blank line in here. django is a 3rd-party module and rbintegrations is the current module.

  26. rbintegrations/mattermost/integration.py (Diff revision 3)
     
     
     
     
     
     
    Show all issues

    Because this is a property, we can document it like a property (just a single line `"""The icons used for the integration.""")

  27. rbintegrations/mattermost/tests.py (Diff revision 3)
     
     
    Show all issues

    Module docstring

  28. rbintegrations/mattermost/tests.py (Diff revision 3)
     
     
    Show all issues

    Class docstring

  29. rbintegrations/mattermost/tests.py (Diff revision 3)
     
     
    Show all issues

    Don't need this blank line.

  30. rbintegrations/mattermost/tests.py (Diff revision 3)
     
     
    Show all issues

    Should include a docstring.

  31. rbintegrations/slack/forms.py (Diff revision 3)
     
     
    Show all issues

    Looks like we can just get rid of this file, no?

    1. Forgot about this :)

  32. rbintegrations/slack/integration.py (Diff revision 3)
     
     
     
    Show all issues

    Maybe mention that this is oriented towards Slack, but broken out of the integration class because other services (like Mattermost) duplicate Slack APIs.

  33. rbintegrations/slack/integration.py (Diff revision 3)
     
     
    Show all issues

    This argument should be called integration instead of self

  34. rbintegrations/slack/integration.py (Diff revision 3)
     
     
    Show all issues

    Perhaps just "configured channels"? I think it's definitely possible that other slack clones will pop up in the future.

  35. 
      
giuliacm
Review request changed
Commit:
8758758bd112f33dbf1e68b04c481d094fed3e49
a6594f554d5c647cae123908cc8a0eba29272916

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

giuliacm
giuliacm
Review request changed
Commit:
79c27470b387cfc0b72631153077686e48e3372a
95749603d0d7308afbd47ac3b0cbcd1c976a7b92

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

giuliacm
giuliacm
bleblan2
  1. 
      
  2. rbintegrations/basechat/integration.py (Diff revision 8)
     
     
    Show all issues

    I think an argument needs a default value in order for it to be optional, I'm not seeing a default value for pre_text or fallback_text in the method declaration.

  3. rbintegrations/basechat/integration.py (Diff revision 8)
     
     

    If fallback_text is optional, I'm guessing that means it can be empty. If it is empty, the new value of fallback_text would have an extra : in it. Might be worth adding extra logic for checking if fallback_text is empty and removing the extra :.

  4. rbintegrations/basechat/integration.py (Diff revision 8)
     
     

    Same thing here with a possibly empty fallback_text getting passed in.

  5. Show all issues

    This import is coming from the same location as the line above, this can be updated to a single line with from rbintegrations.slack.integration import utility_format_link, utility_notify.

  6. rbintegrations/slack/integration.py (Diff revision 8)
     
     
    Show all issues

    There might be a good reason why this line was moved away from the django imports, but having the line here doesn't follow the standard of having third party imports before local imports.

  7. 
      
giuliacm
giuliacm
david
  1. Ship It!
  2. 
      
giuliacm
Review request changed
Status:
Completed
Change Summary:
Pushed to master (b0667c7)