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

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

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

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

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.

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?

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

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.

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 too.
Please add a period at the end of this help text.

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

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 of the file, do:

logger = logging.getLogger(__name__)


Then here, change it to logger.debug(...)

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 from anywhere else, and having it as its own top-level means we have that giant docstring appearing twice.

Leftover debug output?

Leftover debug output?

Leftover debug output?

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.

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

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

This comment should end with a period.

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.

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

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 we can do all the replacements in a single pass, which will be more efficient as the list of unicode shortcodes grows.

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.

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

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

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

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 to all occurrences below.

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?

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.

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

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

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.

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

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

Making some tweaks and getting this landed. Thanks!