add Matrix Integration option

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

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

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
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
Review request changed
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. 
      
Loading...