add Matrix Integration option
Review Request #11219 — Created Oct. 14, 2020 and submitted
Information | |
---|---|
ruonanjia | |
rbintegrations | |
master | |
Reviewers | |
rbintegrations, students | |
Admins can choose to add Matrix Integration.
This will send notifications to the registered matrix room
about new review requets and updates to existing ones.
Tested in RB4.
Edited Slack unit test cases to test Matrix integration.
All tests have passed except 2 of then complaining about
missing image attachmenttests with errors are:
- test_notify_new_review_request_with_image_file_attachment
- test_notify_updated_review_request_with_new_image_attachments
Summary | Author |
---|---|
ruonan |
Description | From | Last Updated |
---|---|---|
I think you might need to uncommit the files in the .idea/ directory since they are from PyCharm |
|
|
Can you please attach the icon.png and icon@2x.png files to the review request? |
|
|
Hmmm... the mentors will have the final say on this. Personally, I would move the matrixValue lines to MatrixIntegration, since … |
|
|
I just realized today that the integrations are in alphabetical order. So, you may want to move MatrixIntegration at lines … |
|
|
Hm - it looks like we're hardcoding in the matrix.org homeserver in here. But we should make it so that … |
|
|
'Slack' to 'Matrix' |
|
|
'Slack' to 'Matrix' |
|
|
Does Matrix also require urls to be formatted like this? If so, maybe replace Slack/Mattermost with Matrix in the comments |
|
|
'Slack' -> 'Matrix' |
|
|
E501 line too long (87 > 79 characters) |
![]() |
|
I agree with Hailan's review that this smells bad. Since we already have code that's checking the fields, let's pull … |
|
|
Please keep this list in alphabetical order. |
|
|
Please add a blank line between these two. The module docstring should also be clear that this is about matrix. |
|
|
Does the capitalization listed here match the capitalization in the matrix app? If it's capitalized there, it should be here … |
|
|
Please add a period at the end of this help text. |
|
|
Can we tweak the usage of "e.g." here? "The server HTTP requests will be sent to, e.g., https://matrix.org" |
|
|
This can wrap a little bit tighter. |
|
|
Please add a module docstring to this file. |
|
|
Let's switch these to keep them in alphabetical order. |
|
|
We need to be very careful regarding the use of str in Python, because it has different meanings in Python … |
|
|
Same comments here regarding str and format_html |
|
|
Same comments here regarding str and format_html |
|
|
This is probably fine to keep since it's a debug message, but let's use a named logger. At the top-level … |
|
|
This would be better using a format string: url = ('%s/_matrix/client/r0/rooms/%s/send/' 'm.room.message?access_token=%s' % (server, room_id, access_token)) |
|
|
This blank line can go away. |
|
|
Instead of using the continuation character here, can we just wrap it in parens? |
|
|
It feels like the contents of the notify method could be moved back into this class. We're not calling it … |
|
|
Leftover debug output? |
|
|
Leftover debug output? |
|
|
Leftover debug output? |
|
|
Col: 10 'authorize' is defined but never used. |
![]() |
|
This should be moved, so that it is in alphabetical order. |
![]() |
|
I forgot to include that I think that this field should have a default value set to https://matrix.org. I am … |
![]() |
|
Lines 10 and 11 should be swapped, so that the alphabetical order is maintained. |
![]() |
|
This comment should end with a period. |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E303 too many blank lines (2) |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
W291 trailing whitespace |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
W293 blank line contains whitespace |
![]() |
|
E303 too many blank lines (3) |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
E302 expected 2 blank lines, found 1 |
![]() |
|
E127 continuation line over-indented for visual indent |
![]() |
|
Please add a second blank line here. |
|
|
This needs a docstring. |
|
|
It might be nice to change this to use re.sub and pass a function for the repl argument. That way … |
|
|
This should use format_html from django.utils.html. |
|
|
This should use format_html from django.utils.html. Please also use single quotes. |
|
|
Please use single quotes throughout. Final item also needs a trailing comma. |
|
|
This needs a module docstring. |
|
|
This needs a class docstring. |
|
|
Please put this """ on its own line. |
|
|
This needs a trailing comma. |
|
|
This line can go away. |
|
|
Please add a docstring for this. |
|
|
E128 continuation line under-indented for visual indent |
![]() |
|
E128 continuation line under-indented for visual indent |
![]() |
|
"Matrix" |
|
|
It looks like the text is wrapping too early. You should be able to fit more per line, I think? |
|
|
Same here. |
|
|
will be sent to. |
|
|
... and clicking on the "Advanced" tab. |
|
|
This should be replace_shortcode. It should also go on the class, since it's specific to this implementation. |
|
|
Docstring summaries must be one line, without wrapping, and must fit within column bounds. |
|
|
How about: "... replaced by their equivalent Unicode characters." |
|
|
No blank line here. |
|
|
emoji_unicode |
|
|
We should define the regex as a constant and compile it, so this is faster every time it needs to … |
|
|
We don't need to mention Slack here, since this isn't related to Slack's implementation, right? |
|
|
This should be localized using ugettext_lazy. |
|
|
Mind sorting these alphabetically? It helps with maintenance. |
|
|
This will be nicer if formatted as: ASSETS_BASE_URL = \ 'https://...' |
|
|
Since this is newly-added, let's pick a more modern date and time (like today's). |
|
|
The order of arguments here doesn't match the documented order. Can you fix that up? |
|
|
This needs , optional. |
|
|
This needs , optional. |
|
|
String building is faster using format strings (%). |
|
|
Same here. |
|
|
This will need an .encode('utf-8') after it. |
|
|
When formatting multi-line strings/arguments, we generally format it like: url = ( '...' '...' % (...) ) |
|
|
Mind alphabetizing these while you're here? |
|
|
Old code probably had an old pattern for this, but you can get rid of exc_info= and instead use logger.exception(...). |
|
|
Can this just include format_link's code from above? I don't think we need this split up at all. |
|
|
Completely optional request, but it'd be more maintainable if these could be broken up around HTML tags a bit better, … |
|
|
Feels like this wraps too early. |
|
|
Blank line betewen these. |
|
|
Formatting here isn't consistent with other tests (or style guidelines). The keys should be on their own lines. This applies … |
|
|
You'll need to pass id=10000 as a parameter to create_review_request() to avoid an issue that can impact other tests. |
|
|
Looks like a ! snuck in here. |
|
|
Missing a "Returns". |
|
|
Missing , optional. |
|
|
Is this doable at this stage? |
|
|
"profile" or "Profile"? Can you change ">" to "->"? It's more standard. |
|
|
on the "Advanced" tab. |
|
|
Let's put the "e.g., https://matrix.org" in parens, and end with a period after. |
|
|
Missing a trailing comma. |
|
|
For format_html(), it's very important that we differentiate between what's HTML and what's text going into it. We don't want … |
|
|
This will also need to use {} and arguments when calling format_html(). Also, HTML attribute values should have " around … |
|
|
String concatenation in Python isn't very efficient. Rather than doing this, a common pattern is: html = [] html.append(...) html.append(...) … |
|
|
Blank line between these. |
|
|
No blank line here. |
|
|
The pattern should be compiled for the instance in __init__, rather than each time this function is called (otherwise it … |
|
|
This should be a reviewboard.integrations.models.IntegrationConfig. |
|
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+2842 -2) |
Checks run (2 succeeded)
Description: |
|
||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
-
-
rbintegrations/basechat/integration.py (Diff revision 2) Hmmm... the mentors will have the final say on this. Personally, I would move the matrixValue lines to MatrixIntegration, since basechat is shared among all chat integration services. Something like "if value is :warning: then replace it with the warning emoji string '\U000026A0'"
-
-
-
rbintegrations/matrix/integration.py (Diff revision 2) Does Matrix also require urls to be formatted like this? If so, maybe replace Slack/Mattermost with Matrix in the comments
-
-
This is great work, Ruonan! Just one comment to start, since it's a high-level piece of feedback that might require you to update forms.py and integration.py.
-
rbintegrations/matrix/integration.py (Diff revision 2) Hm - it looks like we're hardcoding in the matrix.org homeserver in here. But we should make it so that custom home servers can be used, too. Can you add the server to the configuration form?
-
-
rbintegrations/extension.py (Diff revision 2) I just realized today that the integrations are in alphabetical order. So, you may want to move MatrixIntegration at lines 17 and 38 to lines 13 and 34 respectively, just before the MattermostIntegration
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+2766) |
Checks run (1 failed, 1 succeeded)
flake8
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+2768) |
Checks run (2 succeeded)
-
-
rbintegrations/basechat/integration.py (Diff revision 4) I agree with Hailan's review that this smells bad. Since we already have code that's checking the fields, let's pull out the
value
and then have a small method that replaces these three shortcode emojis with the unicode values. -
-
rbintegrations/matrix/forms.py (Diff revision 4) Please add a blank line between these two. The module docstring should also be clear that this is about matrix.
-
rbintegrations/matrix/forms.py (Diff revision 4) Does the capitalization listed here match the capitalization in the matrix app? If it's capitalized there, it should be here too.
Please add a period at the end of this help text.
-
-
rbintegrations/matrix/forms.py (Diff revision 4) Can we tweak the usage of "e.g." here?
"The server HTTP requests will be sent to, e.g., https://matrix.org"
-
-
-
rbintegrations/matrix/integration.py (Diff revision 4) Let's switch these to keep them in alphabetical order.
-
rbintegrations/matrix/integration.py (Diff revision 4) We need to be very careful regarding the use of
str
in Python, because it has different meanings in Python 2 and 3. In this case, everything is unicode so we shouldn't have to have any explicit casting.In general we prefer using format strings over the + operator for strings, but in this case since we're dealing with HTML, we should use
django.utils.html
'sformat_html
method:string = format_html('<strong>{}</strong><p>{}</p>', fallback_text, title)
This will ensure that no matter what is in those strings, we won't expose ourselves to possible XSS security problems.
-
rbintegrations/matrix/integration.py (Diff revision 4) Same comments here regarding
str
andformat_html
-
rbintegrations/matrix/integration.py (Diff revision 4) Same comments here regarding
str
andformat_html
-
rbintegrations/matrix/integration.py (Diff revision 4) This is probably fine to keep since it's a debug message, but let's use a named logger. At the top-level of the file, do:
logger = logging.getLogger(__name__)
Then here, change it to
logger.debug(...)
-
rbintegrations/matrix/integration.py (Diff revision 4) This would be better using a format string:
url = ('%s/_matrix/client/r0/rooms/%s/send/' 'm.room.message?access_token=%s' % (server, room_id, access_token))
-
-
rbintegrations/matrix/integration.py (Diff revision 4) Instead of using the continuation character here, can we just wrap it in parens?
-
rbintegrations/matrix/integration.py (Diff revision 4) It feels like the contents of the notify method could be moved back into this class. We're not calling it from anywhere else, and having it as its own top-level means we have that giant docstring appearing twice.
-
-
-
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+522 -18) |
Checks run (1 failed, 1 succeeded)
JSHint
-
rbintegrations/static/js/slack/authorize.js (Diff revision 5) Col: 10 'authorize' is defined but never used.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+2768) |
Checks run (2 succeeded)

-
Nice Work!
I am receiving a single test failure and it seems to be linked with encoding. (Test Case #57) Line 701 there is a little warning sign. Not sure whether this is intended in the test, or the integration is failing.
This also came up during manual testing. Changing the description or posting a code review succesfully displays in a Matrix Chatroom. However, when creating a general comment the request fails, due to some kind ofUnicodeEncodeError: 'ascii' codec can't u'\u26a0' in position 0: ordinal not in range(128)
. This is most likely due to the symbol, and I imagine something similar would occur if there was an error symbol.
Lastly, it would be nice if you could include pictures in the CR, similar to how Hailan has done it. This way others can download the correct images, because of this I was not able to see any of the Matrix logos. -
rbintegrations/extension.py (Diff revision 6) This should be moved, so that it is in alphabetical order.
-
rbintegrations/matrix/integration.py (Diff revision 6) Lines 10 and 11 should be swapped, so that the alphabetical order is maintained.
-

-
-
rbintegrations/matrix/forms.py (Diff revision 6) I forgot to include that I think that this field should have a default value set to
https://matrix.org
. I am not sure when this would be changed, maybe in the future if matrix chooses to change their server URL?It seems to be the same whether you are using the downloaded application or the web browser.
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+2820 -30) |
Checks run (1 failed, 1 succeeded)
flake8
-
-
-
rbintegrations/matrix/integration.py (Diff revision 7) E127 continuation line over-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 7) E127 continuation line over-indented for visual indent
-
-
rbintegrations/matrix/integration.py (Diff revision 7) E127 continuation line over-indented for visual indent
-
-
-
rbintegrations/matrix/integration.py (Diff revision 7) E128 continuation line under-indented for visual indent
Commits: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+2826 -40) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/matrix/integration.py (Diff revision 8) E128 continuation line under-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 8) E128 continuation line under-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 8) E128 continuation line under-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 8) E127 continuation line over-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 8) E128 continuation line under-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 8) E127 continuation line over-indented for visual indent
-
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+2678) |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/matrix/integration.py (Diff revision 9) E127 continuation line over-indented for visual indent
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+2678) |
Checks run (2 succeeded)
-
-
-
-
-
rbintegrations/matrix/integration.py (Diff revision 10) It might be nice to change this to use
re.sub
and pass a function for therepl
argument. That way we can do all the replacements in a single pass, which will be more efficient as the list of unicode shortcodes grows. -
rbintegrations/matrix/integration.py (Diff revision 10) This should use
format_html
fromdjango.utils.html
. -
rbintegrations/matrix/integration.py (Diff revision 10) This should use
format_html
fromdjango.utils.html
. Please also use single quotes. -
rbintegrations/matrix/integration.py (Diff revision 10) Please use single quotes throughout. Final item also needs a trailing comma.
-
-
-
-
-
-
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+2738 -16) |
||||||||||||
Added Files: |
Checks run (1 failed, 1 succeeded)
flake8
-
rbintegrations/matrix/integration.py (Diff revision 11) E128 continuation line under-indented for visual indent
-
rbintegrations/matrix/integration.py (Diff revision 11) E128 continuation line under-indented for visual indent
Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+2724) |
Checks run (2 succeeded)
-
This is great! Pretty much all my comments are stylistic in nature. Once fixed up, this will be ready to land!
-
-
rbintegrations/matrix/forms.py (Diff revision 12) It looks like the text is wrapping too early. You should be able to fit more per line, I think?
-
-
-
-
rbintegrations/matrix/integration.py (Diff revision 12) This should be
replace_shortcode
.It should also go on the class, since it's specific to this implementation.
-
rbintegrations/matrix/integration.py (Diff revision 12) Docstring summaries must be one line, without wrapping, and must fit within column bounds.
-
rbintegrations/matrix/integration.py (Diff revision 12) How about: "... replaced by their equivalent Unicode characters."
-
-
-
rbintegrations/matrix/integration.py (Diff revision 12) We should define the regex as a constant and compile it, so this is faster every time it needs to post.
-
rbintegrations/matrix/integration.py (Diff revision 12) We don't need to mention Slack here, since this isn't related to Slack's implementation, right?
-
rbintegrations/matrix/integration.py (Diff revision 12) This should be localized using
ugettext_lazy
. -
rbintegrations/matrix/integration.py (Diff revision 12) Mind sorting these alphabetically? It helps with maintenance.
-
rbintegrations/matrix/integration.py (Diff revision 12) This will be nicer if formatted as:
ASSETS_BASE_URL = \ 'https://...'
-
rbintegrations/matrix/integration.py (Diff revision 12) Since this is newly-added, let's pick a more modern date and time (like today's).
-
rbintegrations/matrix/integration.py (Diff revision 12) The order of arguments here doesn't match the documented order. Can you fix that up?
-
-
-
rbintegrations/matrix/integration.py (Diff revision 12) String building is faster using format strings (
%
). -
-
rbintegrations/matrix/integration.py (Diff revision 12) This will need an
.encode('utf-8')
after it. -
rbintegrations/matrix/integration.py (Diff revision 12) When formatting multi-line strings/arguments, we generally format it like:
url = ( '...' '...' % (...) )
-
-
rbintegrations/matrix/integration.py (Diff revision 12) Old code probably had an old pattern for this, but you can get rid of
exc_info=
and instead uselogger.exception(...)
. -
rbintegrations/matrix/integration.py (Diff revision 12) Can this just include
format_link
's code from above? I don't think we need this split up at all. -
rbintegrations/matrix/tests.py (Diff revision 12) Completely optional request, but it'd be more maintainable if these could be broken up around HTML tags a bit better, so it's easier to really digest the code and tweak it later if needed, like:
... '<p>#1: Test Review request</p>' '<strong><font color=#efcc96>Description</font>' ...
-
-
-
rbintegrations/matrix/tests.py (Diff revision 12) Formatting here isn't consistent with other tests (or style guidelines). The keys should be on their own lines.
This applies to all occurrences below.
-
rbintegrations/matrix/tests.py (Diff revision 12) You'll need to pass
id=10000
as a parameter tocreate_review_request()
to avoid an issue that can impact other tests. -
-
-
-
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 13 (+2770) |
Checks run (2 succeeded)
-
Looking good! I found a few small tweaks we can make to wording.
The biggest thing in here though is a security vulnerability in building the HTML to send. It's an easy fix.
format_html()
is just being called wrong. -
rbintegrations/matrix/forms.py (Diff revision 13) "profile" or "Profile"?
Can you change ">" to "->"? It's more standard.
-
-
rbintegrations/matrix/forms.py (Diff revision 13) Let's put the "e.g., https://matrix.org" in parens, and end with a period after.
-
-
rbintegrations/matrix/integration.py (Diff revision 13) For
format_html()
, it's very important that we differentiate between what's HTML and what's text going into it. We don't want a title of<script>EVIL THING</script>
going in.So this should be:
string = format_html('<strong>{}</strong><p>{}</p>', fallback_text, title)
Also,
string
is reserved in Python, so let's call ithtml
. -
rbintegrations/matrix/integration.py (Diff revision 13) This will also need to use
{}
and arguments when callingformat_html()
.Also, HTML attribute values should have
"
around them. -
rbintegrations/matrix/integration.py (Diff revision 13) String concatenation in Python isn't very efficient. Rather than doing this, a common pattern is:
html = [] html.append(...) html.append(...) ... payload = { 'formatted_body': ''.join(html), }
Basically, for the most part, joining strings is faster than concatenating strings.
-
-
-
rbintegrations/matrix/integration.py (Diff revision 13) The pattern should be compiled for the instance in
__init__
, rather than each time this function is called (otherwise it doesn't provide any performance gain over just passing a pattern into are
function).Once you compile, you'll call
.sub(...)
on the pattern instead. -
rbintegrations/matrix/tests.py (Diff revision 13) This should be a
reviewboard.integrations.models.IntegrationConfig
.