add Matrix Integration option

Review Request #11219 — Created Oct. 14, 2020 and submitted

ruonanjia
rbintegrations
master
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 attachment

tests with errors are:
- test_notify_new_review_request_with_image_file_attachment
- test_notify_updated_review_request_with_new_image_attachments

Summary Author
Add Matrix Integration option
ruonan

Description From Last Updated

I think you might need to uncommit the files in the .idea/ directory since they are from PyCharm

hailanhailan

Can you please attach the icon.png and icon@2x.png files to the review request?

daviddavid

Hmmm... the mentors will have the final say on this. Personally, I would move the matrixValue lines to MatrixIntegration, since ...

hailanhailan

I just realized today that the integrations are in alphabetical order. So, you may want to move MatrixIntegration at lines ...

hailanhailan

Hm - it looks like we're hardcoding in the matrix.org homeserver in here. But we should make it so that ...

mike_conleymike_conley

'Slack' to 'Matrix'

hailanhailan

'Slack' to 'Matrix'

hailanhailan

Does Matrix also require urls to be formatted like this? If so, maybe replace Slack/Mattermost with Matrix in the comments

hailanhailan

'Slack' -> 'Matrix'

hailanhailan

E501 line too long (87 > 79 characters)

reviewbotreviewbot

I agree with Hailan's review that this smells bad. Since we already have code that's checking the fields, let's pull ...

daviddavid

Please keep this list in alphabetical order.

daviddavid

Please add a blank line between these two. The module docstring should also be clear that this is about matrix.

daviddavid

Does the capitalization listed here match the capitalization in the matrix app? If it's capitalized there, it should be here ...

daviddavid

Please add a period at the end of this help text.

daviddavid

Can we tweak the usage of "e.g." here? "The server HTTP requests will be sent to, e.g., https://matrix.org"

daviddavid

This can wrap a little bit tighter.

daviddavid

Please add a module docstring to this file.

daviddavid

Let's switch these to keep them in alphabetical order.

daviddavid

We need to be very careful regarding the use of str in Python, because it has different meanings in Python ...

daviddavid

Same comments here regarding str and format_html

daviddavid

Same comments here regarding str and format_html

daviddavid

This is probably fine to keep since it's a debug message, but let's use a named logger. At the top-level ...

daviddavid

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

daviddavid

This blank line can go away.

daviddavid

Instead of using the continuation character here, can we just wrap it in parens?

daviddavid

It feels like the contents of the notify method could be moved back into this class. We're not calling it ...

daviddavid

Leftover debug output?

daviddavid

Leftover debug output?

daviddavid

Leftover debug output?

daviddavid

Col: 10 'authorize' is defined but never used.

reviewbotreviewbot

This should be moved, so that it is in alphabetical order.

jblazusijblazusi

I forgot to include that I think that this field should have a default value set to https://matrix.org. I am ...

jblazusijblazusi

Lines 10 and 11 should be swapped, so that the alphabetical order is maintained.

jblazusijblazusi

This comment should end with a period.

jblazusijblazusi

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (3)

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E302 expected 2 blank lines, found 1

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

Please add a second blank line here.

daviddavid

This needs a docstring.

daviddavid

It might be nice to change this to use re.sub and pass a function for the repl argument. That way ...

daviddavid

This should use format_html from django.utils.html.

daviddavid

This should use format_html from django.utils.html. Please also use single quotes.

daviddavid

Please use single quotes throughout. Final item also needs a trailing comma.

daviddavid

This needs a module docstring.

daviddavid

This needs a class docstring.

daviddavid

