Please remove.

TODO comment?

Should this be Discord instead?

Should this be Discord instead?

Should this be Discord instead?

Should this be Discord instead?

It's more of a nitpick but 'mattemost' -> 'discord'

Looking pretty solid. Just some style-related comments.

Let's use double quotes for the quotation marks inside the string.

Please add a blank line between these two.

Maybe move the contents of this method back into the integration class? I don't see a good reason to have a separate top-level function here.

Please use single quotes for these.

Can we use a named logger here? At the top level of the file, add:

logger = logging.getLogger(__name__)


This can then be logger.debug(...)

There's an extra blank line here that can be removed.

Please add a module docstring to the top of this file.

Add a blank line between these two.

Leftover debug output?

This line isn't necessary.

Looking very solid!

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

I don't know much about Discord Integration, so I just nit-picked on some styling issue.

As per the coding style guide, I believe these imports should be moved down to a new group as they are Project imports?

Missing period here.

Should be in alphabetical order.

These imports should be moved down a group too.

Let's switch the use of single- and double-quotes here:

help_text=_('Your unique Discord webhook URL. This can be found at '
            'the "Integration" page after selecting '
            '"Server Settings".')

This looks great! Most of my comments are just small style nits for code/docs.

These two lines should be swapped (alphabetical order).

Two blank lines between imports and other code.

This should use ugettext_lazy, like the form does.

Mind putting this in alphabetical order? Helps keep things organized as new settings are added.

You can bump this to a modern date and time.

This appears to be in the wrong spot. Should be further down, under event_name, and have , optional.

Should have , optional

Can you pass these as keyword arguments? Helps avoid issues as parameters get added, and keeps code readable.

No need for the blank line here. We can convey this as one "paragraph", where we're building the initial state of the payload.

Can you use '%s/slack' % config.... form?

Can you set this to alphabetical order?

If you use logger.exception, you won't need exc_info=True.

Doesn't look like this function is doing that, at least not directly. Maybe state what it wraps, so it's clear where to go for docs?

We don't need docstrings for setUp or tearDown. Sort of an exception to a rule.

You can pass id=12321 to create_review_request() (in fact, it's important to do that — old code may not though).

Same here with id=.

No need to call this, since we already created it above.

This will need a docstring.

This can be a single statement.

These all list , optional, but the arguments are all required in the method signature.

Can you pass all these as keyword arguments?

I just noticed that Christian beat me to the review, so hopefully these aren't duplicates.

Otherwise, I manually tested the integration on my own discord server, and it works wonderfully.

I also ran the tests and all of them worked on RB3 but strangely there are 2 failing test cases on RB4. 

However, these are already documented on the Notion documents.

I think that lines 11 and 12 should be swapped so that they follow alphabetical order.

I noticed that other review board files have 2 lines below the import statements. So I think that you should have 2 lines after the last import statement for your logger = logging.getLogger(__name__) and then 2 lines below before the class definition.

I have to add .encode('UTF-8) at the end of this line to get it to work for some reason. Running into this issue when I try to get a message to show up on discord: TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
Other than that, tested it and works well!

Just a few small things, and then this will land :)

Can you say "integration on Discord", so it's clear where they need to add this?

This has two spellings of "Webhook": "Webhook" and "WebHook". Not your fault — we're inconsistent in places as well. I think we should keep any non-title versions "webhook" (no capitalizations), as this is more standard these days.

"that that" -> "that"

No space after the comma.

lowercase: "utf-8".

To check, is there a reason we use this User-Agent? If so, we should document this. If not, we shouldn't include it.

Let's use "webhook".

This is indented too far.

Should be "dict".

Since there's only one, how about just attachment?

It's best to avoid one-line dictionaries with multiple keys, as it becomes harder to add to. Let's do:

return {
    'attachments': [attachments],
    'icon_url': integration.LOGO_URL,
}


Also note the alphabetical order.

Ship It!