Summary

Add Mattermost chat integration.

Review Request #9361 — Created Nov. 10, 2017 and submitted Feb. 16, 2018, 2:55 p.m.

Information

Owner

giuliacm

Repository

rbintegrations

Branch

master

Bugs

Depends On

Commit

5c9dd88...

Reviewers

Groups

rbintegrations, students

People

Description

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.

Testing Done

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.

Issues

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_conley	Nov. 10, 2017, 12:55 p.m.
We also need an icon@2x.png file.	david	Dec. 3, 2017, 10:31 p.m.
All the files inside .idea shouldn't be part of your commit.	david	Nov. 29, 2017, 7:47 p.m.
You've still got this .idea file in the change.	david	Nov. 29, 2017, 7:47 p.m.
We should include module docstrings at the top of each new file (something like """Forms for chat integrations""")	david	Nov. 29, 2017, 3:54 p.m.
I don't think it was before, but this string should be wrapped in _()	david	Nov. 29, 2017, 3:55 p.m.
I don't think it was before, but this string should be wrapped in _()	david	Nov. 29, 2017, 3:56 p.m.
Module docstring.	david	Nov. 29, 2017, 3:57 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 3:58 p.m.
Shouldn't mention slack.	david	Nov. 29, 2017, 3:58 p.m.
These could all go on the same line.	david	Nov. 29, 2017, 3:58 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 3:59 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 3:59 p.m.
Hmm. This is probably something that we should have an abstract method for in basechat and specialize per subclass.	david	Nov. 29, 2017, 5:33 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4 p.m.
Logging here shouldn't mention slack.	david	Nov. 29, 2017, 4:01 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4:02 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4:02 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4:02 p.m.
Shouldn't mention slack here.	david	Nov. 29, 2017, 4:03 p.m.
We can just skip this entirely because the base class for all integrations defines it.	david	Nov. 29, 2017, 5:08 p.m.
Module docstring.	david	Nov. 29, 2017, 4:05 p.m.
Too many blank lines here.	david	Nov. 29, 2017, 4:05 p.m.
Not too many blank lines enough here.	david	Nov. 29, 2017, 5:09 p.m.
Because this is a property, we can document it like a property (just a single line `"""The icons used for …	david	Nov. 29, 2017, 4:06 p.m.
Module docstring	david	Nov. 29, 2017, 4:06 p.m.
Class docstring	david	Nov. 29, 2017, 4:08 p.m.
Don't need this blank line.	david	Nov. 29, 2017, 4:10 p.m.
Should include a docstring.	david	Nov. 29, 2017, 4:27 p.m.
Looks like we can just get rid of this file, no?	david	Nov. 29, 2017, 4:19 p.m.
Maybe mention that this is oriented towards Slack, but broken out of the integration class because other services (like Mattermost) …	david	Nov. 29, 2017, 4:14 p.m.
This argument should be called integration instead of self	david	Nov. 29, 2017, 4:12 p.m.
Perhaps just "configured channels"? I think it's definitely possible that other slack clones will pop up in the future.	david	Nov. 29, 2017, 4:12 p.m.
E265 block comment should start with '# '	reviewbot	Nov. 29, 2017, 4:34 p.m.
W391 blank line at end of file	reviewbot	Nov. 29, 2017, 5:35 p.m.
I think an argument needs a default value in order for it to be optional, I'm not seeing a default …	bleblan2	Dec. 10, 2017, 9:36 a.m.
This import is coming from the same location as the line above, this can be updated to a single line …	bleblan2	Dec. 3, 2017, 10:33 p.m.
There might be a good reason why this line was moved away from the django imports, but having the line …	bleblan2	Dec. 3, 2017, 10:36 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. 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?

giuliacm Nov. 10, 2017, 11:09 a.m.
```
I did, sorry! I'll add them.
```

Commit:

-	8ab1f36bb6e7b77234a7d5354833c386e50e9fb4
+	7c4e448d64044cb19afe4512bab5f1a23a799d75

Diff:

Revision 2 (+1469 -665)

Show changes

	.idea/misc.xml
	.idea/modules.xml
	.idea/rbintegrations.iml
	.idea/vcs.xml
	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/basechat/forms.py
	7 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

.idea/misc.xml (Diff revision 2)

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 5:37 p.m.

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

Commit:

-	7c4e448d64044cb19afe4512bab5f1a23a799d75
+	8758758bd112f33dbf1e68b04c481d094fed3e49

Diff:

Revision 3 (+3334 -627)

Show changes

	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/basechat/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/forms.py
	rbintegrations/mattermost/integration.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	[WIP] Add Mattermost chat integration.
+	Add Mattermost chat integration.

Testing Done:

~		All existing rbintegrations unit tests pass for Slack pass.
	~	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. Still need to add the Mattermost logo.
~
	~	form for Mattermost is displayed. Mattermost logo has been added, but
	~	icon@2x needs to be created/uploaded still.
-		Still need to add unit tests to for the newly added
-		MattermostIntegration component, as well as for BaseChatIntegration.

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 4:32 p.m.

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.

.idea/workspace.xml (Diff revision 3)

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 4:32 p.m.

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

rbintegrations/basechat/forms.py (Diff revision 3)

The issue has been resolved. Show all issues

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

rbintegrations/basechat/forms.py (Diff revision 3)

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 4:32 p.m.

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

david Nov. 29, 2017, 4:57 p.m.

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.

giuliacm Nov. 29, 2017, 5:20 p.m.
```
thanks!
```

rbintegrations/basechat/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
I don't think it was before, but this string should be wrapped in _()
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Module docstring.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These could all go on the same line.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```

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

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 4:49 p.m.

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

david Nov. 29, 2017, 4:57 p.m.
```
Yeah, that seems appropriate.
```

rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Logging here shouldn't mention slack.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```
rbintegrations/basechat/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Shouldn't mention slack here.
```

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

The issue has been resolved. Show all issues

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

giuliacm Nov. 29, 2017, 4:32 p.m.

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

david Nov. 29, 2017, 4:57 p.m.

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

rbintegrations/mattermost/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Module docstring.
```
rbintegrations/mattermost/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Too many blank lines here.
```

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

The issue has been resolved. Show all issues

Not too many blank lines enough here.

giuliacm Nov. 29, 2017, 4:32 p.m.

Should I add more lines or take some away?

david Nov. 29, 2017, 4:57 p.m.

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

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

The issue has been resolved. 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.""")

rbintegrations/mattermost/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Module docstring
```
rbintegrations/mattermost/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Class docstring
```
rbintegrations/mattermost/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Don't need this blank line.
```
rbintegrations/mattermost/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should include a docstring.
```
rbintegrations/slack/forms.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Looks like we can just get rid of this file, no?
```
1. giuliacm Nov. 29, 2017, 4:32 p.m.
  Forgot about this :)

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