Please put this """ on its own line.

daviddavid

This needs a trailing comma.

daviddavid

This line can go away.

daviddavid

Please add a docstring for this.

daviddavid

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

"Matrix"

chipx86chipx86

It looks like the text is wrapping too early. You should be able to fit more per line, I think?

chipx86chipx86

Same here.

chipx86chipx86

will be sent to.

chipx86chipx86

... and clicking on the "Advanced" tab.

chipx86chipx86

This should be replace_shortcode. It should also go on the class, since it's specific to this implementation.

chipx86chipx86

Docstring summaries must be one line, without wrapping, and must fit within column bounds.

chipx86chipx86

How about: "... replaced by their equivalent Unicode characters."

chipx86chipx86

No blank line here.

chipx86chipx86

emoji_unicode

chipx86chipx86

We should define the regex as a constant and compile it, so this is faster every time it needs to ...

chipx86chipx86

We don't need to mention Slack here, since this isn't related to Slack's implementation, right?

chipx86chipx86

This should be localized using ugettext_lazy.

chipx86chipx86

Mind sorting these alphabetically? It helps with maintenance.

chipx86chipx86

This will be nicer if formatted as: ASSETS_BASE_URL = \ 'https://...'

chipx86chipx86

Since this is newly-added, let's pick a more modern date and time (like today's).

chipx86chipx86

The order of arguments here doesn't match the documented order. Can you fix that up?

chipx86chipx86

This needs , optional.

chipx86chipx86

This needs , optional.

chipx86chipx86

String building is faster using format strings (%).

chipx86chipx86

Same here.

chipx86chipx86

This will need an .encode('utf-8') after it.

chipx86chipx86

When formatting multi-line strings/arguments, we generally format it like: url = ( '...' '...' % (...) )

chipx86chipx86

Mind alphabetizing these while you're here?

chipx86chipx86

Old code probably had an old pattern for this, but you can get rid of exc_info= and instead use logger.exception(...).

chipx86chipx86

Can this just include format_link's code from above? I don't think we need this split up at all.

chipx86chipx86

Completely optional request, but it'd be more maintainable if these could be broken up around HTML tags a bit better, ...

chipx86chipx86

Feels like this wraps too early.

chipx86chipx86

Blank line betewen these.

chipx86chipx86

Formatting here isn't consistent with other tests (or style guidelines). The keys should be on their own lines. This applies ...

chipx86chipx86

You'll need to pass id=10000 as a parameter to create_review_request() to avoid an issue that can impact other tests.

chipx86chipx86

Looks like a ! snuck in here.

chipx86chipx86

Missing a "Returns".

chipx86chipx86

Missing , optional.

chipx86chipx86

Is this doable at this stage?

chipx86chipx86

"profile" or "Profile"? Can you change ">" to "->"? It's more standard.

chipx86chipx86

on the "Advanced" tab.

chipx86chipx86

Let's put the "e.g., https://matrix.org" in parens, and end with a period after.

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

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

chipx86chipx86

This will also need to use {} and arguments when calling format_html(). Also, HTML attribute values should have " around ...

chipx86chipx86

String concatenation in Python isn't very efficient. Rather than doing this, a common pattern is: html = [] html.append(...) html.append(...) ...

chipx86chipx86

Blank line between these.

chipx86chipx86

No blank line here.

chipx86chipx86

The pattern should be compiled for the instance in __init__, rather than each time this function is called (otherwise it ...

chipx86chipx86

This should be a reviewboard.integrations.models.IntegrationConfig.

chipx86chipx86
ruonanjia
ruonanjia
ruonanjia
hailan
  1. I haven't tested it yet, but it looks pretty awesome. Great job!
    Just a few small things that need to be fixed, like changing 'Slack' to 'Matrix'.
    There are a few more places in the notify() function with the same problem that I didn't comment on.
    I don't want to spam your RR, so just gonna mention it here. Ctrl-F will be your best friend ;)

  2. I think you might need to uncommit the files in the .idea/ directory since they are from PyCharm

  3. 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'"
  4. rbintegrations/matrix/integration.py (Diff revision 2)
     
     
     

    'Slack' to 'Matrix'

  5. rbintegrations/matrix/integration.py (Diff revision 2)
     
     

    'Slack' to 'Matrix'

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

  7. rbintegrations/matrix/integration.py (Diff revision 2)
     
     

    'Slack' -> 'Matrix'

  8. 
      
mike_conley
  1. 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.

  2. 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?

  3. 
      
hailan
  1. 
      
  2. 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

  3. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
david
  1. 
      
  2. 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.

  3. rbintegrations/extension.py (Diff revision 4)
     
     
     
     
     
     
     

    Please keep this list in alphabetical order.

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

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

  6. rbintegrations/matrix/forms.py (Diff revision 4)
     
     

    Please add a period at the end of this help text.

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

  8. rbintegrations/matrix/forms.py (Diff revision 4)
     
     
     

    This can wrap a little bit tighter.

  9. rbintegrations/matrix/integration.py (Diff revision 4)
     
     

    Please add a module docstring to this file.

  10. rbintegrations/matrix/integration.py (Diff revision 4)
     
     
     

    Let's switch these to keep them in alphabetical order.

  11. 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's format_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.

  12. rbintegrations/matrix/integration.py (Diff revision 4)
     
     
     
     
     
     
     

    Same comments here regarding str and format_html

  13. rbintegrations/matrix/integration.py (Diff revision 4)
     
     

    Same comments here regarding str and format_html

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

  15. 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))
    
  16. rbintegrations/matrix/integration.py (Diff revision 4)
     
     

    This blank line can go away.

  17. rbintegrations/matrix/integration.py (Diff revision 4)
     
     
     

    Instead of using the continuation character here, can we just wrap it in parens?

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

  19. rbintegrations/matrix/tests.py (Diff revision 4)
     
     

    Leftover debug output?

  20. rbintegrations/matrix/tests.py (Diff revision 4)
     
     

    Leftover debug output?

  21. rbintegrations/matrix/tests.py (Diff revision 4)
     
     

    Leftover debug output?

  22. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

ruonanjia
jblazusi
  1. 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 of UnicodeEncodeError: '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.

    1. Can't reproduce the error on my end

    2. This is an image of the traceback. Perhaps the mentors have a better understanding of what is going on.
      Image

    3. Would it be possible to include the Matrix .png logos into the CR? It makes it possible to test if the images are displaying correctly, during manual testing.

  2. rbintegrations/extension.py (Diff revision 6)
     
     

    This should be moved, so that it is in alphabetical order.

  3. rbintegrations/matrix/integration.py (Diff revision 6)
     
     

    Lines 10 and 11 should be swapped, so that the alphabetical order is maintained.

  4. rbintegrations/matrix/integration.py (Diff revision 6)
     
     

    This comment should end with a period.

  5. 
      
jblazusi
  1. 
      
  2. 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.

    1. Gives option to use custom home servers as mentioned by Mike above ^

  3. 
      
ruonanjia
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
Review request changed

Commits:

Summary Author
-
add Matrix Integration option
ruonan
-
add Matrix Integration option
ruonan
+
add Matrix Integration option
ruonan
+
add Matrix Integration option
ruonan
+
add Matrix Integration option
ruonan

Diff:

Revision 8 (+2826 -40)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
Review request changed

Commits:

Summary Author
-
add Matrix Integration option
ruonan
-
add Matrix Integration option
ruonan
-
add Matrix Integration option
ruonan
+
add Matrix Integration option
ruonan

Diff:

Revision 9 (+2678)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
ruonanjia
david
  1. 
      
  2. Can you please attach the icon.png and icon@2x.png files to the review request?

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

    Please add a second blank line here.

  4. rbintegrations/matrix/integration.py (Diff revision 10)
     
     

    This needs a docstring.

  5. rbintegrations/matrix/integration.py (Diff revision 10)
     
     
     

    It might be nice to change this to use re.sub and pass a function for the repl 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.

  6. rbintegrations/matrix/integration.py (Diff revision 10)
     
     
     

    This should use format_html from django.utils.html.

  7. rbintegrations/matrix/integration.py (Diff revision 10)
     
     

    This should use format_html from django.utils.html. Please also use single quotes.

  8. rbintegrations/matrix/integration.py (Diff revision 10)
     
     
     
     
     

    Please use single quotes throughout. Final item also needs a trailing comma.

  9. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    This needs a module docstring.

  10. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    This needs a class docstring.

  11. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    Please put this """ on its own line.

  12. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    This needs a trailing comma.

  13. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    This line can go away.

  14. rbintegrations/matrix/tests.py (Diff revision 10)
     
     

    Please add a docstring for this.

  15. 
      
