Discord Integration Option

Review Request #11201 — Created Sept. 24, 2020 and submitted

hailan
rbintegrations
master
rbintegrations, students

Added a Discord Integration option.
This allows Review Board to have an option of notifying Discord channels on
any Review Request changes. This commit also includes a refactoring of the
notify() function in SlackIntegration.

Added unit tests in discord/tests.py and they pass in RB3 virtual
environment. RB4 has two exceptions (please see below).

Tested it manually in reviewboard 3.0.x and 4.0.x.

Issues that still need to be addressed:

  • Add assets (e.g. logos, trophies, etc.) at this location:
    https://static.reviewboard.org/integration-assets/discord

  • It has the same problem as the Slack and Mattermost Integration:
    two of the unit tests are complaining about this image here in RB4 -
    /var/folders/f1/r8mpbfkd321804b47_tgytgw0000gn/T/rb-tests-EQOxMU/
    static/rb/images/logo.png

Summary Author
Discord Chat Integration
HailanXyouknow

Description From Last Updated

E261 at least two spaces before inline comment

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

E261 at least two spaces before inline comment

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

Please remove.

MarcusBoayMarcusBoay

TODO comment?

MarcusBoayMarcusBoay

Should this be Discord instead?

MarcusBoayMarcusBoay

Should this be Discord instead?

MarcusBoayMarcusBoay

Should this be Discord instead?

MarcusBoayMarcusBoay

Should this be Discord instead?

MarcusBoayMarcusBoay

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

MarcusBoayMarcusBoay

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

daviddavid

Please add a blank line between these two.

daviddavid

Maybe move the contents of this method back into the integration class? I don't see a good reason to have ...

daviddavid

Please use single quotes for these.

daviddavid

Can we use a named logger here? At the top level of the file, add: logger = logging.getLogger(__name__) This can ...

daviddavid

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

daviddavid

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

daviddavid

Add a blank line between these two.

daviddavid

Leftover debug output?

daviddavid

This line isn't necessary.

daviddavid

E302 expected 2 blank lines, found 1

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

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

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

daviddavid

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

keanwengkeanweng

Missing period here.

keanwengkeanweng

Should be in alphabetical order.

keanwengkeanweng

These imports should be moved down a group too.

keanwengkeanweng

Let's switch the use of single- and double-quotes here: help_text=_('Your unique Discord webhook URL. This can be found at ' ...

daviddavid

These two lines should be swapped (alphabetical order).

chipx86chipx86

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

jblazusijblazusi

Two blank lines between imports and other code.

chipx86chipx86

I noticed that other review board files have 2 lines below the import statements. So I think that you should ...

jblazusijblazusi

This should use ugettext_lazy, like the form does.

chipx86chipx86

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

chipx86chipx86

You can bump this to a modern date and time.

chipx86chipx86

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

chipx86chipx86

Should have , optional

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Can you set this to alphabetical order?

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same here with id=.

chipx86chipx86

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

chipx86chipx86

This will need a docstring.

chipx86chipx86

This can be a single statement.

chipx86chipx86

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

chipx86chipx86

Can you pass all these as keyword arguments?

chipx86chipx86

E201 whitespace after '{'

reviewbotreviewbot

E202 whitespace before '}'

reviewbotreviewbot

I have to add .encode('UTF-8) at the end of this line to get it to work for some reason. Running ...

ruonanjiaruonanjia

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

chipx86chipx86

This has two spellings of "Webhook": "Webhook" and "WebHook". Not your fault — we're inconsistent in places as well. I ...

chipx86chipx86

"that that" -> "that"

chipx86chipx86

No space after the comma.

chipx86chipx86

lowercase: "utf-8".

chipx86chipx86

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

chipx86chipx86

Let's use "webhook".

chipx86chipx86

This is indented too far.

chipx86chipx86

Should be "dict".

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

hailan
Review request changed

Change Summary:

Using Slack's format for Discord webhook messages. It looks cleaner
this way - we don't need to do weird things like translating hex
color code to decimal numbers

Description:

~  

Looks like this right now.

~   Still trying to figure out how to pass in all the relevant fields.
~   Image

  ~

Admins can add a Discord Integration to their Review Board. After

  ~ setting it up, it will alert them on the changes of Review Request
  ~ in the specified Discord Server.

Commits:

Summary Author
-
Discord Chat Integration
HailanXyouknow
+
Discord Chat Integration
HailanXyouknow

Diff:

Revision 2 (+662 -38)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
hailan
hailan
  1. 
      
  2. 
      
hailan
MarcusBoay
  1. 
      
  2. rbintegrations/discord/integration.py (Diff revision 4)
     
     

    Please remove.

  3. rbintegrations/discord/integration.py (Diff revision 4)
     
     

    TODO comment?

  4. rbintegrations/discord/integration.py (Diff revision 4)
     
     

    Should this be Discord instead?

  5. rbintegrations/discord/integration.py (Diff revision 4)
     
     

    Should this be Discord instead?

    1. I've added an explanation on why this it is using Slack's URL. :)

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

    Should this be Discord instead?

  7. rbintegrations/discord/integration.py (Diff revision 4)
     
     

    Should this be Discord instead?

  8. rbintegrations/discord/tests.py (Diff revision 4)
     
     

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

  9. 
      
