Add Mattermost chat integration.

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

giuliacm
rbintegrations
master
5c9dd88...
rbintegrations, students

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.

  • 0
  • 0
  • 39
  • 1
  • 40
Description From Last Updated
  1. 
      
  2. 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)
     
     

    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. 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)
     
     

    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)
     
     

    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)
     
     

    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)
     
     

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

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

    Module docstring.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack.

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

    These could all go on the same line.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    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)
     
     

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    Logging here shouldn't mention slack.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    Shouldn't mention slack here.

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

    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. Module docstring.

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

    Too many blank lines here.

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

    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)
     
     
     
     
     
     

    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)
     
     

    Module docstring

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

    Class docstring

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

    Don't need this blank line.

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

    Should include a docstring.

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

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

    1. Forgot about this :)

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

    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)
     
     

    This argument should be called integration instead of self

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

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

giuliacm
giuliacm
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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. 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)
     
     

    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: Closed (submitted)

Change Summary:

Pushed to master (b0667c7)
Loading...