ruonanjia
Review request changed

Commits:

Summary Author
-
add Matrix Integration option
ruonan
+
add Matrix Integration option
ruonan
+
Matrix Integration
ruonan

Diff:

Revision 11 (+2738 -16)

Show changes

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

ruonanjia
ruonanjia
  1. 
      
  2. 
      
chipx86
  1. This is great! Pretty much all my comments are stylistic in nature. Once fixed up, this will be ready to land!

  2. rbintegrations/matrix/forms.py (Diff revision 12)
     
     

    "Matrix"

  3. 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?

  4. rbintegrations/matrix/forms.py (Diff revision 12)
     
     
     
     
     

    Same here.

  5. rbintegrations/matrix/forms.py (Diff revision 12)
     
     

    will be sent to.

  6. rbintegrations/matrix/forms.py (Diff revision 12)
     
     

    ... and clicking on the "Advanced" tab.

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

  8. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     

    Docstring summaries must be one line, without wrapping, and must fit within column bounds.

  9. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    How about: "... replaced by their equivalent Unicode characters."

  10. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     
     

    No blank line here.

  11. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    emoji_unicode

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

  13. 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?

  14. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     
     
     
     

    This should be localized using ugettext_lazy.

  15. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     
     

    Mind sorting these alphabetically? It helps with maintenance.

  16. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     

    This will be nicer if formatted as:

    ASSETS_BASE_URL = \
        'https://...'
    
  17. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    Since this is newly-added, let's pick a more modern date and time (like today's).

  18. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     
     

    The order of arguments here doesn't match the documented order. Can you fix that up?

  19. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    This needs , optional.

  20. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    This needs , optional.

  21. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     

    String building is faster using format strings (%).

  22. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     
     

    Same here.

  23. rbintegrations/matrix/integration.py (Diff revision 12)
     
     

    This will need an .encode('utf-8') after it.

  24. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     

    When formatting multi-line strings/arguments, we generally format it like:

    url = (
      '...'
      '...'
      % (...)
    )
    
  25. rbintegrations/matrix/integration.py (Diff revision 12)
     
     
     

    Mind alphabetizing these while you're here?

  26. 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 use logger.exception(...).

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

  28. 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>'
    ...
    
  29. rbintegrations/matrix/tests.py (Diff revision 12)
     
     
     

    Feels like this wraps too early.

  30. rbintegrations/matrix/tests.py (Diff revision 12)
     
     
     

    Blank line betewen these.

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

  32. rbintegrations/matrix/tests.py (Diff revision 12)
     
     
     
     
     
     
     
     

    You'll need to pass id=10000 as a parameter to create_review_request() to avoid an issue that can impact other tests.

  33. rbintegrations/matrix/tests.py (Diff revision 12)
     
     

    Looks like a ! snuck in here.

  34. rbintegrations/matrix/tests.py (Diff revision 12)
     
     
     
     
     
     
     

    Missing a "Returns".

  35. rbintegrations/matrix/tests.py (Diff revision 12)
     
     

    Missing , optional.

  36. rbintegrations/matrix/tests.py (Diff revision 12)
     
     
     

    Is this doable at this stage?

    1. I'm getting this error when I try to add the images test: [No such file or directory: '/var/folders/jm/c43j59q96716g48s49cbw9dc0000gn/T/rb-tests-6lqol9qg/static/rb/images/logo.png']

    2. Hi Ruonan, I'm wondering if you tried running the tests in the rb3 virtual environment?
      We might be able to add the tests, and then fix the bug in another Review Request in the future.
      I think it was David who suggested that the cause might be from a python2 python3 difference.

    3. I found the cause this past week. Pull down the latest from the Review Board tree, and this problem will be fixed.

  37. 
      