The issue has been resolved. 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.

rbintegrations/slack/integration.py (Diff revision 3)
The issue has been resolved. Show all issues
```
This argument should be called integration instead of self
```

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

The issue has been resolved. Show all issues

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

Commit:

-	8758758bd112f33dbf1e68b04c481d094fed3e49
+	a6594f554d5c647cae123908cc8a0eba29272916

Diff:

Revision 4 (+3407 -639)

Show changes

	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/basechat/integration.py (Diff revision 4)
The issue has been resolved. Show all issues
```
E265 block comment should start with '# '
```

Commit:

-	a6594f554d5c647cae123908cc8a0eba29272916
+	79c27470b387cfc0b72631153077686e48e3372a

Diff:

Revision 5 (+3400 -639)

Show changes

	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	79c27470b387cfc0b72631153077686e48e3372a
+	95749603d0d7308afbd47ac3b0cbcd1c976a7b92

Diff:

Revision 6 (+3425 -620)

Show changes

	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbintegrations/basechat/integration.py (Diff revision 6)
The issue has been resolved. Show all issues
```
W391 blank line at end of file
```

Commit:

-	95749603d0d7308afbd47ac3b0cbcd1c976a7b92
+	975cf058f08560c1a3be9ee4b03a6f760b3a2db4

Diff:

Revision 7 (+3421 -620)

Show changes

	.idea/workspace.xml
	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	975cf058f08560c1a3be9ee4b03a6f760b3a2db4
+	9ac14f99c39e5bf23a13d6f7ab14170e2712b827

Diff:

Revision 8 (+2513 -620)

Show changes

	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

The issue has been dropped. 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.

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

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

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

rbintegrations/mattermost/integration.py (Diff revision 8)

The issue has been resolved. 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.

rbintegrations/slack/integration.py (Diff revision 8)

The issue has been resolved. 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.

Commit:

-	9ac14f99c39e5bf23a13d6f7ab14170e2712b827
+	00ee2a089b40f50208530577ebc453fa5b39068a

Diff:

Revision 9 (+2513 -620)

Show changes

	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

-	00ee2a089b40f50208530577ebc453fa5b39068a
+	5c9dd8801cb5dcceaec449a5431768469dff54d7

Diff:

Revision 10 (+2511 -618)

Show changes

	rbintegrations/extension.py
	rbintegrations/basechat/__init__.py
	rbintegrations/slack/forms.py
	rbintegrations/basechat/integration.py
	rbintegrations/mattermost/__init__.py
	rbintegrations/mattermost/integration.py
	rbintegrations/mattermost/tests.py
	rbintegrations/slack/integration.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (b0667c7)