hailan
hailan
david
  1. Looking pretty solid. Just some style-related comments.

  2. rbintegrations/discord/forms.py (Diff revision 5)
     
     
     
     

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

  3. rbintegrations/discord/integration.py (Diff revision 5)
     
     
     

    Please add a blank line between these two.

  4. rbintegrations/discord/integration.py (Diff revision 5)
     
     
     
     

    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.

  5. rbintegrations/discord/integration.py (Diff revision 5)
     
     
     

    Please use single quotes for these.

  6. rbintegrations/discord/integration.py (Diff revision 5)
     
     

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

  7. rbintegrations/discord/integration.py (Diff revision 5)
     
     

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

  8. rbintegrations/discord/tests.py (Diff revision 5)
     
     

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

  9. rbintegrations/discord/tests.py (Diff revision 5)
     
     
     

    Add a blank line between these two.

  10. rbintegrations/discord/tests.py (Diff revision 5)
     
     
     

    Leftover debug output?

  11. rbintegrations/slack/integration.py (Diff revision 5)
     
     

    This line isn't necessary.

  12. 
      
hailan
Review request changed

Change Summary:

Addressed David's feedback.
Tested DiscordIntegration manually and reran the unit tests.

Commits:

Summary Author
-
Discord Chat Integration
HailanXyouknow
+
Discord Chat Integration
HailanXyouknow

Diff:

Revision 6 (+3844 -38)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
david
  1. Looking very solid!

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

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

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

    1. Thanks you! 
      I was able to learn why some modules are grouped together through this review.
  2. rbintegrations/discord/forms.py (Diff revision 9)
     
     
     

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

    1. I went through other integrations and saw that reviewboard modules are grouped together with django and djblets modules.
      I think this is because reviewboard modules are considered "modules not belonging to the current project codebase" as per the style guide.
      Will wait for mentors to confirm this.

    2. That's correct. From the point of view of the rbintegrations package, reviewboard is a third-party module just like django.

  3. rbintegrations/discord/integration.py (Diff revision 9)
     
     

    Missing period here.

  4. rbintegrations/discord/tests.py (Diff revision 9)
     
     
     

    Should be in alphabetical order.

    1. These lines are already in alphabetical order contrib > utils

  5. rbintegrations/discord/tests.py (Diff revision 9)
     
     
     
     

    These imports should be moved down a group too.

    1. Same as above, will wait for confirmation from mentors.

  6. 
      
hailan
david
  1. 
      
  2. rbintegrations/discord/forms.py (Diff revision 10)
     
     
     
     

    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".')
    
  3. 
      
hailan
hailan
  1. 
      
  2. 
      
chipx86
  1. This looks great! Most of my comments are just small style nits for code/docs.

  2. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     

    These two lines should be swapped (alphabetical order).

  3. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     

    Two blank lines between imports and other code.

  4. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     
     

    This should use ugettext_lazy, like the form does.

  5. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     

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

  6. rbintegrations/discord/integration.py (Diff revision 11)
     
     

    You can bump this to a modern date and time.

  7. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     

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

  8. rbintegrations/discord/integration.py (Diff revision 11)
     
     

    Should have , optional

  9. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     

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

  10. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     

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

  11. rbintegrations/discord/integration.py (Diff revision 11)
     
     

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

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

    Can you set this to alphabetical order?

  13. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     

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

  14. rbintegrations/discord/integration.py (Diff revision 11)
     
     
     
     

    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?

  15. rbintegrations/discord/tests.py (Diff revision 11)
     
     

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

  16. rbintegrations/discord/tests.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     

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

  17. rbintegrations/discord/tests.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     

    Same here with id=.

  18. rbintegrations/discord/tests.py (Diff revision 11)
     
     

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

  19. rbintegrations/discord/tests.py (Diff revision 11)
     
     

    This will need a docstring.

  20. rbintegrations/slack/integration.py (Diff revision 11)
     
     
     
     
     
     

    This can be a single statement.

  21. rbintegrations/slack/integration.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

  22. rbintegrations/slack/integration.py (Diff revision 11)
     
     
     
     

    Can you pass all these as keyword arguments?

  23. 
      
jblazusi
  1. 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.

  2. rbintegrations/discord/integration.py (Diff revision 11)
     
     

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

  3. rbintegrations/discord/integration.py (Diff revision 11)
     
     

    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.

  4. 
      
hailan
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

hailan
ruonanjia
  1. 
      
  2. rbintegrations/discord/integration.py (Diff revision 13)
     
     

    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!

    1. Yeah, that'll be important on Python 3, and must be done before the length is calculated (so we get a length in bytes and not characters).

  3. 
      
hailan
chipx86
  1. Just a few small things, and then this will land :)

  2. rbintegrations/discord/forms.py (Diff revision 14)
     
     
     

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

  3. rbintegrations/discord/forms.py (Diff revision 14)
     
     
     
     

    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.

  4. rbintegrations/discord/forms.py (Diff revision 14)
     
     

    "that that" -> "that"

  5. rbintegrations/discord/forms.py (Diff revision 14)
     
     

    No space after the comma.

  6. rbintegrations/discord/integration.py (Diff revision 14)
     
     

    lowercase: "utf-8".

  7. rbintegrations/discord/integration.py (Diff revision 14)
     
     

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

    1. Yes, to avoid urllib blocking (403 error).
      I will add a comment in this section.

  8. rbintegrations/slack/integration.py (Diff revision 14)
     
     

    Let's use "webhook".

  9. rbintegrations/slack/integration.py (Diff revision 14)
     
     
     
     

    This is indented too far.

  10. rbintegrations/slack/integration.py (Diff revision 14)
     
     

    Should be "dict".

  11. rbintegrations/slack/integration.py (Diff revision 14)
     
     

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

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

    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.

  13. 
      
hailan
chipx86
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (133d51e).

Loading...