ruonanjia
chipx86
  1. 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.

  2. rbintegrations/matrix/forms.py (Diff revision 13)
     
     

    "profile" or "Profile"?

    Can you change ">" to "->"? It's more standard.

  3. rbintegrations/matrix/forms.py (Diff revision 13)
     
     

    on the "Advanced" tab.

  4. rbintegrations/matrix/forms.py (Diff revision 13)
     
     
     

    Let's put the "e.g., https://matrix.org" in parens, and end with a period after.

  5. rbintegrations/matrix/forms.py (Diff revision 13)
     
     

    Missing a trailing comma.

  6. 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 it html.

  7. rbintegrations/matrix/integration.py (Diff revision 13)
     
     
     
     

    This will also need to use {} and arguments when calling format_html().

    Also, HTML attribute values should have " around them.

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

  9. rbintegrations/matrix/integration.py (Diff revision 13)
     
     
     

    Blank line between these.

  10. rbintegrations/matrix/integration.py (Diff revision 13)
     
     
     
     

    No blank line here.

  11. 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 a re function).

    Once you compile, you'll call .sub(...) on the pattern instead.

  12. rbintegrations/matrix/tests.py (Diff revision 13)
     
     

    This should be a reviewboard.integrations.models.IntegrationConfig.

  13. 
      
ruonanjia
  1. 
      
  2. 
      
david
  1. Making some tweaks and getting this landed. Thanks!

  2. 
      
ruonanjia
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (7ee699f)
Loading...